Closed Bug 1315201 Opened 3 years ago Closed 2 years ago

[a11y] Improve accessibility for Activity Stream context menu

Categories

(Firefox for Android :: Awesomescreen, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.28
Tracking Status
firefox52 --- wontfix
firefox57 --- fixed

People

(Reporter: ahunt, Assigned: mcomella)

References

(Depends on 1 open bug)

Details

(Whiteboard: [MobileAS] 1.26 1.27)

Attachments

(10 files, 1 obsolete file)

90.08 KB, image/png
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
liuche
: review+
Details
59 bytes, text/x-review-board-request
liuche
: review+
Details
Opening the menu with talkback enabled:
- Title for highlight/topsite is repeated
- Focus moves to entire screen, then to menu, and only then to first menu item - we should instead be focused on the first menu item immediately?

(We should also investigate whether there are any useful guidelines for such issues.)

Further issues:
- Menu buttons only state "menu button", we should maybe expand this to say "menu button for highlight/topsite ...?"
- Every menu item states "not ticked, [name],...". These aren't checkable items, hence "not ticked"/"ticked" isn't too useful.
(But we still want to make sure bookmark state is repeated?)
- Menu doesn't autoscroll until you are one item off-screen (i.e. it's as if the system button bar is ignored when determining if scrolling is needed?)
Assignee: nobody → ahunt
Iteration: --- → 1.8
Priority: -- → P1
Whiteboard: [MobileAS]
I'm going to defer this for now - I'm still reading up on accessibility guidelines and recommendations, so it's probably better to reevaluate our accessibility story once I'm better informed about what is actually useful/useless/bad/etc.
Assignee: ahunt → nobody
Iteration: 1.8 → ---
Priority: P1 → P2
Mass wontfix for bugs affecting firefox 52.
Rank: 1
Whiteboard: [MobileAS] → [MobileAS] 1.26
Iteration: --- → 1.26
Priority: P2 → P1
Iteration: 1.26 → 1.27
Whiteboard: [MobileAS] 1.26 → [MobileAS] 1.26 1.27
Assignee: nobody → michael.l.comella
To review what issues still remain:

(In reply to Andrzej Hunt :ahunt from comment #0)
> Opening the menu with talkback enabled:
> - Title for highlight/topsite is repeated
> - Focus moves to entire screen, then to menu, and only then to first menu
> item - we should instead be focused on the first menu item immediately?
> - Menu buttons only state "menu button", we should maybe expand this to say
> "menu button for highlight/topsite ...?"
> - Menu doesn't autoscroll until you are one item off-screen (i.e. it's as if
> the system button bar is ignored when determining if scrolling is needed?)

These all still exist.

> - Every menu item states "not ticked, [name],...". These aren't checkable
> items, hence "not ticked"/"ticked" isn't too useful.
> (But we still want to make sure bookmark state is repeated?)

This does not exist.

> (We should also investigate whether there are any useful guidelines for such
> issues.)

In progress.
Iteration: 1.27 → 1.28
Bug 1382332 and bug 1390356 have been unblocked so I have my plate full for now.

Marco, do you have any a11y resources we could use as developers? Also, if we have questions, do you still do a11y for Firefox for Android? Or do you have another contact who would be more appropriate?
Assignee: michael.l.comella → nobody
Flags: needinfo?(mzehe)
(In reply to Michael Comella (:mcomella) from comment #4)
> Marco, do you have any a11y resources we could use as developers? Also, if
> we have questions, do you still do a11y for Firefox for Android? Or do you
> have another contact who would be more appropriate?

Ressources in terms of developer time looks a bit thin-spread right now, given Firefox 57 Windows E10S and Photon accessibility, but for mere questions you could ping either :eeejay or myself. I still do testing for Android if need arises, but am focused on Windows desktop almost exclusively right now given the crunch time.
Flags: needinfo?(mzehe)
However, the Android folks at Google have revamped their developer resources for accessibility in recent months, making it a lot better as a resource than it used to be. So this is definitely worth checking out for the most up to date information.
Assignee: nobody → michael.l.comella
Note to self: try tablet too.
:eeejay, conventionally, should Talkback users be able to focus and tap outside of a dialog to dismiss it? A Google designed Android control (BottomSheetDialog) is giving me this behavior and it's unexpected. (Technical details below)

---

> - Focus moves to entire screen, then to menu, and only then to first menu
> item - we should instead be focused on the first menu item immediately?

fwiw, in Chrome, when you long press a top site, the first swipe will select the first item of the list. They have no header (with page domain & title) so they don't read it. You can only focus the four list items (e.g. open in tab) on the screen and to exit, you have to go to the back button.

It looks like we're first focusing the entire screen because we're focusing a view that is injected by the BottomSheetDialog. The hierarchy is:

 <CoordinatorLayout>
  <View id=touch_outside> <- focused
  <Framelayout id=design_bottom_sheet> <- our content injected in here

afaict, touch_outside is used by sighted users to tap outside the dialog to dismiss the dialog. However, in Talkback, there is no ContentDescription so it's unclear that this is what would happen. I wrote a hacky patch to reach into the view hierarchy to disable focusability of this view - I'll attach it.

Since this is part of a standard Android control, I'm not sure whether this is intended or not. I asked about this above.

[1]: http://www.codeforge.com/read/463944/design_bottom_sheet_dialog.xml__html
Flags: needinfo?(eitan)
I'll be addressing all the open issues in comment 5 here:

---

To address the context with which the user can enter the context menu:
1) Highlighting a top site (which will speak the top sites title) and long-pressing
2) Highlighting a highlight (which currently speaks all the text attributes: domain, page title, and why it's in highlights - this is subject to change), then the user has to swipe right to the next item (which speaks "Menu button") or manually find it but it's a small hit target on the edge of the view.

For increasing discoverability of 2), I would recommend: also allowing the user to long press the view to open the context menu (but this can conflict with Android's standard selection functionality, if we choose to add it) or increasing the size of the hit target and maybe moving it towards the center.

As for the button title, "Menu button" - the Play Store has a similar behavior in that you highlight an app, swipe right and it says, "Options button" so I think this is probably fine. Their button is more discoverable when seeking because the app hit targets are smaller and thus the options button is a bigger part of that.

---

My proposal for behavior of the context menu, now that the user has opened it (and you have their context from above):
- On dialog open, the page title is announced, nothing is focused (this is consistent with Firefox Beta). Ideally, it also mentions "Showing items x of y" as Firefox Beta does.
- The first focusable item is the title (consistent with Firefox Beta but maybe undesirable since re-announcing the title seems verbose to me) - we announce the domain followed by the page title (both relevant pieces of info)
- Then each item in the list is focusable
- Menu autoscrolls as you swipe left-right to navigate list

Related issues we'll need to address:
- The user can currently focus the dismiss space (open question in comment 8)
- The "Dismiss" button speaks "dismiss" and seems ambiguous. Does this dismiss the dialog or does it dismiss the highlight? (answer: the latter). We could either change the text for everyone or just change the contentDescription.
Bryan, there are a few issues with the UX from an a11y perspective:

1) The "Dismiss" item in the context menu is ambiguous, particularly to non-visual users (does this dismiss the dialog or the highlights?). I'd propose we change the text to "Dismiss from highlights". We could change the non-visual hint text or change it visually so that it affects all users. Do you have an opinion? (See the attached screenshot)

2) The menu button in the highlights, to open the context menu, can be hard to find if you are seeking along the page without visuals. (Note: the menu button can also be found by using swiping navigation but I don't know how frequently that pattern is used) Would we want to change the UX at all to accommodate this? My recommendations would be:
  i) allowing users to long press to open the context menu (but this would mean it'd be harder to add other long-press actions in the future)
  ii) moving the menu button closer to the center of the screen
  iii) making it larger. I might be able to change the hit target just for a11y purposes (e.g. make it half the size of the highlights row) but I think this might go against best practices

fwiw, I intend to make the hit target a little wider without increasing the icon size (so it's easier to hit for motor impairment) and this helps the non-visual issue but doesn't solve it.

Let me know if you want to jump on a call so I can show you how the assistive technologies work on Android.
Flags: needinfo?(bbell)
(In reply to Michael Comella (:mcomella) from comment #10)
> I'll be addressing all the open issues in comment 5 here:
> 
> ---
> 
> To address the context with which the user can enter the context menu:
> 1) Highlighting a top site (which will speak the top sites title) and
> long-pressing

Yup. This is the current behavior, right?

> 2) Highlighting a highlight (which currently speaks all the text attributes:
> domain, page title, and why it's in highlights - this is subject to change),
> then the user has to swipe right to the next item (which speaks "Menu
> button") or manually find it but it's a small hit target on the edge of the
> view.
> 
> For increasing discoverability of 2), I would recommend: also allowing the
> user to long press the view to open the context menu (but this can conflict
> with Android's standard selection functionality, if we choose to add it) or
> increasing the size of the hit target and maybe moving it towards the center.
> 

Yeah, that would be really nice. The inconsistency with the top sites behavior will not be intuitive to a TalkBack user. I just tried double-long tapping and it just opens the page, which is bad.

> As for the button title, "Menu button" - the Play Store has a similar
> behavior in that you highlight an app, swipe right and it says, "Options
> button" so I think this is probably fine. Their button is more discoverable
> when seeking because the app hit targets are smaller and thus the options
> button is a bigger part of that.

I think its fine. The hit target doesn't need to be huge if you add the long press option you mentioned above.

> 
> ---
> 
> My proposal for behavior of the context menu, now that the user has opened
> it (and you have their context from above):
> - On dialog open, the page title is announced, nothing is focused (this is
> consistent with Firefox Beta). Ideally, it also mentions "Showing items x of
> y" as Firefox Beta does.

Yup, that would be nice.

> - The first focusable item is the title (consistent with Firefox Beta but
> maybe undesirable since re-announcing the title seems verbose to me) - we
> announce the domain followed by the page title (both relevant pieces of info)

Yes. And I would remove the menu container from the focus order. It is "activatable" (ie. It says "double tap to activate" after the site title). double tapping it opens a private tab, for me. I think that has something to do with it synthesizing a tap in the center coords of the container. It could just as easily be delete from history. So that's bad.

> - Then each item in the list is focusable

Yup.

> - Menu autoscrolls as you swipe left-right to navigate list

Good luck with that. Things may have improved in Android, but that was my biggest pet peeve when I was working on it.

> 
> Related issues we'll need to address:
> - The user can currently focus the dismiss space (open question in comment 8)

Tapping outside the dialog doesn't really make sense for TalkBack users. The standard way for a TalkBack user to dismiss a dialog is the back button/gesture. And it would probably be the first thing they would try anyway.

If possible, I would remove this entirely from the focus order (its been a while since I did Android a11y, so I can't help on how).

> - The "Dismiss" button speaks "dismiss" and seems ambiguous. Does this
> dismiss the dialog or does it dismiss the highlight? (answer: the latter).
> We could either change the text for everyone or just change the
> contentDescription.

Yeah, good point. I would not diverge the visible from the non-visible language. For many reasons, user support being one of them.


Unrelated to the context menu, I noticed that swiping through the top sites past the end brings you to invisible items.
Flags: needinfo?(eitan)
(In reply to Michael Comella (:mcomella) from comment #11)
> Bryan, there are a few issues with the UX from an a11y perspective:

Spoke with bbell on video.

> 1) The "Dismiss" item in the context menu is ambiguous

He agreed this could be ambiguous without visual context. He decided it's better to keep the "Dismiss" copy consistent across platforms and change the non-visual text to, "Dismiss from Highlights".

> 2) The menu button in the highlights, to open the context menu, can be hard
> to find if you are seeking along the page without visuals.

We decided on:

>   i) allowing users to long press to open the context menu (but this would
> mean it'd be harder to add other long-press actions in the future)

Since we don't plan to have a long-press context menu to select items (and never have) and it's consistent with the top sites behavior.
Flags: needinfo?(bbell)
Thanks for looking over my proposal, eeejay! It's very helpful! Expect to see me on bug 1315197! ;)

(In reply to Eitan Isaacson [:eeejay] from comment #12)
> > 1) Highlighting a top site (which will speak the top sites title) and
> > long-pressing
> 
> Yup. This is the current behavior, right?

Yes.

> > 2) Highlighting a highlight (which currently speaks all the text attributes:
> > domain, page title, and why it's in highlights - this is subject to change),
> > then the user has to swipe right to the next item (which speaks "Menu
> > button") or manually find it but it's a small hit target on the edge of the
> > view.
> > 
> > For increasing discoverability of 2), I would recommend: also allowing the
> > user to long press the view to open the context menu
> 
> Yeah, that would be really nice. The inconsistency with the top sites
> behavior will not be intuitive to a TalkBack user.

As per comment 13, we're doing this.

> > - Menu autoscrolls as you swipe left-right to navigate list
> 
> Good luck with that. Things may have improved in Android, but that was my
> biggest pet peeve when I was working on it.

I did some web searches and I could not find anyone else complaining about this (and thus no solutions!). If you've seen this before and don't have suggestions, I'm not sure there's a great solution.

My thought was if receive an a11y event on a menu item, we could check its view coordinates, see if it was onscreen, and scroll it on-screen if not but it's a concerningly manual process, especially for UX that our core developers don't see every day so are unlikely to find if they break. I'll think whether it's worthwhile.

> > - The user can currently focus the dismiss space (open question in comment 8)
> 
> Tapping outside the dialog doesn't really make sense for TalkBack users. The
> standard way for a TalkBack user to dismiss a dialog is the back
> button/gesture. And it would probably be the first thing they would try
> anyway.
> 
> If possible, I would remove this entirely from the focus order (its been a
> while since I did Android a11y, so I can't help on how).

My WIP patch addresses this but it's hacky since it reaches inside the implementation of a third-party component. Maybe we can write a UI test for this...?

> > - The "Dismiss" button speaks "dismiss" and seems ambiguous. Does this
> > dismiss the dialog or does it dismiss the highlight?
> 
> Yeah, good point. I would not diverge the visible from the non-visible
> language. For many reasons, user support being one of them.

While I didn't raise the issue of user support with Bryan, he decided he wanted to change the non-visual text so that the visual text is consistent across products.

> Unrelated to the context menu, I noticed that swiping through the top sites
> past the end brings you to invisible items.

This is bug 1315197 - thanks for the heads up!
Aaron, in comment 10 I mentioned the problem where the "Dismiss" item in the context menu is ambiguous for non-visual users (does it dismiss the context menu? Or does it dismiss the item from highlights?). I spoke with Bryan and we decided to keep the visual text as "Dismiss" but to change accessibility text to, "Dismiss from Highlights" so that we could keep the visuals consistent across platforms.

However, because of the way it is coded, it's turned out to be non-trivial to override that text (if not impossible without rewriting the whole menu). Do you think I should change the visual text to address the accessibility concerns, to something like "Dismiss from Highlights"?

fwiw, I didn't get a chance to tell Bryan this but :eeejay, one of experienced a11y devs, mentioned in comment 12:

> Yeah, good point. I would not diverge the visible from the non-visible language.
> For many reasons, user support being one of them.
Flags: needinfo?(abenson)
To be explicit, I have not yet implemented:
- (NI comment 15) "Dismiss" is still ambiguous

And I do not intend to implement:
- I don't auto-scroll as the user swipes through the list - :eeejay says it's hard (comment 12) and afaik has been in this game for a while so I don't think it's worth adding the code. My speculative fix is in comment 14.

Otherwise, my patch series for review should be final pending review. :)
Attachment #8897660 - Attachment is obsolete: true
Comment on attachment 8899644 [details]
Bug 1315201: Decrease visibility of BottomSheetContextMenu fields.

https://reviewboard.mozilla.org/r/170952/#review176300
Attachment #8899644 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899645 [details]
Bug 1315201: Add license header to bottomsheet.

https://reviewboard.mozilla.org/r/170954/#review176302
Attachment #8899645 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899646 [details]
Bug 1315201: Do not focus tap-to-dismiss BottomSheetDialog view with Talkback.

https://reviewboard.mozilla.org/r/170956/#review176304
Attachment #8899646 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899647 [details]
Bug 1315201: Do not focus BottomSheetDialog in Talkback.

https://reviewboard.mozilla.org/r/170958/#review176306
Attachment #8899647 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899648 [details]
Bug 1315201: Do not focus AS context menu titles in Talkback.

https://reviewboard.mozilla.org/r/170960/#review176308
Attachment #8899648 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899649 [details]
Bug 1315201: Add long-click support to Highlights for Talkback users.

https://reviewboard.mozilla.org/r/170962/#review176310
Attachment #8899649 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899650 [details]
Bug 1315201: Override AS context menu a11y title.

https://reviewboard.mozilla.org/r/170964/#review176874

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/BottomSheetContextMenu.java:112
(Diff revision 1)
>          navigationView.setNavigationItemSelectedListener(this);
>  
>          super.postInit();
>      }
>  
> +    private void overrideInitialAccessibilityAnnouncement(final View pageDomainView, final View pageTitleView,

Wow, .. that's quite complex. I had to jump between commit message and code a couple of times to understand why we are doing this. Maybe it's worth adding an explanation comment to the method here? :)
Attachment #8899650 - Flags: review?(s.kaspari) → review+
Comment on attachment 8899650 [details]
Bug 1315201: Override AS context menu a11y title.

https://reviewboard.mozilla.org/r/170964/#review176874

> Wow, .. that's quite complex. I had to jump between commit message and code a couple of times to understand why we are doing this. Maybe it's worth adding an explanation comment to the method here? :)

Done. I tried to add the comments inline, but yeah, that got complex. :)
(In reply to Michael Comella (:mcomella) from comment #15)
> Aaron, in comment 10 I mentioned the problem where the "Dismiss" item in the
> context menu is ambiguous for non-visual users (does it dismiss the context
> menu? Or does it dismiss the item from highlights?). I spoke with Bryan and
> we decided to keep the visual text as "Dismiss" but to change accessibility
> text to, "Dismiss from Highlights" so that we could keep the visuals
> consistent across platforms.
> 
> However, because of the way it is coded, it's turned out to be non-trivial
> to override that text (if not impossible without rewriting the whole menu).
> Do you think I should change the visual text to address the accessibility
> concerns, to something like "Dismiss from Highlights"?
> 
> fwiw, I didn't get a chance to tell Bryan this but :eeejay, one of
> experienced a11y devs, mentioned in comment 12:
> 
> > Yeah, good point. I would not diverge the visible from the non-visible language.
> > For many reasons, user support being one of them.

Okay, let's change 'Dismiss' to 'Remove'
Flags: needinfo?(abenson)
The patches in this bug depend on code from this other bug.
Depends on: 1390356
Comment on attachment 8900407 [details]
Bug 1315201: Add activity_stream_welcome_dismiss str.

https://reviewboard.mozilla.org/r/171746/#review177026
Attachment #8900407 - Flags: review?(liuche) → review+
Comment on attachment 8900408 [details]
Bug 1315201: Dismiss -> Remove in AS context menu.

https://reviewboard.mozilla.org/r/171748/#review177028
Attachment #8900408 - Flags: review?(liuche) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4bbea68eee6a
Decrease visibility of BottomSheetContextMenu fields. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/11668ca7abeb
Add license header to bottomsheet. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c4d8e0ccc2ba
Do not focus tap-to-dismiss BottomSheetDialog view with Talkback. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c1a476999fd4
Do not focus BottomSheetDialog in Talkback. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c59592e98081
Do not focus AS context menu titles in Talkback. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/13fc2d5116f6
Add long-click support to Highlights for Talkback users. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a0882ffc12ac
Override AS context menu a11y title. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/bfa1c7a361ba
Add activity_stream_welcome_dismiss str. r=liuche
https://hg.mozilla.org/integration/autoland/rev/29a2d717c525
Dismiss -> Remove in AS context menu. r=liuche
I had to back out bug 1390356, and these patches made changes the same files so had to be backed out in https://hg.mozilla.org/integration/autoland/rev/956d6d351f29
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d07f34a40f91
Decrease visibility of BottomSheetContextMenu fields. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/8eb8d72f35f3
Add license header to bottomsheet. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/18c8a0cd0669
Do not focus tap-to-dismiss BottomSheetDialog view with Talkback. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/9aaeb6ba49bd
Do not focus BottomSheetDialog in Talkback. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/71f52a2fca16
Do not focus AS context menu titles in Talkback. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/21d40ef6a296
Add long-click support to Highlights for Talkback users. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/08f357917170
Override AS context menu a11y title. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/9c6a40a07c69
Add activity_stream_welcome_dismiss str. r=liuche
https://hg.mozilla.org/integration/autoland/rev/32d2d9a521d2
Dismiss -> Remove in AS context menu. r=liuche
NI self: I forgot to test on tablet.
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #63)
> NI self: I forgot to test on tablet.

There is an issue with tablet: I filed bug 1394566.
Depends on: 1394566
Flags: needinfo?(michael.l.comella)
You need to log in before you can comment on or make changes to this bug.