Web Inspector: REGRESSION: Elements: the forced pseudo-class indicator isn't visible when hovering

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. right-click on any node in the Elements Tab
3. toggle any of the submenu items of "Forced Pseudo-Classes"
 => a white circle appears to the left of the toggled node
4. select a different node
5. hover over the node from step 2 (and step 3)
 => the white circle disappears

[Devin Rousso]

Created attachment 372774 [details]
Patch

[Matt Baker]

I think the real issue is that the hovered .selection-area div has an opacity of 0.3, making the ::before element hard to read regardless of the background color:

DOMTreeOutline.css:69:

.tree-outline.dom:not(.non-selectable) li.hovered:not(.selected) .selection-area {
    opacity: 0.3;
}

If we could break out the pseudo class indicator from the .selection-area that would be awesome.

[Matt Baker]

Created attachment 372842 [details]
[Image] Mockup - hovered pseudo class indicator

If we break this out of the .selection-area, the indicator can continue to use --glyph-color-active and be legible. Granted, the contrast is lower in dark mode, but that is true of other UI as well.

[Matt Baker]

Comment on attachment 372774 [details]
Patch

r- for now, as I don't think this gives us the appearance we want in either light or dark mode.

[Matt Baker]

And are we sure this is a regression? Did it regress during the CSS variable changes made for dark mode support? Or possibly the changes to use the system accent color?

If so, please add the revision responsible.

[Devin Rousso]

Created attachment 372872 [details]
Patch

[Matt Baker]

Comment on attachment 372872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372872&action=review

Very nice! Colors look good in light and dark mode. Just a couple comments.

> Source/WebInspectorUI/ChangeLog:32
> +        indicator, such as in the Console.

Should we be doing this? We didn't show the indicator in the console before.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1856
> +        if (!listItemElement)

Can we remove this local variable? Below `this.listItemElement` is used directly.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:41
> +    --item-pseudo-class-indicator-start: -2px;

This should still be 2px. The indicator is cut off otherwise.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:154
> +    background-color: white;

background-color: var(--selected-foreground-color);

[Matt Baker]

Comment on attachment 372872 [details]
Patch

r-, due to the indicator being cut off when the gutter isn't showing. Otherwise this change looks great.

[Devin Rousso]

Comment on attachment 372872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372872&action=review

>> Source/WebInspectorUI/ChangeLog:32
>> +        indicator, such as in the Console.
> 
> Should we be doing this? We didn't show the indicator in the console before.

We actually did, just not for the logged node itself.  If a child had a forced pseudo-class, it was shown (and updated) in the Console.

[Devin Rousso]

Created attachment 372957 [details]
Patch

[Matt Baker]

Comment on attachment 372957 [details]
Patch

r=me, nice work!

[WebKit Commit Bot]

Comment on attachment 372957 [details]
Patch

Clearing flags on attachment: 372957

Committed r246855: <https://trac.webkit.org/changeset/246855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52217790>