Created attachment 397769 [details] [Image] Bug This button and other instances of `.navigation-bar .item.button.text-only` should use a different focus offset.
<rdar://problem/62475815>
Created attachment 397950 [details] Patch
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.
(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!
Created attachment 398543 [details] Patch
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.
Created attachment 400760 [details] Patch
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.
Committed r262397: <https://trac.webkit.org/changeset/262397> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400760 [details].