All users were logged out of Bugzilla on October 13th, 2018

[Settings] Convert accessibility switches to use gaia-switch

RESOLVED FIXED in 2.2 S14 (12june)

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

unspecified
2.2 S14 (12june)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8616496 [details] [review]
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!
Attachment #8616496 - Flags: review?(ejchen)
Target Milestone: --- → 2.2 S14 (12june)
Comment on attachment 8616496 [details] [review]
Pull request - convert accessibility panel to use gaia-switch

Thanks Kevin, this patch looks good to me ! r+
Attachment #8616496 - Flags: review?(eragonj+moz) → review+
(Assignee)

Comment 3

3 years ago
Thanks for the review! In master: https://github.com/mozilla-b2g/gaia/commit/956982413ef87c192ebc778012f6c085e678570b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

3 years ago
Created attachment 8617723 [details] [review]
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.
Attachment #8616496 - Attachment is obsolete: true
Attachment #8617723 - Flags: review+
(Assignee)

Comment 6

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

Er, I mean *didn't* fail on try.
Comment hidden (obsolete)
(Assignee)

Comment 8

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

Comment 9

3 years ago
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!
Flags: needinfo?(yzenevich)
Flags: needinfo?(eitan)
(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?
Flags: needinfo?(yzenevich)
(Assignee)

Comment 11

3 years ago
(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/.
(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.
(Assignee)

Comment 13

3 years ago
(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?)
Flags: needinfo?(yzenevich)
(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?
Flags: needinfo?(yzenevich)
(Assignee)

Comment 15

3 years ago
(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.
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
(Assignee)

Comment 17

3 years ago
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?
Flags: needinfo?(yzenevich)
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.
Flags: needinfo?(eitan)
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.
Flags: needinfo?(yzenevich)
(Assignee)

Comment 20

3 years ago
(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.
(Assignee)

Comment 21

3 years ago
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.
Created attachment 8624843 [details] [review]
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)
Attachment #8624843 - Flags: review?(kgrandon)
(Assignee)

Comment 23

3 years ago
Comment on attachment 8624843 [details] [review]
Fixed tests

This is great, thanks so much for taking care of the tests for me!
Attachment #8624843 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 24

3 years ago
In master: https://github.com/mozilla-b2g/gaia/commit/9fcbca92c63e239510cd5821cd18ad00d4e5e84f
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.