Closed Bug 1488918 Opened 6 years ago Closed 5 years ago

Screenreader check for browser.xhtml

Categories

(Firefox :: Disability Access, task, P3)

Product:
Firefox
Component:
Disability Access
Type:
task

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bgrins, Unassigned)

References

Details

The a11y mochitests are passing in the browser.xhtml build, but I'd like to confirm that things work the same as in browser.xul.

I see some hits on IsXULDocument that make me wonder if there will be different behavior: https://searchfox.org/mozilla-central/search?q=IsXULDocument%28%29, for example:

- native role as application (https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/DocAccessible.cpp#210)
- default buttons (https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/Accessible.cpp#1799
Marco, I've got a build going at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46e940039ac50689f0e598985b64841c3141dc2 - could you please check and let me know if you have any issues using that with a screenreader, or redirect the request as needed?
Flags: needinfo?(mzehe)
Just a basic sanity check would be a great start (open new tab, type in url bar, navigate around some pages, open hamburger menu, etc). Note that there is still quite a bit broken in the build so I don't want to take a ton of your time (as things that aren't working right may just be broken). We can do a more thorough check when we get closer to a green try push.
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Just a basic sanity check would be a great start (open new tab, type in url
> bar, navigate around some pages, open hamburger menu, etc).

Note that the hamburger menu isn't really accessible in Firefox yet (bug 1379387, etc.). I'm guessing this build doesn't change that, so that's not something we can test.

Also, based on bug 1473165, I'm guessing the menu bar won't work yet?
(In reply to Brian Grinstead [:bgrins] from comment #0)
> - native role as application
> (https://searchfox.org/mozilla-central/rev/
> 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/DocAccessible.
> cpp#210)

It seems this only triggers if this is a content document (not chrome). Are XUL content documents even possible these days?

I tested the build and was expecting to see role document, but was pleasantly surprised to see... no change. I guess the rule a few lines above this will trigger instead:

203         if (sameTypeRoot == docShell) {
204           // Root of content or chrome tree
205           if (itemType == nsIDocShellTreeItem::typeChrome)
206             return roles::CHROME_WINDOW;

> - default buttons
> (https://searchfox.org/mozilla-central/rev/
> 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/Accessible.
> cpp#1799

Yeah, I guess this will break. I didn't even know we supported this until now. NVDA doesn't use it. JAWS does have a command to report the default button, so *maybe* they do. It doesn't seem like we set this default="true" attribute in many places, though. Also, it looks to be somewhat broken. It just returns the first button in the document with default="true", but it doesn't check visibility, etc. So, if you open the Add Bookmark dialog (which does set this) and then close it, it'll happily return the Done button from the (now closed) Add Bookmark dialog.

We should check whether JAWS uses this. In the short term, we can just remove the XUL document requirement and always do this if the HTML form check fails.
(In reply to James Teh [:Jamie] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Just a basic sanity check would be a great start (open new tab, type in url
> > bar, navigate around some pages, open hamburger menu, etc).
> 
> Note that the hamburger menu isn't really accessible in Firefox yet (bug
> 1379387, etc.). I'm guessing this build doesn't change that, so that's not
> something we can test.

Oh - I didn't realize that. I would say then that confirming opening / closing tabs, typing in URL bar, navigating backwards/forwards all work would be a great start.

> Also, based on bug 1473165, I'm guessing the menu bar won't work yet?

Actually the menu bar sort of does work now, with the caveat that some key shortcuts for menuitems don't work until you've opened the menu (Bug 1486895). That may be an OSX only bug - I haven't really dug into what's causing that particular issue yet. I should close Bug 1473165 now since we can handle individual breakage in more specific bugs. If you see any issues with screenreader support on the menubar let me know - it should generally work.
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Actually the menu bar sort of does work now,

So I saw in my (very brief) testing.

> with the caveat that some key
> shortcuts for menuitems don't work until you've opened the menu (Bug
> 1486895). That may be an OSX only bug

No, I see it on Windows too. Even things like alt+d/control+l to focus the address bar and control+w to close a tab don't work.
(In reply to Brian Grinstead [:bgrins] from comment #0)
> - native role as application
> (https://searchfox.org/mozilla-central/rev/
> 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/DocAccessible.
> cpp#210)

XUL windows usually represent applications unlike a typical web page, thus we use role application.

> - default buttons
> (https://searchfox.org/mozilla-central/rev/
> 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/Accessible.
> cpp#1799

you don't need to rely on XUL document here. Just querySelector or similar will work well.

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Marco, I've got a build going at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f46e940039ac50689f0e598985b64841c3141dc2 - could you
> please check and let me know if you have any issues using that with a
> screenreader, or redirect the request as needed?

for some reason, it doens't show me a diff, it could be helpful to say whether you do a correct thing :)
(In reply to James Teh [:Jamie] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #0)
> > - native role as application
> > (https://searchfox.org/mozilla-central/rev/
> > 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/DocAccessible.
> > cpp#210)
> 
> It seems this only triggers if this is a content document (not chrome). Are
> XUL content documents even possible these days?

do about:preferences and such live in content, or no?
Priority: -- → P3
> (In reply to James Teh [:Jamie] from comment #4)
> > (In reply to Brian Grinstead [:bgrins] from comment #0)
> > > - native role as application
> > > (https://searchfox.org/mozilla-central/rev/
> > > 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/accessible/generic/DocAccessible.
> > > cpp#210)
> > 
> > It seems this only triggers if this is a content document (not chrome). Are
> > XUL content documents even possible these days?

I believe there are some test harnesses that end up running XUL with content priv, but remotely hosted XUL in product is generally unsupported and planned to be disabled (bug 1460732 / https://groups.google.com/d/msg/firefox-dev/GEDBHOUaVlM/VbeiBlWcCQAJO).

(In reply to alexander :surkov (:asurkov) from comment #8)
> do about:preferences and such live in content, or no?

Not about:preferences specifically - that's a chrome-priv XUL document. Certain in-content about: pages are content priv, but AFAIK those are all HTML documents.
(In reply to alexander :surkov (:asurkov) from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > Marco, I've got a build going at
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=f46e940039ac50689f0e598985b64841c3141dc2 - could you
> > please check and let me know if you have any issues using that with a
> > screenreader, or redirect the request as needed?
> 
> for some reason, it doens't show me a diff, it could be helpful to say
> whether you do a correct thing :)

That try push is the current state of browser.xhtml without any additional changes, so there's no code yet for handling these cases. Given Comment 4, we may only need to remove the XUL document requirement for default buttons and not make a change for the role.

Also note that the two places I pointed out in Comment 0 are just two that I found from scanning https://searchfox.org/mozilla-central/search?q=IsXULDocument%28%29 - there are probably others.
Since Jamie did a test and there are many keyboard shortcuts etc. still broken, clearing NI for now, since I believe some testing was done already. Let me know if and when you need a new test run. :)
Flags: needinfo?(mzehe)
No longer blocks: top-level-html
Depends on: 1533881
Blocks: 1533881
No longer depends on: 1533881

Hi Marco and Jamie, we are getting much more green now for browser.xhtml so I was hoping one of you could do another screenreader test. Here's an (ongoing) try push with it enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4838e76851e37e53553e31de4f70c9cbcb7fd987.

Flags: needinfo?(mzehe)
Flags: needinfo?(jteh)

I don't know why this try push only has OSX builds triggered - it's the same syntax I've used before to include windows and linux. Let me try again, but in the meantime it can be built locally with mk_add_options 'export MOZ_BROWSER_XHTML=1' in the mozconfig file.

OK, still not sure what's going on with those decision tasks, but here's a push that does have Windows binaries: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cda6fb8d7df5b60b91301c2b7cdb75bc95c5d4d

As far as I can see this is still XUL content (including XUL window document element) hosted inside browser.xhtml. As long as document type is the only change, I wouldn't expect major a11y regressions. A11y code has some code paths where IsXULDocument() is used, which returns false now I assume, but those doens't seem relevant or at least critical.

Anyway, I played a bit with VoiceOver, and didn't notice anything suspicious. Marco or Jamie will be in a better spot for sure to make that check. So, If Marco or Jamie notice anything unusual on either platform, then IsXULDocument() places should be inspected, otherwise it's good to enable by default from a11y perspective I'd say.

Sorry, am currently unable to work on this.

Flags: needinfo?(mzehe)

Jamie, here's a recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a84a0b1d89f1e767824498dc6958669ac99a256b. I expect this is all working correctly now but could you do a quick sanity check to make sure stuff like https://bugzilla.mozilla.org/show_bug.cgi?id=1488918#c6 is fixed?

Type: enhancement → task

I did some whirlwind and not very scientific testing of things like: menu bar, alt+d, control+l, control+t, control+w, visiting a URL, alt+leftArrow (Back), alt+rightArrow (Forward), toolbar keyboard navigation, opening/navigating/closing the Firefox hamburger menu, control+shift+j (browser console), activating various context menus with the applications key, control+o. All seemed to work as expected.

Flags: needinfo?(jteh)

(In reply to James Teh [:Jamie] from comment #18)

I did some whirlwind and not very scientific testing of things like: menu bar, alt+d, control+l, control+t, control+w, visiting a URL, alt+leftArrow (Back), alt+rightArrow (Forward), toolbar keyboard navigation, opening/navigating/closing the Firefox hamburger menu, control+shift+j (browser console), activating various context menus with the applications key, control+o. All seemed to work as expected.

Thank you Jamie!

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.