Bug 185930 – Web Inspector: Styles: overridden CSS property should have go-to button to jump to effective property

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

It's often hard to find why the property is strikethrough, and what overrode it.

To solve this, display an arrow up icon after the overridden CSS property. Clicking this icon would scroll up to either:
1. The active property (highest priority property).
2. Next property with the higher priority, but not necessary the active property. If this is hard to implement, [1] is fine.

After scrolling to the property, focus on the property and highlight it (the same way we highlight the properties when jumping from the Computed panel).

This has been requested several times. AFAIK, neither Chrome DevTools nor Firefox DevTools have this.

[Radar WebKit Bug Importer]

<rdar://problem/40506252>

[Nikita Vasilyev]

Created attachment 363569 [details]
Patch

[Nikita Vasilyev]

Created attachment 363570 [details]
[Video] With patch applied

[Matt Baker]

(In reply to Nikita Vasilyev from comment #3)
> Created attachment 363570 [details]
> [Video] With patch applied

Nice! To reduce UI clutter, should we only reveal the arrow on mouseover?

[EWS Watchlist]

Comment on attachment 363569 [details]
Patch

Attachment 363569 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11371958

New failing tests:
fast/scrolling/ios/hit-testing-iframe-004.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html
fast/scrolling/ios/hit-testing-iframe-001.html

[EWS Watchlist]

Created attachment 363598 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

(In reply to Matt Baker from comment #4)
> (In reply to Nikita Vasilyev from comment #3)
> > Created attachment 363570 [details]
> > [Video] With patch applied
> 
> Nice! To reduce UI clutter, should we only reveal the arrow on mouseover?

Yeah, I do like that!

[Nikita Vasilyev]

Created attachment 363610 [details]
Patch

[Matt Baker]

Comment on attachment 363610 [details]
Patch

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

r- because of a couple general comments:

I'm worried that CSSProperty _overridden and _overriddenBy could get out of sync. What about dropping _overridden and replacing the public property with:

get overridden()
{
    return !!this._overriddingProperty;
}

You'd need to eliminate the WI.CSSProperty constructor argument `overridden`, or replaced it with `overriddingProperty`. This could be an issue in DOMNodeStyles.prototype._parseStylePropertyPayload, which constructs a WI.CSSProperty and sets overridden equal to true if payload.status.inactive. If this is too annoying to refactor, you can disregard this but be sure to fix the issue in CSSProperty.prototype.update.

My other comment is style.

1) The spacing between the property value, goto arrow, and overriding property value feels too cramped.
2. The overriding property value hint should be shown with the arrow. It feels odd having it appear only when the goto arrow is hovered.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:34
> +        this._overriddenBy = null;

What about _overridingProperty?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:266
> +            this._overriddenBy = null;

This should also be nulled out in update() when !overridden.

[Matt Baker]

I wonder if we want a way to reveal/flash a property, like we do with source code locations. I was a little surprised that the property became selected after clicking the goto arrow. It's fine for now, but we might want to consider this for a follow-up.

[Nikita Vasilyev]

Created attachment 363692 [details]
Patch

[Nikita Vasilyev]

(In reply to Matt Baker from comment #10)
> I wonder if we want a way to reveal/flash a property, like we do with source
> code locations. I was a little surprised that the property became selected
> after clicking the goto arrow. It's fine for now, but we might want to
> consider this for a follow-up.

Yes, it would be a nice follow-up!

We used to do that. It had several bugs. For instance, lines being stuck highlighted when the animation was interrupted (by switching to a different tab or panel, for instance). Regardless, I think we should bring it back and fix the bugs.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #11)
> Created attachment 363692 [details]
> Patch

This doesn't address the hover or horizontal spacing issues.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #9)
> Comment on attachment 363610 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363610&action=review
> 
> r- because of a couple general comments:
> 
> I'm worried that CSSProperty _overridden and _overriddenBy could get out of
> sync. What about dropping _overridden and replacing the public property with:
> 
> get overridden()
> {
>     return !!this._overriddingProperty;
> }
> 
> You'd need to eliminate the WI.CSSProperty constructor argument
> `overridden`, or replaced it with `overriddingProperty`. This could be an
> issue in DOMNodeStyles.prototype._parseStylePropertyPayload, which
> constructs a WI.CSSProperty and sets overridden equal to true if
> payload.status.inactive. If this is too annoying to refactor, you can
> disregard this but be sure to fix the issue in CSSProperty.prototype.update.

The current patch adds an experimental setting and tries to make as little as possible changes
to the existing behavior.

I agree with what you're saying! I think this refactoring should be done when the feature is turned on by default.

> 
> My other comment is style.
> 
> 1) The spacing between the property value, goto arrow, and overriding
> property value feels too cramped.

More than 40% of our users use Web Inspector docked to right with the styles sidebar being very narrow. What are you suggesting, exactly?

> 2. The overriding property value hint should be shown with the arrow. It
> feels odd having it appear only when the goto arrow is hovered.

I tried it out and it didn't seem too obnoxious, so I think I agree with you.

[Nikita Vasilyev]

Created attachment 363712 [details]
Patch

Addressed the hover. Left a comment about the horizontal spacing above.

[Devin Rousso]

Comment on attachment 363712 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:300
> +        if (!WI.settings.experimentalEnableStylesJumpToEffective.value)
> +            return;

When this value changes, should we reset `_overridingProperty` to `null` (or force a reload of Web Inspector)?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:921
>                              property.overridden = true;
> +                            property.overridingProperty = effectiveProperty;

Rather than have both an `overridden` and an `overridingProperty`, why don't we say that `get overridden` returns true if `overridingProperty` is non-null?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:926
>                          property.overridden = true;
> +                        property.overridingProperty = effectiveProperty;

Ditto (>920).

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:932
>                          effectiveProperty.overridden = true;
> +                        effectiveProperty.overridingProperty = property;

Ditto (>920).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:169
> +.spreadsheet-style-declaration-editor .property .select-effective-property {

Does this sector match anything?  I would think that we only create `.select-effective-property` when we're `.overridden`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:173
> +    margin-left: 0;

NIT: I try to keep this in between "sizing" (e.g. `width` and `height`) and "content" (e.g. `line-height`, `vertical-align`, etc.) properties.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:184
> +    content: attr(data-value);

NIT: I normally put this with other "content"-esque properties, like above `white-space`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:187
> +    display: inline-block;

NIT: I normally put this as the first property, since it can have the most impact/effect of other properties (e.g. `display: none;` trumps all other styles).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:224
> +                this._jumpToEffectiveButton = arrowButton;

Why not just use `this._jumpToEffectiveButton` instead of making another variable `arrowButton`?
```
    this._jumpToEffectiveButton = WI.createGoToArrowButton();
    this._jumpToEffectiveButton.classList.add("select-effective-property");
    ...
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:279
> +                this._jumpToEffectiveButton.dataset.value = this._property.overridingProperty ? this._property.overridingProperty.rawValue : "";

Shouldn't we always have an `overridingProperty`?  Maybe just assert instead?

[Nikita Vasilyev]

Comment on attachment 363712 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:921
>> +                            property.overridingProperty = effectiveProperty;
> 
> Rather than have both an `overridden` and an `overridingProperty`, why don't we say that `get overridden` returns true if `overridingProperty` is non-null?

Yes, I agree that would make things easier.

However, currently, the overridden logic is split between the backend and the front-end. See _parseStylePropertyPayload and _markOverriddenProperties. If we move all the logic to the front-end, then I could make `get overridden` returns true if `overridingProperty` is non-null. If I do this, I'd want to write tests.

I outlined some other concerns related to the existing `overridden` logic here: https://bugs.webkit.org/show_bug.cgi?id=195389#c2

[Nikita Vasilyev]

Comment on attachment 363712 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:300
>> +            return;
> 
> When this value changes, should we reset `_overridingProperty` to `null` (or force a reload of Web Inspector)?

Good point.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:184
>> +    content: attr(data-value);
> 
> NIT: I normally put this with other "content"-esque properties, like above `white-space`.

I still think `content` is the most important property.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:224
>> +                this._jumpToEffectiveButton = arrowButton;
> 
> Why not just use `this._jumpToEffectiveButton` instead of making another variable `arrowButton`?
> ```
>     this._jumpToEffectiveButton = WI.createGoToArrowButton();
>     this._jumpToEffectiveButton.classList.add("select-effective-property");
>     ...
> ```

Ok, since this is what we usually do.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:279
>> +                this._jumpToEffectiveButton.dataset.value = this._property.overridingProperty ? this._property.overridingProperty.rawValue : "";
> 
> Shouldn't we always have an `overridingProperty`?  Maybe just assert instead?

Yes, it should. Good point.

[Nikita Vasilyev]

Created attachment 363854 [details]
Patch

[Matt Baker]

(In reply to Nikita Vasilyev from comment #19)
> Created attachment 363854 [details]
> Patch

A few issues we observed offline, which need to be resolved:

1) The tip text with the overriding property value is hard to see in dark mode. This is compounded when the property has a warning, and has the warning color background color.
2) Goto arrow doesn't appear for overridden User Agent properties.

[Nikita Vasilyev]

Created attachment 363938 [details]
Patch

[Matt Baker]

Comment on attachment 363938 [details]
Patch

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

r=me, with some suggestions.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:915
> +localizedStrings["Show Jump to Effective Property Buttons"] = "Show Jump to Effective Property Buttons";

This should be singular: "Show Jump to Effective Property Button".

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:299
> +        if (!WI.settings.experimentalEnableStylesJumpToEffective.value)

Should we be performing this check? I think we should always track the effective property, but only show the UI when the setting is enabled.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:921
> +                            property.overridingProperty = effectiveProperty;

You could eliminate some of the syncing of these two properties by doing this:

class CSSProperty
{
    set overridingProperty(property = null)
    {
        if (this._overridingProperty === property)
            return;
        this._overridingProperty = property;
        this._overridden = !!this._overridingProperty
    }
}

Unless it's possible that `overridden === true && overridingProperty === null`. Just a thought. If it doesn't simplify things, please disregard.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:226
> +        /* .select-effective-property has inverted colors. Invert the text color again to make it readable. */

Suggestion: ".select-effective-property has inverted colors. Invert the pseudo-element again to restore the original text color."

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:42
> +        this._jumpToEffectiveButton = null;

this._jumpToEffectivePropertyButton

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:264
>          if (this._property.overridden) {

If it is not possible that `overridden && !overridingProperty`, I'd test this._overridingProperty instead. Then you can eliminate the assert and test below.

[Nikita Vasilyev]

Created attachment 363943 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363943 [details]
Patch for landing

Clearing flags on attachment: 363943

Committed r242622: <https://trac.webkit.org/changeset/242622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.