1055355 - [SMS] Make recipients field and suggestions accessible

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Eitan Isaacson [:eeejay]]

When navigating to the "Message" field at the bottom of the thread view or compose new message view double tapping does nothing. I think a role of "textbox" on the edidable content is required.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23013

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8474914 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23013

Hi Eitan, I left some comment on github, please request review again when you're ready, thanks!

BTW, about the attachment inside contentEditable element, it seems another bug to me if double-tap could not work. Could you please confirm if it's a general issue or problem specific to message app?

Comment 3

[Eitan Isaacson [:eeejay]]

Thanks, Steve.

Yeah, we don't really support rich text editing well. So, there is definitely an issue there. I'll come up with another patch and flag it for review.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Steve Chung [:steveck] from comment #2)
> BTW, about the attachment inside contentEditable element, it seems another
> bug to me if double-tap could not work. Could you please confirm if it's a
> general issue or problem specific to message app?

So it took me a while to hunt down the issue. It is not the attachment that is the issue. It is rather a focus problem from bringing up an activity. The screen reader won't restore keyboard focus to the app, and so the IME does not work. I filed another bug for that against the screen reader.

Comment 5

[Eitan Isaacson [:eeejay]]

I took the liberty of doing a bunch of accessibility tweaks for the compose screen. There are still issues with focus/ime that will need to be resolved on the screen reader end. Also, The recipients field needs another round of work that is probably too high risk at this point in the cycle, so I spun off bug 1059060 for that.

The changes in the PR include:
 - Give textbox roles to editable fields.
 - Change placeholder styling to not interfere with the screen reader by altering the entry.
 - Removed MMS label 0 font size hack.
 - Made contact suggestions listbox options.

Comment 6

[Steve Chung [:steveck]]

Thanks for the great efforts on it, I believe we definitely need to create separate bugs to address other issues. However we also got composer visual refresh issue bug 1048845 in review and it might cause some conflicts. So we will land the new layout first and you could address the a11y issue once for all instead of creating follow up bug. Sorry for the inconvenience.

Comment 7

[Eitan Isaacson [:eeejay]]

Hey Steve,

I made sure to coordinate with Oleg on bug 1048845. I just updated the pull request, and worked on all the other accessibility issues in the compose screen:

 - Change placeholder styling to not interfere with the screen reader by altering the entry.
 - Made contact suggestions listbox options.
 - Made recipient field accessible.

The last one is probably the biggest and least trivial change. Besides the role markup (button for assimilated contacts, and textbox for editable node), we needed to give the textbox minimal space. I achieved this with a flex view and some margin hacks that are similar to what was there before.

Flagging you here so that you will remember to review this. Thanks!

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8474914 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23013

Sorry for the late reply, it looks really great and thanks for the further improvement in subject placeholder! It would be perfect if you could rebase to master(some conflicts here) and add unit test for additional recipient role changes. Please request the last review(I think it should be) and we could land this in master.

Comment 9

[Eitan Isaacson [:eeejay]]

[Blocking Requested - why for this release]:

This accounts for a round of accessibility improvements for the SMS app that are required for the screen reader. The changes here are critical for basic functionality. The review process started a while ago, but slipped the branching deadline.

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: this is more like a feature.

Comment 11

[Marco Zehe (:MarcoZ)]

[Blocking Requested - why for this release]:(In reply to Wesley Huang [:wesley_huang] from comment #10)
> triage: this is more like a feature.

This is part of the goal to make the core apps Messaging, Phone, Contacts, and Settings fully accessible for version 2.1. Re-requesting 2.1 blocking.

Comment 12

[Steve Chung [:steveck]]

Hi Eitan, the patch is great! but sorry I still have some questions for you on github. The main concern is the role presentation for other list item, and they seems no need to expose in ally tree either. Could you please confirm it? Thanks!

Comment 13

[Eitan Isaacson [:eeejay]]

(In reply to Steve Chung [:steveck] from comment #12)
> Hi Eitan, the patch is great! but sorry I still have some questions for you
> on github. The main concern is the role presentation for other list item,
> and they seems no need to expose in ally tree either. Could you please
> confirm it? Thanks!

Replied in the PR.

Elements with role presentation get removed from the a11y tree, their descendants still stay though. It is a way to sanitize the tree.

In the case here we have ul -> li -> a. Since a is the actionable node, it should have an appropriate role of 'option'. The spec says that 'options' need a parent of 'listbox', since the DOM parent is an li, we need to remove it and make the ul (with role 'listbox') the direct parent in the a11y tree, this is done via the 'presentation' role which basically says the element is there for layout purposes only. I hope that clears in up further.

Comment 14

[Steve Chung [:steveck]]

(In reply to Eitan Isaacson [:eeejay] from comment #13)
> (In reply to Steve Chung [:steveck] from comment #12)
> > Hi Eitan, the patch is great! but sorry I still have some questions for you
> > on github. The main concern is the role presentation for other list item,
> > and they seems no need to expose in ally tree either. Could you please
> > confirm it? Thanks!
> 
> Replied in the PR.
> 
> Elements with role presentation get removed from the a11y tree, their
> descendants still stay though. It is a way to sanitize the tree.
> 
> In the case here we have ul -> li -> a. Since a is the actionable node, it
> should have an appropriate role of 'option'. The spec says that 'options'
> need a parent of 'listbox', since the DOM parent is an li, we need to remove
> it and make the ul (with role 'listbox') the direct parent in the a11y tree,
> this is done via the 'presentation' role which basically says the element is
> there for layout purposes only. I hope that clears in up further.

Thanks for the explanation. Although we have same  ul -> li -> a structure in the Thread list codebase, but we didn't use <a> for handling the events, so it's fine without attentional role for elements currently. r=me and really appreciate your help :)

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8474914 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23013

BTW, I think it's 2.1 blocker(if we really want to address a11y issue in 2.1) instead of feature because this makes user unable to complete recipient input under a11y senario.

Comment 16

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/a74801edbbe6cd53c407c93f09a4891624fd8b64

Comment 17

[Eitan Isaacson [:eeejay]]

Comment on attachment 8474914 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23013

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Screen reader users will not be able to sent SMS messages
[Testing completed]: Introduced unit tests, and they pass on tbpl b2g-inound.
[Risk to taking this patch] (and alternatives if risky): Minor risks due to styling changes.
[String changes made]: None.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/79dc972d637ff5ef7667b231e93118b4ed83ba9c

Comment 19

[Maria Angeles Oteo (:oteo)]

Removing the flag, as it's already landed in 2.1