REGRESSION (r266288): Web Inspector: ::marker shows on every element now

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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>

[Nikita Vasilyev]

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]

Created attachment 421999 [details]
Patch v2.0 - Backend Fix, No Test Yet

[Devin Rousso]

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 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]

Created attachment 422023 [details]
Patch v2.1 - Review Notes, Added Tests

[Patrick Angle]

Created attachment 422024 [details]
Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo

[Devin Rousso]

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 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 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 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]

Created attachment 422032 [details]
Patch v2.2 - Revised Approach

[Devin Rousso]

Comment on attachment 422032 [details]
Patch v2.2 - Revised Approach

r=me, nice fix!

[Patrick Angle]

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]

Created attachment 422094 [details]
Patch v2.3 - Re-baselined related test

[Patrick Angle]

Created attachment 422096 [details]
Patch v2.3.1 - Re-baselined related test, Added reviewer to changelogs

[EWS]

Committed r273821: <https://commits.webkit.org/r273821>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422096 [details].