Bug 195299 – Web Inspector: REGRESSION(r242118): WI.ScopeBar missing background

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Created attachment 363563 [details]
[Image] Screenshot of Issue

# STEPS TO REPRODUCE:
1. inspect any page with a <canvas>
2. go to the Canvas tab
3. take a recording of any <canvas> on the page
4. wait for the recording to finish processing
 => "Recording 1" scope bar in the navigation sidebar has no background

[Devin Rousso]

It also appears to have broken the Audit tab :(

[Devin Rousso]

Created attachment 363571 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363571 [details]
Patch

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

rs=me Sounds good to me.

> Source/WebInspectorUI/ChangeLog:9
> +        use CSS variables instead. Divides the variables into three categories:

I only see 2 groups, default and override. I don't see "actual".

> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.css:98
> +    vertical-align: -4px;

How is this calculated? Just adjusting until it looks nice?

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:-129
> -.scope-bar > li:matches(.selected, :active) {
> -    transition-duration: 75ms;
> -}

Heh, so we aren't transitioning anything?

[Matt Baker]

Comment on attachment 363571 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:60
> +    border: 1px solid var(--scope-bar-border-color);

Borders are only used in the Audit tab, correct? What about just overriding the width, height, and border properties in AuditTestGroupContentView.css, since it isn't generally used?

[Devin Rousso]

Comment on attachment 363571 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:9
>> +        use CSS variables instead. Divides the variables into three categories:
> 
> I only see 2 groups, default and override. I don't see "actual".

The actual is the variable that doesn't have an `-override` or `-default` at the end of it.  The idea is that you'd want to set the `-override` variable (e.g. `--scope-bar-text-color-override`) and get the non-`-override` variable (e.g. `--scope-bar-text-color`).

>> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.css:98
>> +    vertical-align: -4px;
> 
> How is this calculated? Just adjusting until it looks nice?

Yup.  I'd like to avoid setting "significant" properties (e.g. `display`) as they can lead to very different behavior.  This only affects the <img> (whereas `display` affects the `WI.ScopeBarItem`'s element).

>> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:60
>> +    border: 1px solid var(--scope-bar-border-color);
> 
> Borders are only used in the Audit tab, correct? What about just overriding the width, height, and border properties in AuditTestGroupContentView.css, since it isn't generally used?

I'd rather not, as that means that if we did add a border in the future, it would break the audit tab.  That's exactly what happened to cause this bug.  We should strive to be as flexible as possible with our general UI views so that they can handle multiple things at once (or at the very least have them be written in such a way so as to make sure that changes don't break varying uses).

>> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:-129
>> -}
> 
> Heh, so we aren't transitioning anything?

Not that I could tell.  The only one I did find was the console's filters, but that uses an animation.

[Devin Rousso]

Created attachment 363591 [details]
Patch

Resetting back to r? since I did some additional fixes with the console

[EWS Watchlist]

Comment on attachment 363591 [details]
Patch

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

New failing tests:
fast/images/imagemap-polygon-focus-ring.html
fast/scrolling/ios/hit-testing-iframe-003.html

[EWS Watchlist]

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

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

[Nikita Vasilyev]

Comment on attachment 363608 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

Unrelated test failures.

[Joseph Pecoraro]

Comment on attachment 363591 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:96
> +            indicatorElement.classList.add("indicator");

Nit:`elem.className = "indicator"` is more common and faster for the simple case where we just created the element and are setting classes.

> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:178
>          // Clicking when selected will cause the menu it appear instead.

Fix the typo in this comment while you're in the area. "menu it appear" => "menu to appear"

> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:181
> +        if (this._element.classList.contains("selected")) {
> +            let newEvent = new event.constructor(event.type, event);
> +            newEvent.__original = event;

This should really be its own separate change.

[Devin Rousso]

Comment on attachment 363591 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/MultipleScopeBarItem.js:181
>> +            newEvent.__original = event;
> 
> This should really be its own separate change.

I would tend to agree, but in this case, since I was already fixing so much of the CSS for `WI.ScopeBar`, I felt that it would make sense.

[Devin Rousso]

Created attachment 363895 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363895 [details]
Patch

Clearing flags on attachment: 363895

Committed r242604: <https://trac.webkit.org/changeset/242604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48683234>