Page MenuHomePhabricator

Bug 2035143 - [devtools] Move handling of var() in OutputParser to onCloseParenthesis. r=#devtools.
ClosedPublic

Authored by nchevobbe on Apr 27 2026, 6:00 PM.
Referenced Files
Unknown Object (File)
Aug 11 2026, 2:45 PM
Unknown Object (File)
Aug 9 2026, 4:19 PM
Unknown Object (File)
Aug 9 2026, 4:19 PM
Unknown Object (File)
Aug 9 2026, 10:12 AM
Unknown Object (File)
Jul 21 2026, 10:11 AM
Unknown Object (File)
Jul 20 2026, 11:41 PM
Unknown Object (File)
Jun 23 2026, 1:18 PM
Unknown Object (File)
Jun 14 2026, 12:56 PM

Details

Summary

This is moving the last special cases for Function tokens to onCloseParenthesis,
which means we can also remove #parseVariable, #parseMatchingParens and the
stopAtComma parameter for #doParse.

This has the nice side effects of adding color swatches for some declarations
where we didn't use to be able to compute those (results of color-mix(),
contrast-color(), relative color syntax from color, var() with fallback),
which are reflected in the changes in the tests.

With this final change, we also needed to change how we're handling font family
names so they can fit nicely in this new design. So instead of keeping track
of the different font family parts, and then appending them to parts, we
now keep a single index when we detect a font family name, and when we find
a delimiter or a closing parenthesis, we wrap the meaningful part of the
font family in a dedicated element.

As we needed something similar to what we have for stack entry for fonts, in order
to map a "part" to multiple tokens, we update the CLOSED_STACK_ENTRY "token",
renaming it to better convey what it does, and renaming the stackEntry into
a more generic data property.

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
devtools/client/shared/output-parser.js
440–446

note: Was only used from #parseVariable

508–533

note: I didn't use a dedicated function as we're not doing much, but I wouldn't mind moving it

753–760

note: stopAtCloseParen was only used via #parseVariable, and we were only having done here to handle it, so this can go away and we can always add the closing parenthesis

1631

note: This might be Bug 1904013, but it only mentions "initial", so I'm not sure we have similar behavior with other CSS-wide keywords.
I'll check

1693–1699

note: I'll file a bug for this

1844

note: This was the stopAtComma parameter that we remove in this patch

2045–2046

note: This was used by the part of the code that was handling colors as a result of a variable. This isn't needed as #appendColor is only called for "simple" colors now (named color or hexa)
Functions that return a color would call #createColorContainerElement, which has the children parameter that allows us to wrap them in the color container

2345

note: We return the index so it can easily be used for the fontFamilyIndex (instead of querying the stack entry parts length)

devtools/client/shared/test/browser_outputparser.js
82–103

note: Extracted this so we can build strings for nested color spans

250–253

note: We now get a color swatch for the resulting color of color-mix() 🎉

337–338

note: We also get a color swatch for the from color in relative syntax 🎉
This kind of fixes Bug 1898254 (but need some extra work to make sure we don't show contrast information in the tooltip for those)

472–474

note: Also getting a color swatch for the resulting color of contrast-color() 🎉

Fixes Bug 1996837

1511–1520

note: We now get a color swatch for the resulting color of var() even when the fallback value is used 🎉
Fixes Bug 2035144

2063

note: Keeping the original whitespaces

2076

note: Keeping the original whitespaces

2091

note: Keeping the original whitespaces

2123

note: Extending the case a bit so we can check that we handle font family names with var() in the middle of it, quoted names in the var fallback, and !important (which was already handled in the code but wasn't tested here)

Code analysis found 1 defect in diff 1259473:

  • 1 defect found by eslint (Mozlint)
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.

You can run this analysis locally with:

  • ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1259473.

nchevobbe edited the summary of this revision. (Show Details)

tweaks

rebase and address review comments

jdescottes added a project: testing-approved.
jdescottes added a subscriber: jdescottes.

Thanks Nicolas, mostly nits about comments/naming/clarity, but I think the change looks otherwise fine.

devtools/client/shared/output-parser.js
79–82

I would add a comment here to explain that:

  • this symbol is used as a stack entry type
  • this is used for entries made of multiple parts (or compounds) which shouldn't be processed as individual entries

Otherwise it's hard to know if compounds refers to a specific term here or if it's the generic meaning.

Maybe consider another name eg COLLAPSED_TOKENS_ENTRY_TYPE

170–171

I imagine we only stop at EOF now?

297

suggestion: Could we define sawComma: false in #createStackEntry?

454

nitpick:

542
546

nitpick: missing end of the sentence here?

549

nitpick: total 10 occurrences of this typo in the patch.

561–562

nitpick: should that still be called "closedStackEntryToken"?

1026–1027

Should we consider updating that comment, with the new symbol name I'm not sure it will be easy to understand for future reviewers.

Eg

// a collapsed function call (e.g. var()) counts as a single argument

Same comment for the 2 other duplicated comments.

1844

(I like seeing unnamed boolean flags removed :) )

2207

Also what makes it "fake"? If the purpose of this token type is to aggregate various parts that shouldn't be handled independently, why is this one fake compared to other "compound" tokens?

2208

nitpick: "go other those parts" missing some words? Or "go over those parts" maybe?

This revision is now accepted and ready to land.Apr 30 2026, 6:48 AM
nchevobbe added inline comments.
devtools/client/shared/output-parser.js
79–82

In another review comments you used "aggregated" which I think might be even more fitted? It could be AGGREGATED_TOKEN_TYPE

170–171

yes!

297

yeah, I'm pretty sure it was on my list but I forgot

546

yeah, will reword that

549

ouch, really struggling with that one eh

561–562

probably not indeed with the symbol being renamed

1026–1027

sure, your suggestion looks good, so I'll use that 👍

2207

my wording is poor here, that means that we have something that doesn't come from InspectorCSSParser. I'll reword this

2208

yeah, that was for "go over"

nchevobbe edited the summary of this revision. (Show Details)
nchevobbe marked 9 inline comments as done.

rebase and address review comments