WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
222384
REGRESSION (
r266288
): Web Inspector: ::marker shows on every element now
https://bugs.webkit.org/show_bug.cgi?id=222384
Summary
REGRESSION (r266288): Web Inspector: ::marker shows on every element now
Nikita Vasilyev
Reported
2021-02-24 14:48:22 PST
Every element in inspector has a ::marker section in the style sidebar. Steps To Reproduce: 1. Inspect any web page 2. Select an Element in the Elements tab Results: ::marker section taking space in the sidebar whether or not the element has a marker. <
rdar://problem/74398791
>
Attachments
Patch
(1.80 KB, patch)
2021-02-24 15:09 PST
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Patch v2.0 - Backend Fix, No Test Yet
(6.78 KB, patch)
2021-03-02 15:00 PST
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v2.1 - Review Notes, Added Tests
(11.35 KB, patch)
2021-03-02 16:34 PST
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo
(11.35 KB, patch)
2021-03-02 16:35 PST
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v2.2 - Revised Approach
(8.81 KB, patch)
2021-03-02 19:24 PST
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v2.3 - Re-baselined related test
(10.62 KB, patch)
2021-03-03 08:59 PST
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Patch v2.3.1 - Re-baselined related test, Added reviewer to changelogs
(10.62 KB, patch)
2021-03-03 09:03 PST
,
Patrick Angle
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Nikita Vasilyev
Comment 1
2021-02-24 15:09:22 PST
Created
attachment 421461
[details]
Patch
https://nv.github.io/webkit-inspector-bugs/222384_marker-pseudo-element/
Inspect <li>. Both Author (`li::marker`) and UserAgent styles are shown. Inspect <div>. No `::marker` rules are shown.
Patrick Angle
Comment 2
2021-03-02 15:00:24 PST
Created
attachment 421999
[details]
Patch v2.0 - Backend Fix, No Test Yet
Devin Rousso
Comment 3
2021-03-02 15:29:39 PST
Comment on
attachment 421999
[details]
Patch v2.0 - Backend Fix, No Test Yet View in context:
https://bugs.webkit.org/attachment.cgi?id=421999&action=review
> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:510 > + if (!matchedRulesProtocolObjects->length())
good idea :)
> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1106 > + if (!inspectorStyleSheet || (inspectorStyleSheet->origin() == Protocol::CSS::StyleSheetOrigin::UserAgent && rule->selectorText() == "::marker"_s && element.computedStyle()->display() != DisplayType::ListItem))
NIT: I'd recommend splitting this out into two different `if` so that it's easier to read (less parenthesis/grouping) NIT: Is there really a benefit to showing even non-UserAgent `::marker` if not `display: list-item`?
Patrick Angle
Comment 4
2021-03-02 15:37:50 PST
Comment on
attachment 421999
[details]
Patch v2.0 - Backend Fix, No Test Yet View in context:
https://bugs.webkit.org/attachment.cgi?id=421999&action=review
>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1106 >> + if (!inspectorStyleSheet || (inspectorStyleSheet->origin() == Protocol::CSS::StyleSheetOrigin::UserAgent && rule->selectorText() == "::marker"_s && element.computedStyle()->display() != DisplayType::ListItem)) > > NIT: I'd recommend splitting this out into two different `if` so that it's easier to read (less parenthesis/grouping) > > NIT: Is there really a benefit to showing even non-UserAgent `::marker` if not `display: list-item`?
To the second NIT: I was ready to say yes, but after typing my reason for saying yes I realize that it wasn't a valid reason, so here is my reason for saying no now: If the author defines a blanket `::marker` selector, they would expect it to apply only where markers could apply. If they define a silly selector like `table::marker`, it would still be shown after removing the origin check anyways because its selector no exactly matches `::marker`.
Patrick Angle
Comment 5
2021-03-02 16:34:11 PST
Created
attachment 422023
[details]
Patch v2.1 - Review Notes, Added Tests
Patrick Angle
Comment 6
2021-03-02 16:35:37 PST
Created
attachment 422024
[details]
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo
Devin Rousso
Comment 7
2021-03-02 16:49:12 PST
Comment on
attachment 422024
[details]
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo View in context:
https://bugs.webkit.org/attachment.cgi?id=422024&action=review
> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1110 > + if (rule->selectorText() == "::marker"_s && element.computedStyle()->display() != DisplayType::ListItem)
NIT: I'd swap this around so that we do the `display() `check first (an `enum class` is cheaper to compare than a `String`). Would this also match the `table::marker` example that you gave? It seems odd to me that we'd hide `::marker` but not `table::marker`. Out of curiosity, do we know what any other tools do in these cases?
> LayoutTests/inspector/css/getMatchedStyleForNodeMarkerPseudoId.html:31 > + async function changeElementDisplayValue(id, value)
unused?
Patrick Angle
Comment 8
2021-03-02 16:55:28 PST
Comment on
attachment 422024
[details]
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo View in context:
https://bugs.webkit.org/attachment.cgi?id=422024&action=review
>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1110 >> + if (rule->selectorText() == "::marker"_s && element.computedStyle()->display() != DisplayType::ListItem) > > NIT: I'd swap this around so that we do the `display() `check first (an `enum class` is cheaper to compare than a `String`). > > Would this also match the `table::marker` example that you gave? It seems odd to me that we'd hide `::marker` but not `table::marker`. Out of curiosity, do we know what any other tools do in these cases?
It would not match the `table::marker` (see the test where a `#nonListItemWithMarkerSpecified::marker` rule makes it to the frontend). Arguably at the point where such a rule exists, the author has made a mistake. I'm not convinced hiding their mistake from them is worthwhile. As far as other tool implementation, Chrome does not filter any of the `*::marker` selectors, and Firefox filters all of them on elements that don't support it, even if the author explicitly defined it.
Devin Rousso
Comment 9
2021-03-02 17:16:20 PST
Comment on
attachment 422024
[details]
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo View in context:
https://bugs.webkit.org/attachment.cgi?id=422024&action=review
>>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1110 >>> + if (rule->selectorText() == "::marker"_s && element.computedStyle()->display() != DisplayType::ListItem) >> >> NIT: I'd swap this around so that we do the `display() `check first (an `enum class` is cheaper to compare than a `String`). >> >> Would this also match the `table::marker` example that you gave? It seems odd to me that we'd hide `::marker` but not `table::marker`. Out of curiosity, do we know what any other tools do in these cases? > > It would not match the `table::marker` (see the test where a `#nonListItemWithMarkerSpecified::marker` rule makes it to the frontend). Arguably at the point where such a rule exists, the author has made a mistake. I'm not convinced hiding their mistake from them is worthwhile. As far as other tool implementation, Chrome does not filter any of the `*::marker` selectors, and Firefox filters all of them on elements that don't support it, even if the author explicitly defined it.
I'm not sure I'd call it a "mistake". I can envision a scenario where JS toggles between `display: *;` and `display: line-item;` (although I admit that it's likely a rare occurrence) and the developer has `.foo::marker` on the expectation that it'd only have an effect when `display: list-item;` (kinda like how `::selection` is used very generally on the expectation that it'll only affect selection).
Patrick Angle
Comment 10
2021-03-02 17:26:10 PST
Comment on
attachment 422024
[details]
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo View in context:
https://bugs.webkit.org/attachment.cgi?id=422024&action=review
>>>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1110 >>>> + if (rule->selectorText() == "::marker"_s && element.computedStyle()->display() != DisplayType::ListItem) >>> >>> NIT: I'd swap this around so that we do the `display() `check first (an `enum class` is cheaper to compare than a `String`). >>> >>> Would this also match the `table::marker` example that you gave? It seems odd to me that we'd hide `::marker` but not `table::marker`. Out of curiosity, do we know what any other tools do in these cases? >> >> It would not match the `table::marker` (see the test where a `#nonListItemWithMarkerSpecified::marker` rule makes it to the frontend). Arguably at the point where such a rule exists, the author has made a mistake. I'm not convinced hiding their mistake from them is worthwhile. As far as other tool implementation, Chrome does not filter any of the `*::marker` selectors, and Firefox filters all of them on elements that don't support it, even if the author explicitly defined it. > > I'm not sure I'd call it a "mistake". I can envision a scenario where JS toggles between `display: *;` and `display: line-item;` (although I admit that it's likely a rare occurrence) and the developer has `.foo::marker` on the expectation that it'd only have an effect when `display: list-item;` (kinda like how `::selection` is used very generally on the expectation that it'll only affect selection).
You make a good point, which leads to a desire for consistency, which leads me to conclude that we should just hide it when it doesn't apply, no matter how specific or general. This might take a hot second to implement, since I think `selectorText()` could be a comma-delimited list of selectors, but I've now been convinced we should suppress `::marker` for ALL non-list-item elements.
Patrick Angle
Comment 11
2021-03-02 19:24:09 PST
Created
attachment 422032
[details]
Patch v2.2 - Revised Approach
Devin Rousso
Comment 12
2021-03-02 20:15:05 PST
Comment on
attachment 422032
[details]
Patch v2.2 - Revised Approach r=me, nice fix!
Patrick Angle
Comment 13
2021-03-02 20:22:46 PST
Comment on
attachment 422032
[details]
Patch v2.2 - Revised Approach Need to re-baseline the CSS.getMatchedStyleForNode test, as it checks a dump of styles, which is now missing the `::marker` rule we are filtering.
Patrick Angle
Comment 14
2021-03-03 08:59:27 PST
Created
attachment 422094
[details]
Patch v2.3 - Re-baselined related test
Patrick Angle
Comment 15
2021-03-03 09:03:59 PST
Created
attachment 422096
[details]
Patch v2.3.1 - Re-baselined related test, Added reviewer to changelogs
EWS
Comment 16
2021-03-03 09:59:36 PST
Committed
r273821
: <
https://commits.webkit.org/r273821
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 422096
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug