Pro 8513 where issue by BoDonkey · Pull Request #11 · apostrophecms/postcss-viewport-to-container-toggle

BoDonkey · 2025-11-05T19:27:28Z

Summary

Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"
This PR fixes two issues. One, with the Apollo Astro starter kits, switching on breakpoint preview would cause all the fonts to use the base system font. This was due to how the :where() pseudoclass was wrapping elements and reducing their specificity to 0. Two, a user noted that responsive Tailwind classes were not working in breakpoint preview. This was due to how Tailwind 4.x nests media queries. Closes PRO-8513

What are the specific steps to test this change?

For example:

Run the website and log in as an admin

Open a piece manager modal and select several pieces

Click the "Archive" button on the top left of the manager and confirm that it should proceed

Check that all pieces have been archived properly

Install Tailwind using npx astro add tailwind in a starter-kit-astro-apollo project.
Add import "../styles/global.css" to the frontend/src/[...slug].astro file.
To the frontend/src/templates/HomePage.astro add the following above the AposArea:

  <div class="p-4 sm:p-8 md:p-12 lg:p-16 m-2 sm:m-4 md:m-6 lg:m-8 bg-red-100 sm:bg-blue-100 md:bg-green-100 lg:bg-purple-100 text-base sm:text-lg md:text-3xl lg:text-6xl">
  Test content - watch me change!
</div>

Symlink this branch into the frontend project.
Observe that the styles have the same changes whether resizing the browser window or selecting different breakpoint sizes.
Observe that the base fonts, like the navigation, do not change.
Run the tests in this branch and see that they all pass.

What kind of change does this PR introduce?

(Check at least one)

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

linear · 2025-11-05T19:27:31Z

PRO-8513 When using Breakpoint Preview with Astro in Apollo, the fonts break in Preview mode.

ValJed

See biggest comment, also I pushed a test scenario to show you what it should look like.

ValJed · 2025-11-18T08:40:47Z

+      // Check that not selectors were added
+      const notSelectorMatches = result.css.match(/:where\(body:not\(\[data-breakpoint-preview-mode\]\)\)/g);
+      assert.ok(notSelectorMatches && notSelectorMatches.length >= 2,
+        'Should have not selectors for both media queries');


I think it's more robust / readable to test the entire css output. Also there is a run method that takes the input and output and verify, you should use it like in other tests.

ValJed · 2025-11-18T09:13:48Z

-                  [ containerBodySelector ]
-                )
+            // For nested media queries
+            // For nested media queries


duplicated comment, + I think you don't need any, the variable name isNested is clear enough.

ValJed · 2025-11-18T10:05:03Z

+                  // Convert viewport units if needed
+                  let value = containerDecl.value;
+                  if (Object.keys(unitConverter.units)
+                    .some(unit => value.includes(unit))) {


A variable could be more readable here.

I think I made the change you wanted.

ValJed · 2025-11-18T10:06:35Z

+                  });
+
+                  // Convert viewport units if needed
+                  let value = containerDecl.value;


Why defining value outside of the scope of the if statement if you use it only inside it. Should be defined inside the if.

ValJed · 2025-11-18T10:20:05Z

+                  .addTargetToSelectors(
+                    originalSelector,
+                    conditionalNotSelector
+                  ),


This won't work, unfortunately this where mechanism, which is more the mechanism allowing to run media queries only when not in mobile preview mode (we check the body attribute) using conditionalNotSelector. So this mechanism doesn't work with nested css, I think I definitely missed that possibility..
Since it checks by default some body attribute it cannot be nested in other selectors or it will just not work..
I allowed myself to write a test case to show you ideally how it should look like, I intentionally used a deeply (2 levels) nested selector. The check on the body attribute should be done at the first level, so I think we should somehow copy the full container (all levels), add the check on the body selector to the first level and copy the rest as it is, example:

.foo { .bar { .inside { @media screen and (max-width: 500px) { margin: 2rem; } } }`;

would become

:where(body:not([data-breakpoint-preview-mode])) .foo, :where(body:not([data-breakpoint-preview-mode])).foo { .bar { .inside { @media screen and (max-width: 500px) { margin: 2rem; } } }`;

instead of

.foo { .bar { :where(body:not([data-breakpoint-preview-mode])).inside, :where(body:not([data-breakpoint-preview-mode])) inside { @media screen and (max-width: 500px) { margin: 2rem; } } }`;

Because this last case won't work. Of course normal rules should stay as they are in the original css without the where target. and of course container query should still be added..

ValJed

Looks doing what is expected globally 👍🏼
Few concerns, seeing a duplicated media query node in the tests.
Main function starts to be huge my be splitted in the future.
I don't get all the logic for the nested part. Given the complexity I think we should cover all cases we have in mind in the tests, can be improved later too.

ValJed · 2025-11-24T08:44:05Z

-                    value = unitConverter.convertUnitsInExpression(value);
+                  const convertibleUnits = Object.keys(unitConverter.units);
+
+                  if (convertibleUnits.some(unit => containerDecl.value.includes(unit))) {


I meant extracting all the logic in a nicely names variable for readability like:

let value = containerDecl.value; const containsValuesToConvert = Object.keys(value) .some(unit => value.includes(unit)); if (containsValuesToConvert) { ... }

But I'm good with this version, looks readable enough.

Okay, thanks.

ValJed · 2025-11-24T08:51:47Z

+              let rootRule = atRule.parent;
+              while (rootRule && rootRule.parent && rootRule.parent.type === 'rule') {
+                rootRule = rootRule.parent;
+              }


ValJed · 2025-11-24T08:58:38Z

+
+              // Find the root nesting level
+              let rootParent = parentRule;
+              let nestingDepth = 0;


[Optional] Detail but could be a boolean since you only check if it's equal to 0.
Something like:

let oneLevelNesting = true;

Set to false in the while loop.

ValJed · 2025-11-24T09:36:06Z

+    }
+    @media (width > 800px) {
+      top: 5rem;
+    }


I don't think this media query node should be duplicated?

Well crap. I missed that doing a cut-and-paste from the input. The test passes because of a side effect of cssnano. Looking at my nesting logic, I'm cloning at the wrong point. Test fixed and will fix the nesting logic.

ValJed · 2025-11-24T09:38:17Z

    "test": "npm run eslint && npm run mocha",
    "mocha": "mocha",
-    "eslint": "eslint --ext .js,.vue ."
+    "eslint": "eslint --fix --ext .js,.vue ."


I don't think we want this command to actually fix code, since it runs in the CI env it might fix code in a container and not inform about linting error. You can configure your IDE to fix linting errors on save.

I tried several times to set up VSCode to fix linting errors, but it does this odd thing where it gets rid of spaces inside []. I put that --fix there as a temporary work-around and forgot to remove it. Removed now.

Maybe does not read properly your eslint config..

ValJed · 2025-11-24T09:42:16Z

+                    clonedNode.walkAtRules('media', (mediaRule) => {
+                      mediaRule[processed] = true;
+                    });
+                    conditionalRule.append(clonedNode);


I have doubts about this block, we mark rules as processed but shouldn't we go through each node individually to handle everything?
I don't get all this part of the code nicely so prefer to ask.

No problem! Let me walk through what's happening here for the multi-level nesting case:
Context: We have deeply nested CSS like:

.foo { .bar { .inside { @media (max-width: 500px) { margin: 2rem; } } } }

Goal: Create a conditional wrapper at the root level (.foo) that contains the media query, so it only applies when the breakpoint preview is OFF.
What this code does:

We clone the entire nested structure from .foo down
During cloning, we remove any media queries that aren't the one currently being processed (to avoid duplicates)
We mark the current media query as [processed] = true so when the plugin encounters it again during traversal, it skips it
We append this cloned structure to the conditional wrapper

We mark as processed because after we clone and append the media query to the wrapper, PostCSS will walk through that new wrapper structure. Without the [processed] flag, the plugin would try to process those media queries again, creating infinite loops or duplicate container queries.
The walkAtRules('media', ...) ensures that not just the top-level media query but any nested ones are also marked as processed.
Does that clarify the logic? Happy to explain any specific part in more detail!

The updates above this code prevent duplicate media queries by only cloning the current media query being processed, not all media queries in the nested structure.

I understand, and thanks!
So the [processed] flag is the key to preventing infinite recursion - when you clone the nested structure and append it to the conditional wrapper, PostCSS will naturally walk through that new cloned tree during its traversal. Without marking those cloned media queries as processed, the plugin would encounter them again and try to process them a second time, which would create duplicates or even infinite loops.
And by using walkAtRules('media', ...) on the cloned structure, you're ensuring that not just the immediate media query but any deeply nested ones within the clone are also flagged, so none of them get processed again when PostCSS continues its walk.
The combination of selective cloning (only the current media query) plus the processed flag is what keeps everything clean. Makes total sense - thanks for the detailed explanation!

ValJed · 2025-11-24T09:44:04Z

+                    break;
+                  }
+                  prevNode = prevNode.prev();
+                }


What is for the prevNode, its the root one that gets the :where(body:not?
From what I understand if this wrapper is already here we don't add it again, shouldn't be checked at the rootSelector level directly?

The loop searches for an existing conditional wrapper created by a previously processed media query. Since wrappers are inserted with parentRule.before(conditionalRule) (or rootParent.before()), they become previous siblings. We need to check all previous siblings, not just the immediate one, because there could be multiple wrappers or other rules between them.

ValJed · 2025-11-24T09:51:12Z

+                  rootParent.before(conditionalRule);
+                  debugUtils.log('Created new conditional wrapper at root level', atRule);
+                } else {
+                  // Wrapper exists, add media query to matching nested location


I suppose this is the part where we duplicate media queries maybe.

The duplication issue was in the cloning logic above this line (inside rootParent.each), where all media queries were being cloned. The fix uses isSameMediaQuery to remove media queries that aren't the current one. The rootParent.before(conditionalRule) line just inserts the wrapper - it doesn't cause duplication. The duplication bug has been fixed and all tests are passing.

ValJed · 2025-11-24T10:15:03Z

+                }
+
+                // Remove from original
+                atRule.remove();


Remove what from original, the atRule node?

Yeah, that is correct. Otherwise, we would have the original and the clone, which could cause specificity conflicts.

ValJed

LGTM!!

ValJed · 2025-11-24T15:33:41Z

+                      } else {
+                        mediaRule[processed] = true;
+                      }
+                    });


Shouldn't we check if the media has already been processed.

ValJed · 2025-11-24T15:40:10Z

+                    clonedNode.walkAtRules('media', (mediaRule) => {
+                      mediaRule[processed] = true;
+                    });
+                    conditionalRule.append(clonedNode);


I understand, and thanks!
So the [processed] flag is the key to preventing infinite recursion - when you clone the nested structure and append it to the conditional wrapper, PostCSS will naturally walk through that new cloned tree during its traversal. Without marking those cloned media queries as processed, the plugin would encounter them again and try to process them a second time, which would create duplicates or even infinite loops.
And by using walkAtRules('media', ...) on the cloned structure, you're ensuring that not just the immediate media query but any deeply nested ones within the clone are also flagged, so none of them get processed again when PostCSS continues its walk.
The combination of selective cloning (only the current media query) plus the processed flag is what keeps everything clean. Makes total sense - thanks for the detailed explanation!

BoDonkey added 3 commits November 5, 2025 13:58

Corrects :where() wrapping to preserve original specificity

aa8dac3

Refactors AtRule to account for rule nesting

a96475d

Update CHANGELOG

a72fc32

BoDonkey requested a review from ValJed November 5, 2025 19:27

adds test scenario

30aa0cb

ValJed suggested changes Nov 18, 2025

View reviewed changes

BoDonkey added 4 commits November 20, 2025 05:27

Partial response

c0e26e5

Further refinement

1d9e0c4

Final nesting fix

636ae2a

Change destructuring

227d6cd

BoDonkey requested a review from ValJed November 20, 2025 15:40

ValJed suggested changes Nov 24, 2025

View reviewed changes

Response to second comments

daa75fe

BoDonkey requested a review from ValJed November 24, 2025 14:48

ValJed approved these changes Nov 24, 2025

View reviewed changes

BoDonkey merged commit 5c0972c into main Nov 24, 2025
6 checks passed

BoDonkey deleted the pro-8513-where-issue branch November 24, 2025 15:45

Conversation

BoDonkey commented Nov 5, 2025

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

Uh oh!

linear Bot commented Nov 5, 2025

Uh oh!

ValJed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValJed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValJed Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValJed Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValJed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ValJed left a comment •

edited

Loading

ValJed Nov 24, 2025 •

edited

Loading

ValJed Nov 24, 2025 •

edited

Loading