Bug 211105 - Web Inspector: Timelines: Edit button has wrong outline
Summary: Web Inspector: Timelines: Edit button has wrong outline
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 211104
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-27 16:59 PDT by Nikita Vasilyev
Modified: 2020-06-01 15:19 PDT (History)
3 users (show)

See Also:


Attachments
[Image] Bug (54.87 KB, image/png)
2020-04-27 16:59 PDT, Nikita Vasilyev
no flags Details
Patch (1.30 KB, patch)
2020-04-29 02:56 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (2.49 KB, patch)
2020-05-05 12:52 PDT, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2020-06-01 14:36 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2020-04-27 16:59:31 PDT
Created attachment 397769 [details]
[Image] Bug

This button and other instances of `.navigation-bar .item.button.text-only` should use a different focus offset.
Comment 1 Radar WebKit Bug Importer 2020-04-27 16:59:43 PDT
<rdar://problem/62475815>
Comment 2 Nikita Vasilyev 2020-04-29 02:56:51 PDT
Created attachment 397950 [details]
Patch
Comment 3 Devin Rousso 2020-04-29 07:33:22 PDT
Comment on attachment 397950 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.css:68
> +.navigation-bar .item.button.text-only:focus {

We should be _very_ careful with this.  Right now, the only reason that `.navigation-bar .item.radio.button:focus` applies instead of this is because it was included in Main.html first.
Comment 4 Nikita Vasilyev 2020-05-05 12:51:07 PDT
(In reply to Devin Rousso from comment #3)
> Comment on attachment 397950 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397950&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.css:68
> > +.navigation-bar .item.button.text-only:focus {
> 
> We should be _very_ careful with this.  Right now, the only reason that
> `.navigation-bar .item.radio.button:focus` applies instead of this is
> because it was included in Main.html first.

Good catch, thanks!
Comment 5 Nikita Vasilyev 2020-05-05 12:52:02 PDT
Created attachment 398543 [details]
Patch
Comment 6 Devin Rousso 2020-05-20 10:58:35 PDT
Comment on attachment 398543 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:18
> +        Remove dead code. We only have text-only buttons now.

I don't think we should limit ourselves to only having text-only `WI.RadioButtonNavigationItem`.

If we really do want to do that, we should
 - add `console.assert(this.buttonStyle === WI.ButtonNavigationItem.Style.Text);` to the `constructor`
 - override `set buttonStyle` so that it fails if anything other than `WI.ButtonNavigationItem.Style.Text` is provided

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.css:69
> +    outline-offset: var(--focus-ring-outline-offset);

Perhaps we can use the CSS variable override approach like `WI.ScopeBar` does.

ButtonNavigationItem.css
```
    .navigation-bar .item.button.text-only:focus {
        outline-offset: var(--outline-offset-override, var(--outline-offset-default);

        --outline-offset-default: var(--focus-ring-outline-offset);
    }
```

RadioButtonNavigationItem.css
```
    .navigation-bar .item.radio.text-only:focus {
        --outline-offset-override: -1px;
    }
```

This way, selector specificity is irrelevant.
Comment 7 Nikita Vasilyev 2020-06-01 14:36:09 PDT
Created attachment 400760 [details]
Patch
Comment 8 Nikita Vasilyev 2020-06-01 14:39:12 PDT
Comment on attachment 398543 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:18
>> +        Remove dead code. We only have text-only buttons now.
> 
> I don't think we should limit ourselves to only having text-only `WI.RadioButtonNavigationItem`.
> 
> If we really do want to do that, we should
>  - add `console.assert(this.buttonStyle === WI.ButtonNavigationItem.Style.Text);` to the `constructor`
>  - override `set buttonStyle` so that it fails if anything other than `WI.ButtonNavigationItem.Style.Text` is provided

Done.

>> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.css:69
>> +    outline-offset: var(--focus-ring-outline-offset);
> 
> Perhaps we can use the CSS variable override approach like `WI.ScopeBar` does.
> 
> ButtonNavigationItem.css
> ```
>     .navigation-bar .item.button.text-only:focus {
>         outline-offset: var(--outline-offset-override, var(--outline-offset-default);
> 
>         --outline-offset-default: var(--focus-ring-outline-offset);
>     }
> ```
> 
> RadioButtonNavigationItem.css
> ```
>     .navigation-bar .item.radio.text-only:focus {
>         --outline-offset-override: -1px;
>     }
> ```
> 
> This way, selector specificity is irrelevant.

This is a very verbose way of handling this.  I landed this patch without it. However, I'd like to discuss more how we handle cases like in this bug next time I encounter them.
Comment 9 EWS 2020-06-01 15:19:55 PDT
Committed r262397: <https://trac.webkit.org/changeset/262397>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400760 [details].