Apply suggestions from code review

Co-authored-by: Jonny Gerig Meyer <[email protected]>

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+    "editor.rulers": [80]
+}

index.html

@@ -810,24 +810,29 @@ <h2>
         Anchor declared in media query
       </h2>
       <div style="position: relative" class="demo-elements">
-        <div id="my-anchor-media-query" class="anchor">Anchor</div>
+        <div id="my-anchor-media-query" class="anchor">Screen Anchor</div>
+        <div id="my-print-anchor-media-query" class="anchor">Print Anchor</div>
         <div id="my-target-media-query" class="target">Target</div>
       </div>
       <p class="note">
-        With polyfill applied: Target is positioned at Anchor's bottom right
-        corner.
+        With polyfill applied: Target and Screen Anchor's right and top edges
+        line up.
       </p>
       <pre><code class="language-css"
->@media all {
-  #my-anchor-media-query {
+>@media print {
+  #my-print-anchor-media-query {
     anchor-name: --my-anchor-media-query;
   }
 }
 
+#my-anchor-media-query {
+  anchor-name: --my-anchor-media-query;
+}
+
 #my-target-media-query {
   position: absolute;
-  top: anchor(--my-anchor-media-query bottom);
-  left: anchor(--my-anchor-media-query right);
+  top: anchor(--my-anchor-media-query top);
+  right: anchor(--my-anchor-media-query right);
 }</code></pre>
     </section>
     <section id="sponsor">

public/anchor-media-query.css

@@ -1,11 +1,15 @@
-@media all {
-  #my-anchor-media-query {
+@media print {
+  #my-print-anchor-media-query {
     anchor-name: --my-anchor-media-query;
   }
 }
 
+#my-anchor-media-query {
+  anchor-name: --my-anchor-media-query;
+}
+
 #my-target-media-query {
   position: absolute;
-  top: anchor(--my-anchor-media-query bottom);
-  left: anchor(--my-anchor-media-query right);
+  top: anchor(--my-anchor-media-query top);
+  right: anchor(--my-anchor-media-query right);
 }

src/cascade.ts

@@ -4,9 +4,9 @@ import { nanoid } from 'nanoid/non-secure';
 import { generateCSS, getAST, isDeclaration, type StyleData } from './utils.js';
 
 /**
- * Map of CSS property to CSS custom property that the property's value is shifted into.
- * This is used to subject properties that are not yet natively supported to the CSS cascade and
- * inheritance rules.
+ * Map of CSS property to CSS custom property that the property's value is
+ * shifted into. This is used to subject properties that are not yet natively
+ * supported to the CSS cascade and inheritance rules.
  */
 export const SHIFTED_PROPERTIES: Record<string, string> = {
   'position-anchor': `--position-anchor-${nanoid(12)}`,
@@ -15,8 +15,8 @@ export const SHIFTED_PROPERTIES: Record<string, string> = {
 };
 
 /**
- * Shift property declarations for properties that are not yet natively supported into custom
- * properties.
+ * Shift property declarations for properties that are not yet natively
+ * supported into custom properties.
  */
 function shiftUnsupportedProperties(
   node: csstree.CssNode,
@@ -33,8 +33,8 @@ function shiftUnsupportedProperties(
 }
 
 /**
- * Update the given style data to enable cascading and inheritance of properties that are not yet
- * natively supported.
+ * Update the given style data to enable cascading and inheritance of properties
+ * that are not yet natively supported.
  */
 export function cascadeCSS(styleData: StyleData[]) {
   for (const styleObj of styleData) {

src/dom.ts

@@ -1,9 +1,11 @@
 import { type VirtualElement } from '@floating-ui/dom';
 import { nanoid } from 'nanoid/non-secure';
+
 import { SHIFTED_PROPERTIES } from './cascade.js';
 
 /**
- * Representation of a CSS selector that allows getting the element part and pseudo-element part.
+ * Representation of a CSS selector that allows getting the element part and
+ * pseudo-element part.
  */
 export interface Selector {
   selector: string;
@@ -21,7 +23,8 @@ export interface PseudoElement extends VirtualElement {
 }
 
 /**
- * Possible values for `anchor-scope` (in addition any valid dashed identifier)
+ * Possible values for `anchor-scope`
+ * (in addition to any valid dashed identifier)
  */
 export const enum AnchorScopeValue {
   All = 'all',
@@ -31,8 +34,8 @@ export const enum AnchorScopeValue {
 /**
  * Gets the computed value of a CSS property for an element or pseudo-element.
  *
- * Note: values for properties that are not natively supported are *awlways* subject to CSS
- * inheritance.
+ * Note: values for properties that are not natively supported are *always*
+ * subject to CSS inheritance.
  */
 export function getCSSPropertyValue(
   el: HTMLElement | PseudoElement,
@@ -45,10 +48,11 @@ export function getCSSPropertyValue(
 }
 
 /**
- * Checks whether a given element or pseudo-element has the given property value.
+ * Checks whether a given element or pseudo-element has the given property
+ * value.
  *
- * Note: values for properties that are not natively supported are *awlways* subject to CSS
- * inheritance.
+ * Note: values for properties that are not natively supported are *always*
+ * subject to CSS inheritance.
  */
 export function hasStyle(
   element: HTMLElement | PseudoElement,
@@ -65,16 +69,18 @@ function createFakePseudoElement(
   element: HTMLElement,
   { selector, pseudoElementPart }: Selector,
 ) {
-  // Floating UI needs `Element.getBoundingClientRect` to calculate the position for the anchored element,
-  // since there isn't a way to get it for pseudo-elements;
-  // we create a temporary "fake pseudo-element" that we use as reference.
+  // Floating UI needs `Element.getBoundingClientRect` to calculate the position
+  // for the anchored element, since there isn't a way to get it for
+  // pseudo-elements; we create a temporary "fake pseudo-element" that we use as
+  // reference.
   const computedStyle = getComputedStyle(element, pseudoElementPart);
   const fakePseudoElement = document.createElement('div');
   const sheet = document.createElement('style');
 
   fakePseudoElement.id = `fake-pseudo-element-${nanoid()}`;
 
-  // Copy styles from pseudo-element to the "fake pseudo-element", `.cssText` does not work on Firefox.
+  // Copy styles from pseudo-element to the "fake pseudo-element", `.cssText`
+  // does not work on Firefox.
   for (const property of Array.from(computedStyle)) {
     const value = computedStyle.getPropertyValue(property);
     fakePseudoElement.style.setProperty(property, value);
@@ -95,7 +101,7 @@ function createFakePseudoElement(
 }
 
 /**
- * Finds the first scollable parent of the given element
+ * Finds the first scrollable parent of the given element
  * (or the element itself if the element is scrollable).
  */
 function findFirstScrollingElement(element: HTMLElement) {
@@ -114,7 +120,7 @@ function findFirstScrollingElement(element: HTMLElement) {
 
 /**
  * Gets the scroll position of the first scrollable parent
- * (or the scoll position of the element itself, if it is scrollable).
+ * (or the scroll position of the element itself, if it is scrollable).
  */
 function getContainerScrollPosition(element: HTMLElement) {
   let containerScrollPosition: {
@@ -131,8 +137,8 @@ function getContainerScrollPosition(element: HTMLElement) {
 }
 
 /**
- * Like `document.querySelectorAll`, but if the selector has a pseudo-element it will return a
- * wrapper for the rest of the polyfill to use.
+ * Like `document.querySelectorAll`, but if the selector has a pseudo-element it
+ * will return a wrapper for the rest of the polyfill to use.
  */
 export function getElementsBySelector(selector: Selector) {
   const { elementPart, pseudoElementPart } = selector;
@@ -171,10 +177,11 @@ export function getElementsBySelector(selector: Selector) {
         sheet.remove();
       },
 
-      // For https://floating-ui.com/docs/autoupdate#ancestorscroll to work on `VirtualElement`s.
+      // For https://floating-ui.com/docs/autoupdate#ancestorscroll to work on
+      // `VirtualElement`s.
       contextElement: element,
 
-      // https://floating-ui.com/docs/virtual-elements.
+      // https://floating-ui.com/docs/virtual-elements
       getBoundingClientRect() {
         const { scrollY, scrollX } = globalThis;
         const { scrollTop, scrollLeft } = containerScrollPosition;
@@ -200,11 +207,12 @@ export function getElementsBySelector(selector: Selector) {
 }
 
 /**
- * Checks whether the given element has the given anchor name, based on the element's computed
- * style.
+ * Checks whether the given element has the given anchor name, based on the
+ * element's computed style.
  *
- * Note: because our `--anchor-name` custom property inherits, this function should only be called
- * for elements which are known to have an explicitly set value for `anchor-name`.
+ * Note: because our `--anchor-name` custom property inherits, this function
+ * should only be called for elements which are known to have an explicitly set
+ * value for `anchor-name`.
  */
 export function hasAnchorName(
   el: PseudoElement | HTMLElement,
@@ -223,8 +231,9 @@ export function hasAnchorName(
 /**
  * Checks whether the given element serves as a scope for the given anchor.
  *
- * Note: because our `--anchor-scope` custom property inherits, this function should only be called
- * for elements which are known to have an explicitly set value for `anchor-scope`.
+ * Note: because our `--anchor-scope` custom property inherits, this function
+ * should only be called for elements which are known to have an explicitly set
+ * value for `anchor-scope`.
  */
 export function hasAnchorScope(
   el: PseudoElement | HTMLElement,

src/parse.ts

@@ -22,7 +22,7 @@ interface AtRuleRaw extends csstree.Atrule {
 }
 
 // `key` is the `anchor-name` value
-// `value` is an array of all element selectors associated with that `anchor-name`
+// `value` is an array of all selectors associated with that `anchor-name`
 type AnchorSelectors = Record<string, Selector[]>;
 
 export type InsetProperty =

src/polyfill.ts

@@ -77,7 +77,8 @@ export const resolveLogicalSizeKeyword = (
 };
 
 // This should also check the writing-mode
-// See: https://github.com/oddbird/css-anchor-positioning/pull/22#discussion_r966348526
+// See:
+// https://github.com/oddbird/css-anchor-positioning/pull/22#discussion_r966348526
 // https://trello.com/c/KnqCnHx3
 export const getAxis = (position?: string) => {
   switch (position) {
@@ -344,8 +345,8 @@ async function applyPositionFallbacks(
       target,
       target,
       async () => {
-        // If this auto-update was triggered while the polyfill is already looping
-        // through the possible `@try` blocks, do not check again.
+        // If this auto-update was triggered while the polyfill is already
+        // looping through the possible `@try` blocks, do not check again.
         if (checking) {
           return;
         }
@@ -379,7 +380,8 @@ async function applyPositionFallbacks(
               padding: getMargins(target),
             },
           );
-          // If none of the sides overflow, use this `@try` block and stop loop...
+          // If none of the sides overflow, use this `@try` block and stop
+          // loop...
           if (Object.values(overflow).every((side) => side <= 0)) {
             checking = false;
             break;

src/validate.ts

@@ -77,7 +77,8 @@ async function getContainingBlock(element: HTMLElement) {
 }
 
 /**
- * Validates that el is a acceptable anchor element for an absolutely positioned element query el
+ * Validates that el is a acceptable anchor element for an absolutely positioned
+ * element query el
  * https://drafts.csswg.org/css-anchor-position-1/#acceptable-anchor-element
  */
 export async function isAcceptableAnchorElement(
@@ -150,7 +151,8 @@ export async function isAcceptableAnchorElement(
     }
   }
 
-  // el is in scope for query el, per the effects of anchor-scope on query el or its ancestors.
+  // el is in scope for query el, per the effects of anchor-scope on query el or
+  // its ancestors.
   if (
     anchorName &&
     scopeSelector &&
@@ -168,10 +170,11 @@ function getScope(
   anchorName: string,
   scopeSelector: string,
 ) {
-  // Unlike the real `anchor-scope`, our `--anchor-scope` custom property inherits.
-  // We first check that the element matches the scope selector, so we can be guaranteed that the
-  // computed value we read was set explicitly, not inherited. Then we verify that the specified
-  // anchor scope is actually the one applied by the CSS cascade.
+  // Unlike the real `anchor-scope`, our `--anchor-scope` custom property
+  // inherits. We first check that the element matches the scope selector, so we
+  // can be guaranteed that the computed value we read was set explicitly, not
+  // inherited. Then we verify that the specified anchor scope is actually the
+  // one applied by the CSS cascade.
   while (
     !(element.matches(scopeSelector) && hasAnchorScope(element, anchorName))
   ) {
@@ -206,12 +209,12 @@ export async function validatedForPositioning(
   }
 
   const anchorElements = anchorSelectors
-    // Any element that matches a selector that sets the specified `anchor-name` could be a
-    // potential match.
+    // Any element that matches a selector that sets the specified `anchor-name`
+    // could be a potential match.
     .flatMap(getElementsBySelector)
-    // Narrow down the potential match elements to just the ones whose computed `anchor-name`
-    // matches the specified one. This accounts for the `anchor-name` value that was actually
-    // applied by the CSS cascade.
+    // Narrow down the potential match elements to just the ones whose computed
+    // `anchor-name` matches the specified one. This accounts for the
+    // `anchor-name` value that was actually applied by the CSS cascade.
     .filter((el) => hasAnchorName(el, anchorName));
 
   // TODO: handle anchor-scope for pseudo-elements.

tests/helpers.ts

@@ -1,8 +1,9 @@
 import fs from 'fs';
 import path from 'path';
 import { fileURLToPath } from 'url';
+
 import { cascadeCSS } from '../src/cascade.js';
-import { StyleData } from '../src/utils.js';
+import { type StyleData } from '../src/utils.js';
 
 const __filename = fileURLToPath(import.meta.url);
 const __dirname = path.dirname(__filename);