Closed Bug 1335055 Opened 6 years ago Closed 6 years ago

Improve accessibility of devtools/client/shared/components/tree.js

Categories

(DevTools :: Shared Components, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: access)

Attachments

(1 file)

Need to improve semantics and keyboard a11y for the tree component.
Attached patch 1335055 patchSplinter Review
Attachment #8831813 - Flags: review?(odvarko)
Component: Developer Tools → Developer Tools: Shared Components
Jan, would it be possible to find a different reviewer in case you are busy with other stuff, this really blocks me on things bug 1151468 which is a dev-rel P1 bug. Thanks.
Flags: needinfo?(odvarko)
Comment on attachment 8831813 [details] [diff] [review]
1335055 patch

Review of attachment 8831813 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

One question though before R+

Honza

::: devtools/client/shared/components/tree.js
@@ +634,5 @@
> +            return;
> +          }
> +
> +          let { explicitOriginalTarget } = nativeEvent;
> +          // Only set default focus to the first tree node iff the focus came

nit: typo 'iff the focus' -> 'if the focus'

@@ +644,5 @@
> +          }
> +        },
> +        onClick: () => {
> +          // Focus should always remain on the tree container itself.
> +          this.refs.tree.focus()

missing semicolon

@@ -768,5 @@
>                              this.props.expanded),
> -
> -      // XXX: OSX won't focus/blur regular elements even if you set tabindex
> -      // unless there is an input/button child.
> -      dom.button(this._buttonAttrs)

This isn't necessary anymore?
(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> Comment on attachment 8831813 [details] [diff] [review]
> 1335055 patch
> 
> Review of attachment 8831813 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me!
> 
> One question though before R+
> 
> Honza
> 
> ::: devtools/client/shared/components/tree.js
> @@ +634,5 @@
> > +            return;
> > +          }
> > +
> > +          let { explicitOriginalTarget } = nativeEvent;
> > +          // Only set default focus to the first tree node iff the focus came
> 
> nit: typo 'iff the focus' -> 'if the focus'
> 
> @@ +644,5 @@
> > +          }
> > +        },
> > +        onClick: () => {
> > +          // Focus should always remain on the tree container itself.
> > +          this.refs.tree.focus()
> 
> missing semicolon
> 
> @@ -768,5 @@
> >                              this.props.expanded),
> > -
> > -      // XXX: OSX won't focus/blur regular elements even if you set tabindex
> > -      // unless there is an input/button child.
> > -      dom.button(this._buttonAttrs)
> 
> This isn't necessary anymore?

Yeah, so it looked like keyboard navigation for rows was done as a workaround by adding offscreen buttons for each tree node. Navigation then would be done via tabbing to from nodes. So my solution does 2 things:

* Only [keyboard]focus on the tree container when the tree is reached with the keyboard. This significantly improves navigation in terms of not having to tab through the whole tree, however big it might be, to go the the next element (after/before the tree)

* Navigation is done via arrows (which was already supported) only. Since it is a complex widget it can handle keyboard differently from just tabbing. It is also consistent now with the other a11y friendly tree ui's such as the markup inspector (the rest should follow that pattern IMO). Screen readers track selected nodes via aria-activedescendant should it all works nicely (no actual keyboard focus needed in this case).
Comment on attachment 8831813 [details] [diff] [review]
1335055 patch

Review of attachment 8831813 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation!

R+

Honza
Attachment #8831813 - Flags: review?(odvarko) → review+
Flags: needinfo?(odvarko)
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1d8962e07f9
improving accessibility of a tree component (keyboard and semantics). r=Honza
https://hg.mozilla.org/mozilla-central/rev/b1d8962e07f9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I have an open patch to remove the onFocusedNodeUnmount behavior introduced here.

> In the Bug 1335055, accessibility features were added for re-focusing on
> a TreeNode when it was unmounted. Unfortunately this is incompatible
> with how the Tree provides a virtualized view, where it only has DOM
> nodes on the page that will be visibile when things are scrolled. So
> when a view is scrolled down, the TreeNode components get unmounted.
> The focused node is still technically focused, but the actual component
> is no longer mounted and present on the page. This causes the focus to
> incorrectly jump around as the user scrolls down the page.

:yzen I see your out for a bit, but if this behavior is required for accessibility we should probably file a follow-up bug.
Flags: needinfo?(yzenevich)
The patch is in Bug 1350304.
So I think the main issue is that the focus needs to land somewhere meaningful if currently focused node gets removed. The quick dirty solution would be moving it to the first element in the currently visible/rendered tree (ideally it should be dependent on whether we scroll up/down and focus on last/first respectively). It is really not ideal to have active element eliminated or moved onto document. I would strongly suggest to have a solution for accessibility instead of just removing this code. I am also implying here that accessibility is everyone's responsibility, just like performance is.
Flags: needinfo?(yzenevich) → needinfo?(gtatum)
I agree that accessibility is important. I'd like to do the right thing here. Do you have anything I can read on proper focus behavior for accessibility? Keyboard shortcuts still work as expected, so I'd like to know what exactly the desired behavior is.

Also changing focus on scrolling affects the experience and performance of the memory tool. The currently focused node can bring up a side panel, so re-focusing on scrolling can remove the side panel that the user is looking at, while they are looking through the list of things in memory. This would also make the side panel be rebuilt on scrolling, which would be slow, negating the entire reason for scrolling virtualization. I don't think that changing focus here is the correct thing. If you could explain why this is important from an accessibility perspective, and how desired focus behavior can work, maybe we can come up with a proper solution. In the short term I would like to get the memory tool working again, as this behavior is not taking into account the virtualized scrolling behavior, and breaks scrolling behavior. We can then file a follow-up bug with the desired accessibility behavior.
Flags: needinfo?(gtatum) → needinfo?(yzenevich)
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #12)
> I agree that accessibility is important. I'd like to do the right thing
> here. Do you have anything I can read on proper focus behavior for
> accessibility? Keyboard shortcuts still work as expected, so I'd like to
> know what exactly the desired behavior is.
> 
> Also changing focus on scrolling affects the experience and performance of
> the memory tool. The currently focused node can bring up a side panel, so
> re-focusing on scrolling can remove the side panel that the user is looking
> at, while they are looking through the list of things in memory. This would
> also make the side panel be rebuilt on scrolling, which would be slow,
> negating the entire reason for scrolling virtualization. I don't think that
> changing focus here is the correct thing. If you could explain why this is
> important from an accessibility perspective, and how desired focus behavior
> can work, maybe we can come up with a proper solution. In the short term I
> would like to get the memory tool working again, as this behavior is not
> taking into account the virtualized scrolling behavior, and breaks scrolling
> behavior. We can then file a follow-up bug with the desired accessibility
> behavior.

Hi, sorry I'm a little laggy with the responses.

Just to clear something up, the focus (or focused item) does not necessarily mean keyboard focus, rather than a state in this component, as far as I understand.

It is probably pretty hard to find any examples that are similar to this use case since fast lists that render only visible part is a fairly modern pattern. However it might be helpful to think of what needs to happen as being very close how keyboard navigation works when a user fills up a form. When using a keyboard you expect to tab between fields in order, or, more importantly, if something steals navigation for the time being (dialog, some sort of widget like calendar), a user expects the focus to be put back where it was originally after the interaction.

Screen readers often give their users context based on keyboard focus position, and navigation is often sequential (like tabbing, or jumping between headings, list items, paragraphs etc). It is one of the most common complaints among the screen reader users that the focus jumps somewhere or goes backs onto the document itself and completely disorients them. The widget would simply be unusable if the user looses navigation context all the time.

So I think since we render away the focused node best behaviour is to:
* if node is simply removed to focus on previous or next if it's a first one in the list (similar to original behaviour).
* if scrolling down to focus on first newly rendered node that is visible, and if scrolling up to focus on the last newly rendered node. 

Regarding performance, I wonder what the original behaviour was? If the node is unmounted does the corresponding panel remain until next selection or would it be removed? If it rebuilds all the time while being scrolled perhaps it makes sense to only re-render when scrolling is done for some time (I wonder if scroll actions are throttled somehow or could be).
Flags: needinfo?(yzenevich) → needinfo?(gtatum)
I'm cleaning up my dashboards and had this needinfo still pointing to me with a lack of a response. I'm not sure on the exact desired behavior for the tree view, other than it was supposed to be fast for performance and provide a view into the data. If I recall my reversion was to make the tree view work again, as this patch regressed behavior of the tree. I'd be happy to provide input if some follow-ups are filed, although I'm not currently actively working on the memory tool, other than maintenance patches to fix regressions.
Flags: needinfo?(gtatum)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.