Bug 195299 - Web Inspector: REGRESSION(r242118): WI.ScopeBar missing background
Summary: Web Inspector: REGRESSION(r242118): WI.ScopeBar missing background
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-04 16:00 PST by Devin Rousso
Modified: 2019-03-07 11:27 PST (History)
8 users (show)

See Also:


Attachments
[Image] Screenshot of Issue (44.87 KB, image/png)
2019-03-04 16:00 PST, Devin Rousso
no flags Details
Patch (13.58 KB, patch)
2019-03-04 17:05 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.52 KB, patch)
2019-03-04 20:28 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.04 MB, application/zip)
2019-03-04 22:57 PST, Build Bot
no flags Details
Patch (20.69 KB, patch)
2019-03-07 10:44 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-03-04 16:00:26 PST
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
Comment 1 Devin Rousso 2019-03-04 16:11:01 PST
It also appears to have broken the Audit tab :(
Comment 2 Devin Rousso 2019-03-04 17:05:56 PST
Created attachment 363571 [details]
Patch
Comment 3 Joseph Pecoraro 2019-03-04 17:14:23 PST
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?
Comment 4 Matt Baker 2019-03-04 17:19:50 PST
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?
Comment 5 Devin Rousso 2019-03-04 19:10:17 PST
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.
Comment 6 Devin Rousso 2019-03-04 20:28:25 PST
Created attachment 363591 [details]
Patch

Resetting back to r? since I did some additional fixes with the console
Comment 7 Build Bot 2019-03-04 22:57:05 PST Comment hidden (obsolete)
Comment 8 Build Bot 2019-03-04 22:57:07 PST Comment hidden (obsolete)
Comment 9 Nikita Vasilyev 2019-03-05 00:04:35 PST
Comment on attachment 363608 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

Unrelated test failures.
Comment 10 Joseph Pecoraro 2019-03-07 10:30:13 PST
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.
Comment 11 Devin Rousso 2019-03-07 10:43:23 PST
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.
Comment 12 Devin Rousso 2019-03-07 10:44:28 PST
Created attachment 363895 [details]
Patch
Comment 13 WebKit Commit Bot 2019-03-07 11:24:28 PST
Comment on attachment 363895 [details]
Patch

Clearing flags on attachment: 363895

Committed r242604: <https://trac.webkit.org/changeset/242604>
Comment 14 WebKit Commit Bot 2019-03-07 11:24:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-03-07 11:27:59 PST
<rdar://problem/48683234>