1172356 - [Settings] Convert accessibility switches to use gaia-switch

Status: RESOLVED FIXED

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

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Pull request - convert accessibility panel to use gaia-switch

EJ - you did a review of this panel before, could you take a look at this as well? Thanks!

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8616496 [details] [review]
Pull request - convert accessibility panel to use gaia-switch

Thanks Kevin, this patch looks good to me ! r+

Comment 3

[Kevin Grandon :kgrandon]

Thanks for the review! In master: https://github.com/mozilla-b2g/gaia/commit/956982413ef87c192ebc778012f6c085e678570b

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gip(a) failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=2117246&repo=b2g-inbound

Master: https://github.com/mozilla-b2g/gaia/commit/ff6b5114a17fa6d90ccccc70cfeb7e6ae775281d

Comment 5

[Kevin Grandon :kgrandon]

Attachment: Pull request - convert accessibility panel to use gaia-switch

New patch to re-land, carrying review.

Not sure why this failed on b-i, but this patch should fix it.

Comment 6

[Kevin Grandon :kgrandon]

> Not sure why this failed on b-i, but this patch should fix it.

Er, I mean *didn't* fail on try.

Comment 7

[Kevin Grandon :kgrandon]

On try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1605442bfd72

Comment 8

[Kevin Grandon :kgrandon]

Er, that didn't run Gip tests. Here's another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3a3ef2de98f

Comment 9

[Kevin Grandon :kgrandon]

I'm seeing the following error in the log, 'ERROR -  TEST-UNEXPECTED-ERROR | test_a11y_color_filters.py TestColorFiltersAccessibility.test_a11y_color_filters | Exception: accessibility.js error: acc is null'.

I am fairly sure that this is coming from this line here: https://github.com/mozilla-b2g/gaia/blame/master/tests/atoms/accessibility.js#L48

Eitan, Yura - could you guys help me out here? I'm not super familiar with the gaia ui tests, and am not sure what needs to be done to make these accessibility tests pass. I would like to get this stuff working to keep porting components in-tree, while gaia-components matures. Thanks for any help!

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Kevin Grandon (PTO) :kgrandon from comment #9)
> I'm seeing the following error in the log, 'ERROR -  TEST-UNEXPECTED-ERROR |
> test_a11y_color_filters.py
> TestColorFiltersAccessibility.test_a11y_color_filters | Exception:
> accessibility.js error: acc is null'.
> 
> I am fairly sure that this is coming from this line here:
> https://github.com/mozilla-b2g/gaia/blame/master/tests/atoms/accessibility.
> js#L48
> 
> Eitan, Yura - could you guys help me out here? I'm not super familiar with
> the gaia ui tests, and am not sure what needs to be done to make these
> accessibility tests pass. I would like to get this stuff working to keep
> porting components in-tree, while gaia-components matures. Thanks for any
> help!

Hey Kevin, so I was recently working on gaia-switch a11y improvements, as it was not actionable by the screen reader. AFAIK, you are using the existing version for the switch, I'm suspecting that the issue would be resolved if you were to update to the latest version of gaia-switch. Would that be an option?

Comment 11

[Kevin Grandon :kgrandon]

(In reply to Yura Zenevich [:yzen] from comment #10)
> Hey Kevin, so I was recently working on gaia-switch a11y improvements, as it
> was not actionable by the screen reader. AFAIK, you are using the existing
> version for the switch, I'm suspecting that the issue would be resolved if
> you were to update to the latest version of gaia-switch. Would that be an
> option?

This is what we want to do long term, but for now I'm already working on porting a few components to use the things we built within the shared/ folder. This is to get us closer to using the gaia-components some day, but we can start porting for now as the styles are consistent with what's in master.

For now I'd just like to fix the test so I can land with gaia-switch in shared/.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Kevin Grandon (PTO) :kgrandon from comment #11)
> (In reply to Yura Zenevich [:yzen] from comment #10)
> > Hey Kevin, so I was recently working on gaia-switch a11y improvements, as it
> > was not actionable by the screen reader. AFAIK, you are using the existing
> > version for the switch, I'm suspecting that the issue would be resolved if
> > you were to update to the latest version of gaia-switch. Would that be an
> > option?
> 
> This is what we want to do long term, but for now I'm already working on
> porting a few components to use the things we built within the shared/
> folder. This is to get us closer to using the gaia-components some day, but
> we can start porting for now as the styles are consistent with what's in
> master.
> 
> For now I'd just like to fix the test so I can land with gaia-switch in
> shared/.

I would try applying role="checkbox" to the gaia-switch element, that will definitely make them actionable, and hopefully checked/unchecked state works as well.

Comment 13

[Kevin Grandon :kgrandon]

(In reply to Yura Zenevich [:yzen] from comment #12)
> I would try applying role="checkbox" to the gaia-switch element, that will
> definitely make them actionable, and hopefully checked/unchecked state works
> as well.

Thanks Yura! Is there anyway to hide this hook behind the shadowDom, or declare it inside the web component? I think the declarative gaia-switch syntax is much nicer if we don't have to declare it everywhere. (Although the gaia-switch constructor could manually add this attribute I suppose if not?)

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Kevin Grandon (PTO) :kgrandon from comment #13)
> (In reply to Yura Zenevich [:yzen] from comment #12)
> > I would try applying role="checkbox" to the gaia-switch element, that will
> > definitely make them actionable, and hopefully checked/unchecked state works
> > as well.
> 
> Thanks Yura! Is there anyway to hide this hook behind the shadowDom, or
> declare it inside the web component? I think the declarative gaia-switch
> syntax is much nicer if we don't have to declare it everywhere. (Although
> the gaia-switch constructor could manually add this attribute I suppose if
> not?)

Hmm, would that not require updating gaia-switch version anyways?

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Yura Zenevich [:yzen] from comment #14)
> Hmm, would that not require updating gaia-switch version anyways?

Yes, we will do that in the future, but for now I am actively working on getting components landed, without UX changes.

For now I would like to make the existing gaia-switch component work with this test, so - can we make the elements actionable in a way that's hidden from developers and works for these tests?

The component we're working on landing currently lives here: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_switch/script.js

In the future it should be a simple swap to the new component.

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

Yes so essentially we can make it accessible by adding some things similar to this 2 PR's:

https://github.com/gaia-components/gaia-switch/commit/09a150e025921b0606a8a2464dbbecf80380dde6
https://github.com/gaia-components/gaia-switch/commit/ff6038489b822ce83a0c18edc9e80d5e0067ee2f

In particular:
bootstrapping the element with: 
this.setAttribute('role', 'switch');
and 
this.setAttribute('aria-checked', this.checked);

And ensuring that when checked is set, we update:
this.setAttribute('aria-checked', newValue);

hope this helps

Comment 17

[Kevin Grandon :kgrandon]

Yura - I've tried bootstrapping with both role="checkbox" and role="switch", and neither seem to be able to make the _accRetrieval.getAccessibleFor method return anything. Is this a problem with WebComponents? Any ideas for next steps?

Comment 18

[Eitan Isaacson [:eeejay]]

Are you perhaps calling getAccessibleFor on the container? Some elements don't have accessible objects if they are not deemed useful. having a role/aria attribute on an element should guarantee it has an accessible node.

I'll try this patch and see if I could come up with something.

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Kevin, so I looked at the changes and I think you can avoid adding semantics since inner HTML has the input type="checkbox". I think the only thing to ensure from in gaia-switch implementation is that that input gets checked attribute set correctly (no need to deal with the container). Now in tests we just need to get a correct selector for the actual input type checkbox inside the gaia switch. I think a pattern like

By.CSS_SELECTOR, 'gaia-switch[name="accessibility.colors.enable"] input[type="checkbox"]')

should work.

Comment 20

[Kevin Grandon :kgrandon]

(In reply to Yura Zenevich [:yzen] from comment #19)
> By.CSS_SELECTOR, 'gaia-switch[name="accessibility.colors.enable"]
> input[type="checkbox"]')
> 
> should work.

Unfortunately this doesn't work, I'm assuming because the input is behind the shadow root. Let me see if I can figure out an alternative way to change this test.

Comment 21

[Kevin Grandon :kgrandon]

I think I need to do something like this: 

self.accessibility.execute_async_script(
  "Accessibility.click(arguments[0].shadowRoot.querySelector('%s'));" % selector,
  [switch])

Unfortunately I can't seem to figure out how to get these tests to run locally for me, so I'm going to try to figure that out instead of abusing tryserver.

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Fixed tests

Hi Kevin, I updated the tests to work with shadow DOM. Let me know if this looks of (no gaia-switch.js needed)

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8624843 [details] [review]
Fixed tests

This is great, thanks so much for taking care of the tests for me!

Comment 24

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/9fcbca92c63e239510cd5821cd18ad00d4e5e84f