Closed Bug 1660359 Opened 4 years ago Closed 4 years ago

Print Preview document isn't included in the a11y tree

Categories

(Core :: Disability Access APIs, defect, P1)

defect

Tracking

()

VERIFIED FIXED
82 Branch
Tracking Status
firefox81 --- verified
firefox82 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

Details

Attachments

(3 files)

STR (on Windows; keystrokes/focus behaviour might be different on other OS):

  1. Open this URL:
    data:text/html,<a href="https://mozilla.org/">Mozilla</a>
  2. Press shift+tab to focus the print preview.
    • Expected: A11y focus should be fired on the print preview document.
    • Actual: No a11y focus is fired, but the previous focus loses its focused state.

The print preview document doesn't seem to have an accessible at all. This is not a regression caused by the new print modal; this happens with the old print preview as well.

This is pretty nasty for a screen reader user because if this happens, they won't know where focus went; they'll just be in limbo and may not know how to restore focus to something usable.

Would this be fixed (or at least "fixed") by preventing people from tabbing into the preview (bug 1657459)?

Flags: needinfo?(jteh)

If it wasn't focusable at all, that would definitely make this more of a nice-to-have. If the user can't tab into it but can still get focus into it some other way (e.g. f6), that would reduce the severity, but it would still be something I'd want to try to fix.

Flags: needinfo?(jteh)

If I understand correctly, this was done deliberately in bug 1426807 to fix crashes. The problem is that print preview documents are focusable (even prior to the new print modal). Anything that is focusable must have an Accessible to avoid situations like this where the user doesn't know where focus went.

:JWatt, in bug 1426807 comment 26, you noted that a11y shouldn't ever touch these printing documents. Am I correct that these are also used for print preview? If so:

  1. Do you recall the thinking (aside from crashes) as to why a11y should never touch them at all?
  2. Are these meant to be focusable (e.g. so the user can scroll with the keyboard)? If that is the case, they should probably get Accessibles so a11y users at least know they're focused in a print preview doc.
Flags: needinfo?(jwatt)

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

:JWatt, in bug 1426807 comment 26, you noted that a11y shouldn't ever touch these printing documents.

The user should not be able to interact with the print preview document at all, other than to scroll it in order to see what each page looks like in order to check that the visual output is acceptable to them prior to printing. Essentially, it should act like one big raster image. In the comment you linked to above I asked for a11y feedback and later in bug 1426807 comment 29 asurkov pointed me to the code to remove the document from the a11y tree. However, the subtle distinction between "zero interaction" and "no interaction except scrolling" was something we missed, I think.

I guess there are three alternatives I can think of here (let me know your thoughts).

  1. Perhaps we can make the document non-focusable to avoid this issue, although I'm not sure about the impact on scrolling. Offhand I guess mouse wheel/trackpad scrolling would still work, but keyboard scrolling would be broken.

  2. Maybe we should add the print preview documents back to the Accessibility tree, but make only a single Accessible just for the document. I don't know anything about Accessibles, but possibly that would allow the agent to communicate "this is a print preview document with X number of pages, but that's all you get to know about it".

  3. We stop excluding the document from the a11y tree and probably create a bunch of new special Accessibles.

Option 1 and 2 both seem very unsatisfactory since users without visual impairment get to see visually whether a printout is acceptable, but everyone else has no information in order to figure that out (other than being told the number of pages). On reflection it really seems like we should be creating some sort of Accessibles for the document. I guess the Accessibles should start with something that informs the user that this is a special, immutable print preview document, and then presumably we'd need Accessibles to represent a sheet of paper, and each "page" on a sheet of paper, Accessibles for header-left, header-center, etc., and a page contents container Accessible, and then all the normal Accessibles, but immutable versions?

In the immediate term I'm guessing creating a whole bunch of new Assessibles isn't going to be possible in the v81 timeframe, so maybe option 2 is the best way forward for now. The main issue then is how we avoid the crash.

Anyway, I guess the first step is your feedback on the above.

Flags: needinfo?(jwatt) → needinfo?(jteh)

(In reply to Jonathan Watt [:jwatt] from comment #4)

In the comment you linked to above I asked for a11y feedback and later in bug 1426807 comment 29 asurkov pointed me to the code to remove the document from the a11y tree. However, the subtle distinction between "zero interaction" and "no interaction except scrolling" was something we missed, I think.

Right... and just to clarify, I realise this was a miscommunication at that point, not a failure to seek feedback on your part. Apologies if you felt I was implying otherwise.

  1. Perhaps we can make the document non-focusable to avoid this issue, although I'm not sure about the impact on scrolling. Offhand I guess mouse wheel/trackpad scrolling would still work, but keyboard scrolling would be broken.

Correct. Scrolling the print preview with the keyboard is maybe a less common use case (or at least lower severity than breaking screen reader users here), so this might be an acceptable short term option. NI Asa: can we live with not being able to scroll the print preview with the keyboard for 81? How hard should we push here to keep that? Note that being able to focus the print preview also leaves us with bug 1660363.

  1. Maybe we should add the print preview documents back to the Accessibility tree, but make only a single Accessible just for the document. I don't know anything about Accessibles, but possibly that would allow the agent to communicate "this is a print preview document with X number of pages, but that's all you get to know about it".

I agree that would be the best interim option if we still want to support keyboard scrolling of print preview. The tricky question is how to expose an Accessible for the document which has a custom label, which doesn't expose any children and which doesn't risk crashes.

My first thought is that rather than exposing a DocAccessible for the document, we should instead have a11y lie to the client and tell it that the browser element has focus. That way, we can stick an aria-label on the browser element indicating that it's a print preview, without having to create a DocAccessible for the preview and risk crashes. In order to do that, though, we need some C++ way in the parent process of determining that a remote browser is associated with print preview. I can't see a way to do that. printPreviewBrowser.browsingContext.currentWindowGlobal.documentURI.spec returns the URL of the document, not about:printpreview, so I can't use that. The browser does have a class of "printPreviewBrowser", but I'd prefer to avoid hard-coding UI stuff in the a11y code (though it could be an interim hack I guess). Is there some cleaner way to manage this?

  1. We stop excluding the document from the a11y tree and probably create a bunch of new special Accessibles.

Option 1 and 2 both seem very unsatisfactory since users without visual impairment get to see visually whether a printout is acceptable, but everyone else has no information in order to figure that out (other than being told the number of pages). On reflection it really seems like we should be creating some sort of Accessibles for the document. I guess the Accessibles should start with something that informs the user that this is a special, immutable print preview document, and then presumably we'd need Accessibles to represent a sheet of paper, and each "page" on a sheet of paper, Accessibles for header-left, header-center, etc., and a page contents container Accessible, and then all the normal Accessibles, but immutable versions?

I think that would be the ideal, awesome exposure. However:

In the immediate term I'm guessing creating a whole bunch of new Assessibles isn't going to be possible in the v81 timeframe

I tried allowing DocAccessibles to be created for static clone documents. It does work, but it doesn't expose the fact that we're dealing with a print preview, nor does it expose anything about pages, headers, etc. It didn't crash, even on https://github.com/piroor as cited in bug 1426807, but I'm guessing it would crash with certain documents. As you say, this is definitely not feasible in the 81 timeline.

Flags: needinfo?(jwatt)
Flags: needinfo?(jteh)
Flags: needinfo?(asa)

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

we need some C++ way in the parent process of determining that a remote browser is associated with print preview. I can't see a way to do that. printPreviewBrowser.browsingContext.currentWindowGlobal.documentURI.spec returns the URL of the document, not about:printpreview

Ah. But printPreviewBrowser.webNavigation.currentURI.spec does give us about:printpreview.

Flags: needinfo?(jwatt)

Urgh. But nsIBrowser doesn't expose webNavigation, so I can't do this from C++. Which leads me back to the original question. :)

Flags: needinfo?(jwatt)

Before Fission, FocusManager::FocusedDOMNode rejected a11y focus on remote XUL browsers by checking EventStateManager::IsRemoteTarget.
In bug 1594623, code was added to FocusManager::ProcessDOMFocus to prevent a11y focus on OuterDocAccessibles in order to reject focus on OOP iframes.
In bug 1635784, EventStateManager::IsRemoteTarget was renamed to EventStateManager::IsTopLevelRemoteTarget, and EventStateManager::IsRemoteTarget now checks for OOP iframes as well.
This allows us to unify rejection of a11y focus on remote OuterDocAccessibles in FocusManager::FocusedDOMNode.

Print preview documents don't get DocAccessibles because this currently causes crashes and doesn't provide much value.
However, we still want to tell a11y clients something useful when a print preview document gets focus, rather than a11y focus just going nowhere.
Therefore, we allow a11y focus to land on the OuterDocAccessible (browser element) in this case.

This is reported by a11y tools when the print preview has focus.

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

My first thought is that rather than exposing a DocAccessible for the document, we should instead have a11y lie to the client and tell it that the browser element has focus.

WIP D87997 is a working implementation of this, for now using a hacky invented "printpreview" attribute on the browser element. It'd be nice if there were a better way to do this, as per my previous comments.

That way, we can stick an aria-label on the browser element indicating that it's a print preview

And D87998 implements this. My concern here is whether we can get new l10n strings into 81 at this point.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Attachment #9171632 - Attachment description: Bug 1660359 part 2: Allow a11y focus on the print preview OuterDocAccessible. → Bug 1660359 part 2: Allow a11y focus on a remote print preview OuterDocAccessible.

Note that my patch doesn't handle parent process print preview documents; e.g. if you go to about:support and print. I spent a bunch of time trying to make this work, and while I could probably push it over the finish line, it makes a total mess of a11y::FocusManager and feels pretty brittle. There are a few things which make this tricky:

  1. When print preview has focus in the parent process, DOM focus is on the preview document, not the browser element.
  2. nsFocusManager::FocusedBrowsingContext returns the wrong context when a remote print preview document has focus. It returns the top level chrome context instead of the print preview context. That means you can't share a common code path to get the browser element for the focus regardless of local/remote (nsFocusManager::GetFocusedBrowsingContext()->GetEmbedderElement()).
  3. Making FocusManager::FocusedDOMNode lie and return the browser element doesn't work for a parent process print preview document because ProcessDOMFocus returns early if the node it received isn't the node returned by FocusedDOMNode.
  4. Making ProcessDOMFocus call ActiveItemChanged with the browser element in this case doesn't work because DOM subsequently focuses a window, which clears mActiveItem, thus invalidating the focus.

I think this is a pretty rare use case anyway. Given that, if we're eventually going to make the preview document properly accessible, it doesn't make much sense to invest a lot of time into fixing this.

Cancelling NIs:

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

NI Asa: can we live with not being able to scroll the print preview with the keyboard for 81?

No longer necessary, since I fixed both this bug and bug 1660363.

In order to do that, though, we need some C++ way in the parent process of determining that a remote browser is associated with print preview. I can't see a way to do that.

JWatt seems happy with my printpreview attribute hack for now.

Flags: needinfo?(jwatt)
Flags: needinfo?(asa)
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc650bd7d6b8
part 1: Unify rejection of a11y focus on remote OuterDocAccessibles. r=yzen
https://hg.mozilla.org/integration/autoland/rev/697accb3ceb3
part 2: Allow a11y focus on a remote print preview OuterDocAccessible. r=jwatt,yzen
https://hg.mozilla.org/integration/autoland/rev/a447925c9f8e
part 3: Give the print preview browser an a11y label. r=mstriemer,fluent-reviewers,flod,jwatt

Backed out 4 changesets (bug 1657459, bug 1660359) for browser_modal_print.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64%2Fdebug-mochitest-browser-chrome-fis-e10s%2Cbc3&fromchange=da1424ee1d1106d720c4542773dda1d9a4720e4b&tochange=44ee384376ce8b20ef64db6e4fef50af983c2088&selectedTaskRun=K21W1kmUSmuwXJesCqRCGQ.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/44ee384376ce8b20ef64db6e4fef50af983c2088

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314153840&repo=autoland&lineNumber=23157

...
[task 2020-08-27T05:53:15.602Z] 05:53:15     INFO - TEST-INFO | Main app process: exit 0
[task 2020-08-27T05:53:15.603Z] 05:53:15     INFO - TEST-INFO | Confirming we saw 59 DOCSHELL created and 59 destroyed log strings.
[task 2020-08-27T05:53:15.603Z] 05:53:15     INFO - TEST-INFO | Confirming we saw 166 DOMWINDOW created and 166 destroyed log strings.
[task 2020-08-27T05:53:15.604Z] 05:53:15    ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 window(s) until shutdown [url = chrome://global/content/print.html?browsingContextId=51]
[task 2020-08-27T05:53:15.604Z] 05:53:15    ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 window(s) until shutdown [url = about:blank]
[task 2020-08-27T05:53:15.604Z] 05:53:15    ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 window(s) until shutdown [url = chrome://global/content/print.html?browsingContextId=45]
[task 2020-08-27T05:53:15.605Z] 05:53:15     INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | windows(s) leaked: [pid = 18300] [serial = 20], [pid = 18300] [serial = 23], [pid = 18300] [serial = 18], [pid = 18300] [serial = 25], [pid = 18300] [serial = 16], [pid = 18300] [serial = 13]
[task 2020-08-27T05:53:15.605Z] 05:53:15    ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/printing/tests/browser_modal_print.js | leaked 2 docShell(s) until shutdown
[task 2020-08-27T05:53:15.605Z] 05:53:15     INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | docShell(s) leaked: [pid = 18300] [id = {30261d5e-05f0-4696-8924-3d11a57203cd}], [pid = 18300] [id = {d3e90e56-acaf-416d-a417-471c29528435}]
[task 2020-08-27T05:53:15.606Z] 05:53:15     INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | This test created 1 hidden window(s)
[task 2020-08-27T05:53:15.606Z] 05:53:15     INFO - TEST-INFO | toolkit/components/printing/tests/browser_modal_print.js | This test created 1 hidden docshell(s)
...
Flags: needinfo?(jteh)

I pushed this to try after the backout and didn't see any failures, so I guess I'll push this one patch at a time and see how we go.

Flags: needinfo?(jteh)
Keywords: leave-open
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3b7b1a14ab1
part 1: Unify rejection of a11y focus on remote OuterDocAccessibles. r=yzen
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab4e18b490a3
part 2: Allow a11y focus on a remote print preview OuterDocAccessible. r=jwatt,yzen
Keywords: leave-open
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/469aedfd8763
part 3: Give the print preview browser an a11y label. r=mstriemer,fluent-reviewers,flod,jwatt
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Comment on attachment 9171633 [details]
Bug 1660359 part 3: Give the print preview browser an a11y label.

Beta/Release Uplift Approval Request

  • User impact if declined: Screen reader users will get no information at all when the print preview in the new print modal gets focus, potentially causing confusion.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Straightforward, isolated changes which prevent the a11y engine from ignoring focus for print preview and label the preview for a11y users.
  • String changes made/needed: One string added for printing UI.
Attachment #9171633 - Flags: approval-mozilla-beta?
Attachment #9171631 - Flags: approval-mozilla-beta?
Attachment #9171632 - Flags: approval-mozilla-beta?

Flod, are there any l10n concerns from uplifting these patches with the one string change?

Flags: needinfo?(francesco.lodolo)

We're only one week away from the deadline for l10n, and this landed only a couple of days ago (it's still in the quarantine repo at the moment).

If this is breaking the experience for screen reader users, we need to accept the downside (the string will be in English for a few locales, potentially tier 1). If it's only a nice to have, then I'd prefer to have it ride the trains. That is not completely clear to me based on James' uplift request.

P.S. Is the print preview expected to ride the trains to release in fx81?

Flags: needinfo?(francesco.lodolo)

(In reply to Francesco Lodolo [:flod] from comment #25)

P.S. Is the print preview expected to ride the trains to release in fx81?

Yes, it's a Shirley feature.

Jamie, are you OK with this riding without localization for 81?

Flags: needinfo?(jteh)

I think it's better to ride into 81 untranslated in some languages than to not ride at all. Without this, the print preview tab stop will be completely unlabelled for screen reader users, so they won't have any chance of knowing what it is. At least if it's in the wrong language, they have a chance of translating it to know what it is. Obviously, this isn't ideal, but unfortunately, it took a lot longer to get this landed than I'd hoped.

Flags: needinfo?(jteh)

Per conversations with Ryan on Slack, at this point it's better to just uplift without changes, no need for a beta-specific patch.

Comment on attachment 9171631 [details]
Bug 1660359 part 1: Unify rejection of a11y focus on remote OuterDocAccessibles.

Thanks for the input, everyone. Approved for 81.0b6.

Attachment #9171631 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9171632 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9171633 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

HI James,

I'm trying to do the uplift verification but I'm a bit lost. On comment #1, you said that the str are:

Open this URL:
data:text/html,<a href="https://mozilla.org/">Mozilla</a>
Press shift+tab to focus the print preview.
Expected: A11y focus should be fired on the print preview document.
Actual: No a11y focus is fired, but the previous focus loses its focused state.

But if I do just that, open the URL and press shift+tab I get the exactly same result before and after the bug was fixed. So, could you tell me how to tell if the a11y focus is fired on the print preview document, please? Maybe share a screenshot?

Thanks in advance, Flor.

Flags: needinfo?(jteh)

Sorry; I should have provided user-centric STR. Here they are:

STR (with the NVDA screen reader on Windows):

  1. Open this URL:
    data:text/html,<a href="https://mozilla.org/">Mozilla</a>
  2. Press shift+tab to focus the print preview.
    • Expected: NVDA should say something indicating that the print preview has focus.
    • Actual: NVDA says nothing at all.
Flags: needinfo?(jteh)

Hi James,

Thanks for the str. I have verified that the issue is no longer present in FF Nightly 81 and 82 on windows 10.

Regards, Flor.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: