Bug 211105 – Web Inspector: Timelines: Edit button has wrong outline

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 397769 [details]
[Image] Bug

This button and other instances of `.navigation-bar .item.button.text-only` should use a different focus offset.

[Radar WebKit Bug Importer]

<rdar://problem/62475815>

[Nikita Vasilyev]

Created attachment 397950 [details]
Patch

[Devin Rousso]

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.

[Nikita Vasilyev]

(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!

[Nikita Vasilyev]

Created attachment 398543 [details]
Patch

[Devin Rousso]

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.

[Nikita Vasilyev]

Created attachment 400760 [details]
Patch

[Nikita Vasilyev]

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.

[EWS]

Committed r262397: <https://trac.webkit.org/changeset/262397>

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