Bug 181973 – Web Inspector: Styles Redesign: ensure that tabbing through the last section wraps back to the first

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Tabbing past the last section or shift-tabbing before the inline style should wrap the focus to the section at other end of the sidebar panel.

[Devin Rousso]

Created attachment 332001 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 332001 [details]
Patch

I think tabbing past the last section should focus on the filter field.
Shift-tabbing before the inline style should focus on the last force state checkbox. This one already works as expected.

I don't think "wrapping back" is particularly useful or predictable.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #2)
> Comment on attachment 332001 [details]
> Patch
> 
> I think tabbing past the last section should focus on the filter field.
> Shift-tabbing before the inline style should focus on the last force state
> checkbox. This one already works as expected.
> 
> I don't think "wrapping back" is particularly useful or predictable.
Wrapping is the default behavior in Chrome, as well as our legacy sidebar.
Firefox just moves the focus to the element before/after the sidebar.

I personally don't think that it makes much sense to focus the pseudo-state checkboxes, although I do like the idea of the filter bar.  I can go either way on this though.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #2)
> Comment on attachment 332001 [details]
> Patch
> 
> I think tabbing past the last section should focus on the filter field.

Agree. This is (keyboard) inaccessible otherwise.

> Shift-tabbing before the inline style should focus on the last force state
> checkbox. This one already works as expected.

When I tried the focus disappeared, and after a few more shift tabs moved into the main content view.

> I don't think "wrapping back" is particularly useful or predictable.

What if wrapping still occurred, but included the filter field and pseudo-state checkboxes? Tabbing from the filter field would focus the first checkbox. Shift-tabbing from the first checkbox focuses the filter field.

[Devin Rousso]

Created attachment 332944 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/37724804>

[Matt Baker]

Comment on attachment 332944 [details]
Patch

Shift-tabbing from the Style Attribute section focuses the last pseudo-class checkbox, but tabbing from the last checkbox skips the Style Attribute section and focuses the first selector.

[Devin Rousso]

Created attachment 334330 [details]
Patch

[Matt Baker]

Comment on attachment 334330 [details]
Patch

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

r- for now. Comments on the behaviors I observed when testing:

Styles panel tabbing works great, with one small issue:

- Checking a pseudo-class checkbox then hitting tab focuses the first rule after Style Attribute {}. I expected it to focus Style Attribute {}.

Computed panel tabbing feels pretty broken:

-Shift-tabbing from the first property name focuses the "visited" checkbox (good). Tabbing again doesn't move the focus back to the property name, instead the focus is lost. Hitting tab again will move the focus back to the property name.
- Tabbing from the last item value in either the Properties section or Variables sections adds a blank space at the end of the section.
- Checking the first pseudo-class checkbox then hitting tab should focus the next doesn't focus anything. Tabbing again moves the focus to the filter bar, 
- Shift-tabbing from the filter bar
- Tabbing from the filter bar focuses checkboxes in order but the focus is lost after that.
- The "Show All" checkbox isn't accessible by tabbing.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:585
> +                this._delegate.cssStyleDeclarationTextEditorStartEditingAdjacentRule(this, true);

Nit:

const backward = true;
this._delegate.cssStyleDeclarationTextEditorStartEditingAdjacentRule(this, backward);

[Nikita Vasilyev]

Comment on attachment 334330 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:264
> +    _handleForcedPseudoClassCheckboxKeydown(pseudoClass, event)
> +    {
> +        if (event.key !== "Tab")
> +            return;
> +
> +        let pseudoClasses = WI.CSSStyleManager.ForceablePseudoClasses;
> +        let index = pseudoClasses.indexOf(pseudoClass);
> +        if (event.shiftKey) {
> +            if (index > 0) {
> +                this._forcedPseudoClassCheckboxes[pseudoClasses[index - 1]].focus();
> +                event.preventDefault();
> +            } else {
> +                this._filterBar.inputField.focus();
> +                event.preventDefault();
> +            }
> +        } else {
> +            if (index < pseudoClasses.length - 1) {
> +                this._forcedPseudoClassCheckboxes[pseudoClasses[index + 1]].focus();
> +                event.preventDefault();

Tabbing between these checkboxes already works without any JS. We should only use JS to implement Tab navigation that can't be implemented with pure HTML.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #10)
> Comment on attachment 334330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334330&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:264
> > +    _handleForcedPseudoClassCheckboxKeydown(pseudoClass, event)
> > +    {
> > +        if (event.key !== "Tab")
> > +            return;
> > +
> > +        let pseudoClasses = WI.CSSStyleManager.ForceablePseudoClasses;
> > +        let index = pseudoClasses.indexOf(pseudoClass);
> > +        if (event.shiftKey) {
> > +            if (index > 0) {
> > +                this._forcedPseudoClassCheckboxes[pseudoClasses[index - 1]].focus();
> > +                event.preventDefault();
> > +            } else {
> > +                this._filterBar.inputField.focus();
> > +                event.preventDefault();
> > +            }
> > +        } else {
> > +            if (index < pseudoClasses.length - 1) {
> > +                this._forcedPseudoClassCheckboxes[pseudoClasses[index + 1]].focus();
> > +                event.preventDefault();
> 
> Tabbing between these checkboxes already works without any JS. We should
> only use JS to implement Tab navigation that can't be implemented with pure
> HTML.

I agree. Whenever tabbing issues come up I consider how much of this could be solved by better `tabindex` management. Since the Inspector frontend doesn't abstract the DOM, and we create/reorder elements in an an hoc fashion, these types of bugs will keep needing targeted fixes, that are inherently fragile.

[Devin Rousso]

Comment on attachment 334330 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:264
>>> +                event.preventDefault();
>> 
>> Tabbing between these checkboxes already works without any JS. We should only use JS to implement Tab navigation that can't be implemented with pure HTML.
> 
> I agree. Whenever tabbing issues come up I consider how much of this could be solved by better `tabindex` management. Since the Inspector frontend doesn't abstract the DOM, and we create/reorder elements in an an hoc fashion, these types of bugs will keep needing targeted fixes, that are inherently fragile.

On my machine, tabbing does not work between the checkboxes without this code.  Tabbing from any checkbox immediately moves to the first non-inline property, and shift-tabbing from any checkbox loses the focus entirely.

(In reply to Matt Baker from comment #9)
> - Checking a pseudo-class checkbox then hitting tab focuses the first rule
> after Style Attribute {}. I expected it to focus Style Attribute {}.
I can't seem to reproduce this issue.  It works fine for me.

> Computed panel tabbing feels pretty broken:
I agree, but quite frankly I'd rather just entirely rebuild the Computed panel in a followup.  Having it use CodeMirror seems very unnecessary when none of it is editable anyways.

> -Shift-tabbing from the first property name focuses the "visited" checkbox
> (good). Tabbing again doesn't move the focus back to the property name,
> instead the focus is lost. Hitting tab again will move the focus back to the
> property name.
It looks like the focus shifts to the <textarea> created by CodeMirror before it starts highlighting what's actually shown.  This would be fixed by rebuilding the Computed panel.

> - Tabbing from the last item value in either the Properties section or
> Variables sections adds a blank space at the end of the section.
Yeah, I think that's a "bug" in CSSStyleDeclarationTextEditor.prototype._handleTabKey where it doesn't check to see if it's editable before attempting to add a new line.

> - Checking the first pseudo-class checkbox then hitting tab should focus the
> next doesn't focus anything. Tabbing again moves the focus to the filter
> bar, 
This might be a more complex issue, since it seems like the focus is removed from the checkbox if it's toggled via the mouse.  When I try logging `document.activeElement` after clicking it returns the <body>.  We could manually `focus()` the checkbox when it's "click" event is fired, but that seems like overkill.  I'm not sure what to do about this.

> - Shift-tabbing from the filter bar
See above issue with <textarea>.

> - Tabbing from the filter bar focuses checkboxes in order but the focus is
> lost after that.
See above issue with <textarea>.

> - The "Show All" checkbox isn't accessible by tabbing.
Oh geez.  That's not gonna be easy.  Personally, I'd rather get that addressed in a followup so we don't keep delaying the majority of the fix.  :/

[Devin Rousso]

Created attachment 335760 [details]
Patch

[Nikita Vasilyev]

Created attachment 335794 [details]
[Animated GIF] Tabbing between checkboxes

(In reply to Devin Rousso from comment #12)
> Comment on attachment 334330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334330&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:264
> >>> +                event.preventDefault();
> >> 
> >> Tabbing between these checkboxes already works without any JS. We should only use JS to implement Tab navigation that can't be implemented with pure HTML.
> > 
> > I agree. Whenever tabbing issues come up I consider how much of this could be solved by better `tabindex` management. Since the Inspector frontend doesn't abstract the DOM, and we create/reorder elements in an an hoc fashion, these types of bugs will keep needing targeted fixes, that are inherently fragile.
> 
> On my machine, tabbing does not work between the checkboxes without this
> code.  Tabbing from any checkbox immediately moves to the first non-inline
> property, and shift-tabbing from any checkbox loses the focus entirely.

I commented out the code that handles tabbing between the checkboxes, and tabbing still worked. See the attached GIF.

[Devin Rousso]

Comment on attachment 334330 [details]
Patch

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

>>>>> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:264
>>>>> +                event.preventDefault();
>>>> 
>>>> Tabbing between these checkboxes already works without any JS. We should only use JS to implement Tab navigation that can't be implemented with pure HTML.
>>> 
>>> I agree. Whenever tabbing issues come up I consider how much of this could be solved by better `tabindex` management. Since the Inspector frontend doesn't abstract the DOM, and we create/reorder elements in an an hoc fashion, these types of bugs will keep needing targeted fixes, that are inherently fragile.
>> 
>> On my machine, tabbing does not work between the checkboxes without this code.  Tabbing from any checkbox immediately moves to the first non-inline property, and shift-tabbing from any checkbox loses the focus entirely.
>> 
>> (In reply to Matt Baker from comment #9)
> 
> I commented out the code that handles tabbing between the checkboxes, and tabbing still worked. See the attached GIF.

So it looks like this is related to a setting in system preferences.

System Preferences > Keyboard > Shortcuts > Full Keyboard Access = All Controls

I personally think that we should have tabbing support regardless of system functionality, as we already have it working for the CSS rules themselves.  Alternatively, if we decide to drop support (somehow) there, that would be fine too.

[Matt Baker]

Comment on attachment 335760 [details]
Patch

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

I think we should keep the checkbox tabbing behavior as-is. It's true it isn't needed when "All Controls" are made made tab-accessible via the system setting, and ideally we'd match the system. But not knowing the system setting in the frontend means we can't determine what to focus after tabbing forward from the filter field, or backward from the Style Attribute {} field.

Still r-, because checking the last pseudo-class checkbox and pressing tab skips the Style Attribute {} and focuses the next rule (body). I can reproduce this regardless of my system setting.

> Source/WebInspectorUI/ChangeLog:9
> +        simplicity.

This is true, but is just a detail of the patch and not the main focus. Mentioning the user-impact of the new tabbing behavior makes more sense as a summary.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:247
> +    spreadsheetCSSStyleDeclarationSectionStartEditingAdjacentRule(currentSection, delta)

It's weird that here we take a delta, and in cssStyleDeclarationTextEditorStartEditingAdjacentRule we take a bool indicating "backward". Personally I don't like the delta, unless we plan to pass a value other than 1 or -1 some day. Is there a reason this can't just take a bool?

[Matt Baker]

(In reply to Matt Baker from comment #16)
> Comment on attachment 335760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335760&action=review
> 
> I think we should keep the checkbox tabbing behavior as-is. It's true it
> isn't needed when "All Controls" are made made tab-accessible via the system
> setting, and ideally we'd match the system. But not knowing the system
> setting in the frontend means we can't determine what to focus after tabbing
> forward from the filter field, or backward from the Style Attribute {} field.

In case it wasn't clear, I'm trying to say I agree with Devin's current approach to tabbing, which includes the checkboxes even though it's redundant when system tabbing is set to "All Controls".

[Devin Rousso]

Comment on attachment 335760 [details]
Patch

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

> Still r-, because checking the last pseudo-class checkbox and pressing tab
> skips the Style Attribute {} and focuses the next rule (body). I can
> reproduce this regardless of my system setting.
What website are you testing this on?  I haven't been able to reproduce this :(

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:247
>> +    spreadsheetCSSStyleDeclarationSectionStartEditingAdjacentRule(currentSection, delta)
> 
> It's weird that here we take a delta, and in cssStyleDeclarationTextEditorStartEditingAdjacentRule we take a bool indicating "backward". Personally I don't like the delta, unless we plan to pass a value other than 1 or -1 some day. Is there a reason this can't just take a bool?

I personally find that `delta` is cleaner, and potentially more future-proof.  To me, `backward` is a lot more limiting.

[Matt Baker]

(In reply to Devin Rousso from comment #18)
> Comment on attachment 335760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335760&action=review
> 
> > Still r-, because checking the last pseudo-class checkbox and pressing tab
> > skips the Style Attribute {} and focuses the next rule (body). I can
> > reproduce this regardless of my system setting.
> What website are you testing this on?  I haven't been able to reproduce this
> :(

The page shouldn't matter. I reproduced it on about:blank, and will attach a video with keyboard casting to visualize tab presses.

The behavior is the same regardless of the "System Preferences > Keyboard > Shortcuts > Full Keyboard Access" setting value.

> 
> >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:247
> >> +    spreadsheetCSSStyleDeclarationSectionStartEditingAdjacentRule(currentSection, delta)
> > 
> > It's weird that here we take a delta, and in cssStyleDeclarationTextEditorStartEditingAdjacentRule we take a bool indicating "backward". Personally I don't like the delta, unless we plan to pass a value other than 1 or -1 some day. Is there a reason this can't just take a bool?
> 
> I personally find that `delta` is cleaner, and potentially more
> future-proof.  To me, `backward` is a lot more limiting.

cssStyleDeclarationTextEditorStartEditingAdjacentRule takes a bool, and this takes a delta. Is there a reason for this inconsistency?

spreadsheetCSSStyleDeclarationSectionStartEditingAdjacentRule asserts that delta !== 0, so it is already the case that we don't expect a zero delta. Passing a zero delta, after asserting, is the equivalent of calling `currentSection.startEditingRuleSelector()`. It works, so even the assertion seems strange.

I think this API is more flexible than we actually need. Let's make it simpler!

[Matt Baker]

Created attachment 339232 [details]
[Video] Stylers Sidebar Tabbing

[Matt Baker]

(In reply to Matt Baker from comment #20)
> Created attachment 339232 [details]
> [Video] Stylers Sidebar Tabbing

We should rename it the Stylers Sidebar

[Devin Rousso]

(In reply to Matt Baker from comment #21)
> (In reply to Matt Baker from comment #20)
> > Created attachment 339232 [details]
> > [Video] Stylers Sidebar Tabbing
> 
> We should rename it the Stylers Sidebar

 /$$     /$$ /$$$$$$$$  /$$$$$$ 
|  $$   /$$/| $$_____/ /$$__  $$
 \  $$ /$$/ | $$      | $$  \__/
  \  $$$$/  | $$$$$   |  $$$$$$ 
   \  $$/   | $$__/    \____  $$
    | $$    | $$       /$$  \ $$
    | $$    | $$$$$$$$|  $$$$$$/
    |__/    |________/ \______/

[Devin Rousso]

Created attachment 339369 [details]
Patch

Looks like the reason why the checkbox tabbing wasn't working was that the checkbox was not being focused after it fired "change".

[Matt Baker]

Comment on attachment 339369 [details]
Patch

r=me, nice work.

[WebKit Commit Bot]

Comment on attachment 339369 [details]
Patch

Clearing flags on attachment: 339369

Committed r231372: <https://trac.webkit.org/changeset/231372>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.