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?)

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.

Mass wontfix for bugs affecting firefox 52.

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.

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?

(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.

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.

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

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.

(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.

(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.

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.

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. :)

Comment on attachment 8899644 [details] Bug 1315201: Decrease visibility of BottomSheetContextMenu fields. https://reviewboard.mozilla.org/r/170952/#review176300

Comment on attachment 8899645 [details] Bug 1315201: Add license header to bottomsheet. https://reviewboard.mozilla.org/r/170954/#review176302

Comment on attachment 8899646 [details] Bug 1315201: Do not focus tap-to-dismiss BottomSheetDialog view with Talkback. https://reviewboard.mozilla.org/r/170956/#review176304

Comment on attachment 8899647 [details] Bug 1315201: Do not focus BottomSheetDialog in Talkback. https://reviewboard.mozilla.org/r/170958/#review176306

Comment on attachment 8899648 [details] Bug 1315201: Do not focus AS context menu titles in Talkback. https://reviewboard.mozilla.org/r/170960/#review176308

Comment on attachment 8899649 [details] Bug 1315201: Add long-click support to Highlights for Talkback users. https://reviewboard.mozilla.org/r/170962/#review176310

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? :)

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'

The patches in this bug depend on code from this other bug.

Comment on attachment 8900407 [details] Bug 1315201: Add activity_stream_welcome_dismiss str. https://reviewboard.mozilla.org/r/171746/#review177026

Comment on attachment 8900408 [details] Bug 1315201: Dismiss -> Remove in AS context menu. https://reviewboard.mozilla.org/r/171748/#review177028

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

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

https://hg.mozilla.org/mozilla-central/rev/d07f34a40f91 https://hg.mozilla.org/mozilla-central/rev/8eb8d72f35f3 https://hg.mozilla.org/mozilla-central/rev/18c8a0cd0669 https://hg.mozilla.org/mozilla-central/rev/9aaeb6ba49bd https://hg.mozilla.org/mozilla-central/rev/71f52a2fca16 https://hg.mozilla.org/mozilla-central/rev/21d40ef6a296 https://hg.mozilla.org/mozilla-central/rev/08f357917170 https://hg.mozilla.org/mozilla-central/rev/9c6a40a07c69 https://hg.mozilla.org/mozilla-central/rev/32d2d9a521d2

NI self: I forgot to test on tablet.

(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.