Bug 195190

Summary: Web Inspector: system accent color follow-ups
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, nvasilyev, webkit-bug-importer
Priority: P3 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
[Image] New accent color uses
none
Patch
none
[Image] Inspect Element button
none
[Image] Selected timeline segment with patch applied none

Description Matt Baker 2019-02-28 15:46:25 PST
A few follow-ups to https://webkit.org/b/193507.

- Use accent color for selected timeline record bars
- Use accent color for selected console item "border"
- Increase contrast of activated toolbar button item (Inspect Element button)
- Pressed ScopeBarItem accent color should be same brightness as pressed RadioButtonNavigationItem
Comment 1 Radar WebKit Bug Importer 2019-02-28 15:50:56 PST
<rdar://problem/48492280>
Comment 2 Matt Baker 2019-02-28 15:52:03 PST
Created attachment 363270 [details]
Patch
Comment 3 Matt Baker 2019-02-28 15:56:11 PST
Created attachment 363271 [details]
[Image] New accent color uses
Comment 4 Matt Baker 2019-02-28 16:03:01 PST
Comment on attachment 363270 [details]
Patch

Reviewable, but investigating a ButtonToolbarItem issue.
Comment 5 Matt Baker 2019-02-28 18:18:03 PST
Created attachment 363284 [details]
Patch
Comment 6 Matt Baker 2019-02-28 18:19:29 PST
Created attachment 363285 [details]
[Image] Inspect Element button

Comparison of light/dark, new/old.
Comment 7 Matt Baker 2019-02-28 18:20:21 PST
(In reply to Matt Baker from comment #6)
> Created attachment 363285 [details]
> [Image] Inspect Element button
> 
> Comparison of light/dark, new/old.

The CSS shown to the left refers to the "Patch" column only.
Comment 8 Devin Rousso 2019-03-05 22:17:27 PST
Comment on attachment 363284 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ButtonToolbarItem.css:73
> +        filter: brightness(1.35);

Nice!

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:45
> +    background-color: var(--selected-text-background-color) !important;

This is almost too light now.  It makes it harder to notice for small record bars.  Can you darken it a bit?
Comment 9 Nikita Vasilyev 2019-03-06 12:53:44 PST
Comment on attachment 363284 [details]
Patch

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

Looks all good to me!

>> Source/WebInspectorUI/UserInterface/Views/ButtonToolbarItem.css:73
>> +        filter: brightness(1.35);
> 
> Nice!

I honestly didn't notice that the color was different — it just looked right!

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:45
>> +    background-color: var(--selected-text-background-color) !important;
> 
> This is almost too light now.  It makes it harder to notice for small record bars.  Can you darken it a bit?

Too light on what configuration?

It doesn't seem too light to me. I tried both Dark and Light modes with various accent colors.
Comment 10 Nikita Vasilyev 2019-03-06 13:01:10 PST
Created attachment 363776 [details]
[Image] Selected timeline segment with patch applied

Looks good to me.
Comment 11 Devin Rousso 2019-03-06 13:25:50 PST
(In reply to Nikita Vasilyev from comment #10)
> Created attachment 363776 [details]
> [Image] Selected timeline segment with patch applied
> 
> Looks good to me.
I personally think that that doesn't match the "pattern" of the other records.  The difference between the border and the content of the selected record is greater than that of the non-selected records.

Basically, I think we should make the light-blue slightly darker (or make the border slightly lighter) so that the difference is less pronounced.
Comment 12 Nikita Vasilyev 2019-03-06 14:35:49 PST
(In reply to Devin Rousso from comment #11)
> (In reply to Nikita Vasilyev from comment #10)
> > Created attachment 363776 [details]
> > [Image] Selected timeline segment with patch applied
> > 
> > Looks good to me.
> I personally think that that doesn't match the "pattern" of the other
> records.  The difference between the border and the content of the selected
> record is greater than that of the non-selected records.
> 
> Basically, I think we should make the light-blue slightly darker (or make
> the border slightly lighter) so that the difference is less pronounced.

I see what you mean now.

If anything, the difference should be more pronounced. Try selecting red bars with red accent colors!
Comment 13 Matt Baker 2019-03-06 15:45:47 PST
Comment on attachment 363284 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:45
>>> +    background-color: var(--selected-text-background-color) !important;
>> 
>> This is almost too light now.  It makes it harder to notice for small record bars.  Can you darken it a bit?
> 
> Too light on what configuration?
> 
> It doesn't seem too light to me. I tried both Dark and Light modes with various accent colors.

I see what you mean, but I think the current approach is fine for now - it's simple and uses unmodified system colors.
Comment 14 WebKit Commit Bot 2019-03-06 16:13:20 PST
Comment on attachment 363284 [details]
Patch

Clearing flags on attachment: 363284

Committed r242578: <https://trac.webkit.org/changeset/242578>
Comment 15 WebKit Commit Bot 2019-03-06 16:13:22 PST
All reviewed patches have been landed.  Closing bug.