782547 - Accessible focus not fired after dismissing OS print dialog

Status: VERIFIED FIXED

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

Description

[James Teh [:Jamie]]

When a file chooser dialog is closed and focus returns to the main application, an accessible focus event is not fired for the focused object. As far as accessibility clients are concerned, focus is stuck on the top level frame.

Str:
1. Open and focus a document in Firefox.
2. Press control+o to display the Open dialog.
3. Press escape to close it.
Expected: Focus should be fired on the document focused in step 1.
Actual: The last focus event fired is on the top level frame.

In chronological order, here is some event info from AccProbe:
1. When I alt+tab into Firefox, focus is fired correctly. These first two events are probably fired by Windows:
EVENT_SYSTEM_FOREGROUND Event Time: 16:23:28:419, Accessible Name: Audio Description Trial | ABC Television - Nightly, Accessible Role: window, Accessible State: [focusable, moveable, focused, sizeable], Event Data: hwnd=12704D8 ;objectId=0; childId=0; threadId=3048; windowClass=MozillaWindowCl
EVENT_OBJECT_FOCUS Event Time: 16:23:28:419, Accessible Name: Audio Description Trial | ABC Television - Nightly, Accessible Role: frame, Accessible State: [focusable, readOnly, opaque, moveable, active, sizeable], Event Data: hwnd=12704D8 ;objectId=-4; childId=0; threadId=3048; windowClass=MozillaWindowC
This one is then fired by Firefox:
EVENT_OBJECT_FOCUS Event Time: 16:23:28:497, Accessible Name: Audio Description Trial | ABC Television, Accessible Role: document, Accessible State: [focusable, readOnly, opaque, focused], Event Data: hwnd=12704D8 ;objectId=-4; childId=-169846016; threadId=3048; windowClass=Mozil
2. When I press control+o, I get the following:
EVENT_SYSTEM_FOREGROUND Event Time: 16:23:29:636, Accessible Name: Open File, Accessible Role: window, Accessible State: [focusable, moveable, sizeable], Event Data: hwnd=F000F2 ;objectId=0; childId=0; threadId=3048; windowClass=#32770;
EVENT_OBJECT_FOCUS Event Time: 16:23:29:651, Accessible Name: File name:, Accessible Role: text, Accessible State: [focusable, focused], Event Data: hwnd=1DB0116 ;objectId=-4; childId=0; threadId=3048; windowClass=Edit; 
...
3. When I press escape, I get these two (again probably fired by Windows):
EVENT_SYSTEM_FOREGROUND Event Time: 16:23:31:118, Accessible Name: Audio Description Trial | ABC Television - Nightly, Accessible Role: window, Accessible State: [focusable, moveable, focused, sizeable], Event Data: hwnd=12704D8 ;objectId=0; childId=0; threadId=3048; windowClass=MozillaWindowCl
EVENT_OBJECT_FOCUS Event Time: 16:23:31:118, Accessible Name: Audio Description Trial | ABC Television - Nightly, Accessible Role: frame, Accessible State: [focusable, readOnly, opaque, moveable, active, sizeable], Event Data: hwnd=12704D8 ;objectId=-4; childId=0; threadId=3048; windowClass=MozillaWindowC
However, no further focus events are fired unless I move the focus.

This is particularly annoying after specifying a location for a file being downloaded/saved.

Comment 1

[James Teh [:Jamie]]

This also applies to the Print dialog.

Comment 2

[alexander :surkov (:asurkov)]

Enn, I see neither DOM focus nor DOM blur events when system dialogs like Print dialog are open. Is it a bug?

Comment 3

[Neil Deakin]

Looks like WM_KILLFOCUS is sent before WM_ACTIVATE when the dialog opens so we don't receive a lowered event. Then, when the dialog is closed again, we get the right raise messages, but no events are fired because the focus manager thinks the window is already focused.

Comment 4

[alexander :surkov (:asurkov)]

Enn, can you fix it or perhaps cc somebody who knows the area enough to get a fix?

Comment 5

[Neil Deakin]

Attachment: patch

This patch handles the windows messages in either order. In addition, it removes the blur suppression code, since I don't think it is needed any more (It was added to fix crash bug 68454 which I can no longer reproduce).

Comment 6

[Neil Deakin]

Try builds are at https://tbpl.mozilla.org/?tree=Try&rev=d7787295961e

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 673382 [details] [diff] [review]
patch

bug is fixed, mochitest passes. thank you!

Comment 8

[Neil Deakin]

Comment on attachment 673382 [details] [diff] [review]
patch

This patch allows WM_KILLFOCUS and WM_ACTIVATE to be sent in any order. It also removes the blur suppression since I can no longer reproduce the crash that warranted adding that code. I assume it applies only to the old focus code.

Comment 9

[Jim Mathies [:jimm]]

Doesn't appear to have any negative side effects. I'd suggest ample bake time if possible.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6089a8b78d3

Comment 11

[Jim Mathies [:jimm]]

Apparently this has destabilized flash on youtube for some reason.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I've pushed a backout of this patch to m-c. It should be in the next nightly.
https://hg.mozilla.org/mozilla-central/rev/3621795c03e1

Comment 13

[Marco Bonardo [:mak]]

(In reply to Jim Mathies [:jimm] from comment #11)
> Apparently this has destabilized flash on youtube for some reason.

The flash on youtube hangs disappeared for me on latest nightly, so this was likely culprit.

Comment 14

[alexander :surkov (:asurkov)]

Neil, can you do one more shot please?

Comment 15

[alexander :surkov (:asurkov)]

Neil, it continues to hit AT. Do you have a chance to get back to this in foreseeable future?

Comment 16

[Neil Deakin]

I don't.

Comment 17

[alexander :surkov (:asurkov)]

Nomination to 2013q3a11y bug since focus lost is major problem for users (I get continued asked about this by AT vendors). We need somebody to finish the patch. Thoughts/ideas are welcome.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Marco Bonardo [:mak] from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #11)
> > Apparently this has destabilized flash on youtube for some reason.
> 
> The flash on youtube hangs disappeared for me on latest nightly, so this was
> likely culprit.

If no ideas how the patch could breaks things then does it make sense to give the patch another try just to check if those mystical flash problems were gone?

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

I guess this has to do with windowed flash right?

Comment 20

[alexander :surkov (:asurkov)]

it keeps hitting us and we get more and more reports

Comment 21

[Neil Deakin]

Comment 3 and the patch describe more about the issues here if someone else wants to look at this.

Comment 23

[Łukasz Golonka]

This has been reported again on NVDA bug tracker here). Is there any chance for a fix? From an end user perspective this is quite annoying. Additionally previous patch got reverted due to the problems with Flash and it is now think of the past.

Comment 24

[Julien Cochuyt]

Any update?
This has yet again been reported on NVDA bug tracker (here).

Comment 25

[James Teh [:Jamie]]

Unfortunately, while no longer widespread, Flash is still supported and will be until 2020/2021. Regardless, we should try to fix this again or hack around it in the accessibility module if we can't; I agree it's super annoying.

Comment 26

[Mark Osborne]

Hi there. I'm new to Firefox dev, but do you mind if I have a look at this problem?

Comment 27

[Marco Zehe (:MarcoZ)]

Not at all, go right ahead! :) Thank you for volunteering!

Comment 28

[Mark Osborne]

I've had a look (using VS2017 Enterprise) and the behaviour of the focus when coming back from an external dialogue is odd compared to Edge and Chrome. I am trying to identify specifically when focus goes to an external dialogue so that the element that last had the focus can be specially returned to rather than returning focus to the top layer, but not having too much luck yet. I'll keep tinkering.

Comment 29

[James Teh [:Jamie]]

Note that the patch in the attachments section above is worth looking at. While it probably won't apply cleanly now (code style changes among other bit-rot), I imagine the required fix will be pretty similar.

Comment 30

[Mark Osborne]

Attachment: focuspatch.diff

I think we might have addressed this bug. The patch from earlier was very helpful.
I tested successfully using NVDA and opening and closing a print dialog with https://codepen.io/antonpalinau/pen/MWWOoBJ
The focus returned to the print button after the dialog closed, and the down arrow then read out the next line.
The attached patch is based against the nightly build from 13 November. Is that going to be a problem?

Comment 31

[James Teh [:Jamie]]

Thanks very much. This looks pretty good. However the process for submitting code changed some time ago. We now use Phabricator for patch submission and code review. See this page for instructions:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
You can set me (Jamie) as a reviewer. I'll need to add someone else from a different team for final review, as I don't have authority in this module of the code, but I will do an initial pass.

I noticed there are some code format issues here. Before you submit to Phabricator, you can automatically fix those with:
mach clang-format -c HEAD
Then amend the commit with any changes made.

Thanks again!

Comment 32

[Mark Osborne]

Interesting. I ran the following but no code was changed.

marko@mopc /c/mozilla-source/mozilla-central
$ ./mach clang-format -c HEAD
abort: unknown revision 'HEAD'!

marko@mopc /c/mozilla-source/mozilla-central
$ hg heads
changeset: 501607:b7641b3fb45e
tag: tip
parent: 501605:48a708577712
user: Mark Osborne <[e-mail]>
date: Fri Nov 22 04:36:01 2019 +1100
summary: Focus on last focussed control after dialog closed.

marko@mopc /c/mozilla-source/mozilla-central
$ ./mach clang-format -c tip

marko@mopc /c/mozilla-source/mozilla-central
$ hg status

marko@mopc /c/mozilla-source/mozilla-central
$ ./mach clang-format -p widget/windows/nsWindow*
Processing 9 file(s)...

marko@mopc /c/mozilla-source/mozilla-central
$ hg status

marko@mopc /c/mozilla-source/mozilla-central
$

Comment 33

[James Teh [:Jamie]]

Ah, sorry about the HEAD thing; I use git here instead of hg, but that's obviously not the officially recommended config. :)

That's strange. You might also want to try:
mach lint --outgoing --fix

Failing that, the code review bot will pick up any issues when you submit to Phabricator, so no need for concern.

Comment 34

[Mark Osborne]

Attachment: Bug 782547 - Focus on last focussed control after dialog closed; r=Jamie

Comment 35

[Mark Osborne]

Hi there. Is my reply to your comment in the review visible to you?

Comment 36

[James Teh [:Jamie]]

The only comment I see from you is:

Hi there. Still trying to get lint and clang-format working. I think I have addressed the issues in the meantime, though :) Noticed a revision mentioned: 504148 - back out changes that broke eslint

Am I missing something?

Note that if you're making inline comments, you need to hit save draft on the inline comment, then submit to publish changes.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f55c0e5d9cdc
Focus on last focussed control after dialog closed; r=Jamie,NeilDeakin

Comment 38

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f55c0e5d9cdc

Comment 39

[James Teh [:Jamie]]

For some reason, this fixes the Print dialog but not the file chooser (Save As or Open) dialogs. I'm not sure why.

Comment 40

[Mark Osborne]

Should we open a new bug for the Save As and Open dialogs or re-open this one?

Comment 41

[James Teh [:Jamie]]

A new bug should be opened. Thanks.

Comment 42

[James Teh [:Jamie]]

Adjusting summary because this only fixed the print dialog. Save and open dialogs now covered by bug 1606932.

Comment 43

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the initial issue using NVDA 2019.1 and Fx 70.0 (buildID 20191016161957) on Windows 10 x64.

Confirming the fix using Fx 74.0b5, build ID 20200218224219 under the same circumstances.