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>
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.
Created attachment 421999 [details] Patch v2.0 - Backend Fix, No Test Yet
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`?
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`.
Created attachment 422023 [details] Patch v2.1 - Review Notes, Added Tests
Created attachment 422024 [details] Patch v2.1.1 - Review Notes, Added Tests, Fixed Typo
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?
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.
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).
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.
Created attachment 422032 [details] Patch v2.2 - Revised Approach
Comment on attachment 422032 [details] Patch v2.2 - Revised Approach r=me, nice fix!
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.
Created attachment 422094 [details] Patch v2.3 - Re-baselined related test
Created attachment 422096 [details] Patch v2.3.1 - Re-baselined related test, Added reviewer to changelogs
Committed r273821: <https://commits.webkit.org/r273821> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422096 [details].