Bug 208163

<rdar://problem/59745448>

Created attachment 391596 [details]
Patch

Created attachment 391598 [details]
[Video] With patch applied

Comment on attachment 391596 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:13
> +        you were â the console prompt. This behavior matches MacOS.

nit: macOS

> Source/WebInspectorUI/ChangeLog:17
> +        triggers "click" event.

Interesting!

> Source/WebInspectorUI/ChangeLog:25
> +        Reset the default styles.

Why?

> Source/WebInspectorUI/ChangeLog:30
> +        Before this patch, focused button looked the same as activated buttons. For example, the focused (non-active) bullseye icon looked exactly the same as unfocused active bullseye icon, which was misleading.

Please line-break.

> Source/WebInspectorUI/UserInterface/Views/ActivateButtonNavigationItem.js:74
> +        this.tooltip = flag ? this._activatedToolTip : this._defaultToolTip;

In the interest of preventing programming errors, can we coerce `flag` to a bool at the top of the function? i.e., !!flag. This wasn't as important before but now the code sets ariaPressed to the value of flag directly.

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:159
> +        // Clicking on a button should NOT focus on it.

I think you should leave it at this one line comment. The full justification in the changelog would be easily accessible.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:297
> +                // Generally, clicking on a button should not move focus.

"If clicking on a tab, stop the event from being handled by the button element. Instead, pass focus to the selected tab. Otherwise, let the button become activated normally."

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:406
> +        this.selectedNavigationItem?.element.focus();

Wat (okay, I looked it up)

Comment on attachment 391596 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:25
>> +        Reset the default styles.
> 
> Why?

Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.

Comment on attachment 391596 [details]
Patch

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

>>> Source/WebInspectorUI/ChangeLog:25
>>> +        Reset the default styles.
>> 
>> Why?
> 
> Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.

I don't think we should do this.  We've never done this in the past, and given the finicky nature of the CSS cascade I fear that this could easily cause other problems.

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:31
> +        role = role || "button";
> +        super(identifier, role);

Style: missing newline

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:163
> +        // keep you focused where you were â the console prompt.

Please avoid adding non UTF-8 characters to the source (especially JavaScript), as it can balloon the size of the backing store.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:49
> -    outline: none;
> +    outline-offset: -4px;

Why?

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-292
> -        // Only keep the tabIndex if already focused from keyboard navigation. This matches Xcode.

What about this?

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:295
> +            let isFocused = document.activeElement && this.element.contains(document.activeElement);

You should be able to drop the `document.activeElement` check, as passing `null` to `Node.prototype.contains` should always return `false`.  As such, you could also inline this.

> Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.css:35
> +    outline-offset: -3px;

Why?

Comment on attachment 391596 [details]
Patch

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

>>>> Source/WebInspectorUI/ChangeLog:25
>>>> +        Reset the default styles.
>>> 
>>> Why?
>> 
>> Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.
> 
> I don't think we should do this.  We've never done this in the past, and given the finicky nature of the CSS cascade I fear that this could easily cause other problems.

What are you suggesting? Specifying `margin: 0; padding: 0; border: 0;`?
Or not use <button> at all and add a keydown event listener for Space and Enter?

Created attachment 391722 [details]
Patch

Comment on attachment 391596 [details]
Patch

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

>>>>> Source/WebInspectorUI/ChangeLog:25
>>>>> +        Reset the default styles.
>>>> 
>>>> Why?
>>> 
>>> Reset the default styles so buttons that are just clickable icons (e.g. Force Dark Appearance) don't have the default border, padding, margin, and other style properties that vary between operation systems.
>> 
>> I don't think we should do this.  We've never done this in the past, and given the finicky nature of the CSS cascade I fear that this could easily cause other problems.
> 
> What are you suggesting? Specifying `margin: 0; padding: 0; border: 0;`?
> Or not use <button> at all and add a keydown event listener for Space and Enter?

I excluded all changes related to converting <div role=button> to <button> from my patch since it isn't strictly required for this bug.

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-292
>> -        // Only keep the tabIndex if already focused from keyboard navigation. This matches Xcode.
> 
> What about this?

The NavigationBar element itself doesn't have tabIndex. This hasn't been working as described for quite some time.

Comment on attachment 391722 [details]
Patch

Clearing flags on attachment: 391722

Committed r257411: <https://trac.webkit.org/changeset/257411>

All reviewed patches have been landed.  Closing bug.