Closed Bug 1454866 Opened 6 years ago Closed 6 years ago

[a11y] Can't tab to More Information button in Site Security panel

Categories

(Firefox :: Site Identity, defect, P3)

Product:
Firefox
Component:
Site Identity
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

Details

(Keywords: access)

Attachments

(2 files)

Bug 1454866 part 1: PanelMultiView: Provide a way to allow keyboard navigation to an element without affecting visual presentation.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1454866 part 2: Make the buttons in the Site Security panel keyboard navigable.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
STR:
1. Open an https URL.
2. Press the View site information button to open the site identity panel.
3. Press the Show connection details button.
4. Press tab to move through the controls.
Expected: The More Information button should be reachable with the tab key.
Actual: It isn't.

PanelMultiView has specific code to manage keyboard navigation. It looks like it considers elements with .subviewbutton as candidates for keyboard navigation:
https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1369
The More Information button doesn't have this class:
https://searchfox.org/mozilla-central/source/browser/components/controlcenter/content/panel.inc.xul#182
Adding it might fix this. However, I'm not sure if that would also make the button appear differently visually.
NI'ing johannh since he and I worked on this a while back.
Flags: needinfo?(jhofmann)
Yup, that might work.
Flags: needinfo?(jhofmann)
Priority: -- → P3
Adding class subviewbutton to these buttons fixes the issue, but it also affects their visual styling, as I suspected. So, that's not a feasible solution.
Assignee: nobody → jteh
Comment on attachment 8970059 [details]
Bug 1454866 part 1: PanelMultiView: Provide a way to allow keyboard navigation to an element without affecting visual presentation.

https://reviewboard.mozilla.org/r/238814/#review244958

That looks like a good idea to me! Thank you!
Attachment #8970059 - Flags: review?(jhofmann) → review+
Comment on attachment 8970060 [details]
Bug 1454866 part 2: Make the buttons in the Site Security panel keyboard navigable.

https://reviewboard.mozilla.org/r/238816/#review244962

This looks good but do you think you could add tests for this in https://searchfox.org/mozilla-central/source/browser/base/content/test/siteIdentity/browser_identityPopup_focus.js? It would be great to make sure this doesn't regress again...

I'm not going to block on that, but please open a follow-up bug if you don't want to do it in this bug...
Attachment #8970060 - Flags: review?(jhofmann) → review+
Comment on attachment 8970060 [details]
Bug 1454866 part 2: Make the buttons in the Site Security panel keyboard navigable.

https://reviewboard.mozilla.org/r/238816/#review246050

I've added some tests as requested. However, I'd appreciate it if you could take a look at them; I'm not experienced with automated testing for this stuff. In particular:

1. I don't wait for a focus event after sending the tab key, so there could be a timing issue. It seems the PanelMultiView keyboard navigation tests don't either, but it worries me nevertheless. The problem is that I'd rather not wait for a focus event on the specific button. Otherwise, I'd be, for example, waiting for a focus event on the back button, then asserting that the focus is the back button, which seems rather redundant. I'd like to wait for a focus event in general, but I don't see a way to do that. I tried this to wait for a bubbled focus event:

    focused = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "focus");
    EventUtils.sendKey("tab");
    await focused;

but it seems I can't catch a bubbled event in this way... or maybe it gets prevented before it bubbles.

2. I don't test for a specific button on the second press of tab, just for a button which isn't the back button. My concern here is that the button can be different depending on what's loaded. Also, the buttons don't currently have ids, so it'd be difficult to test for a specific button, although I guess we could add ids easily enough. Would you prefer I test for a specific button?
Attachment #8970060 - Flags: review?(jteh)
Comment on attachment 8970060 [details]
Bug 1454866 part 2: Make the buttons in the Site Security panel keyboard navigable.

https://reviewboard.mozilla.org/r/238816/#review246052
Attachment #8970060 - Flags: review?(jteh)
Urg. I can't figure out how to re-request review with mozreview, so using NI instead. Sorry. :(
Flags: needinfo?(jhofmann)
(In reply to James Teh [:Jamie] from comment #9)
> Comment on attachment 8970060 [details]
> Bug 1454866 part 2: Make the buttons in the Site Security panel keyboard
> navigable.
> 
> https://reviewboard.mozilla.org/r/238816/#review246050
> 
> I've added some tests as requested. However, I'd appreciate it if you could
> take a look at them; I'm not experienced with automated testing for this
> stuff. In particular:
> 
> 1. I don't wait for a focus event after sending the tab key, so there could
> be a timing issue. It seems the PanelMultiView keyboard navigation tests
> don't either, but it worries me nevertheless. The problem is that I'd rather
> not wait for a focus event on the specific button. Otherwise, I'd be, for
> example, waiting for a focus event on the back button, then asserting that
> the focus is the back button, which seems rather redundant. I'd like to wait
> for a focus event in general, but I don't see a way to do that. I tried this
> to wait for a bubbled focus event:
> 
>     focused = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup,
> "focus");
>     EventUtils.sendKey("tab");
>     await focused;
> 
> but it seems I can't catch a bubbled event in this way... or maybe it gets
> prevented before it bubbles.

Does using the capture parameter on waitForEvent help with this? https://searchfox.org/mozilla-central/rev/68fdb6cf4f40bea1a1f6c07531ebf58fb8ab060b/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#863

Otherwise I don't mind waiting for the event to happen on that specific button, you can just leave out the assertion in this case. The reason other tests don't wait for it could be that the button does not exist before the panel is created. I don't think it's a problem either, we can just find another solution if the test gets intermittent. :)

> 
> 2. I don't test for a specific button on the second press of tab, just for a
> button which isn't the back button. My concern here is that the button can
> be different depending on what's loaded. Also, the buttons don't currently
> have ids, so it'd be difficult to test for a specific button, although I
> guess we could add ids easily enough. Would you prefer I test for a specific
> button?

Well since you control what's loaded (https://example.com) you can be sure that the button in this case will be the "more information" button, so it would be great to assert that it's really that button. You could also make a page with e.g. mixed active content (see https://mixed-script.badssl.com/) and assert that the button for enabling that is correctly focused, but I really wouldn't require it.

For checking the button you could either give it an id or check that the label equals "More Information". I'd be okay with either solution.

Apart from that the test looks great, I think.

Thanks!
Flags: needinfo?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #12)
> > but it seems I can't catch a bubbled event in this way... or maybe it gets
> > prevented before it bubbles.
> Does using the capture parameter on waitForEvent help with this?

It turns out that focus doesn't bubble, so I used focusin instead. No need for capture. :)

> Well since you control what's loaded (https://example.com) you can be sure
> that the button in this case will be the "more information" button, so it
> would be great to assert that it's really that button.

Done, using an id.

Do you want to give this one more pass or shall I land it after a try run? Which things should I pass to try syntax for this: mochitest-browser or something?
That looks great, thank you.

For stuff like this I usually just run all mochitests, so for example:

try: -b do -p linux64,macosx64,win32,win64 -u mochitests -t none --artifact
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c817a94b000
part 1: PanelMultiView: Provide a way to allow keyboard navigation to an element without affecting visual presentation. r=johannh
https://hg.mozilla.org/integration/autoland/rev/c6a2315cda76
part 2: Make the buttons in the Site Security panel keyboard navigable. r=johannh
https://hg.mozilla.org/mozilla-central/rev/0c817a94b000
https://hg.mozilla.org/mozilla-central/rev/c6a2315cda76
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: