Closed Bug 782547 Opened 8 years ago Closed 3 months ago

Accessible focus not fired after dismissing OS print dialog

Categories

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

x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla74
Tracking Status
firefox74 --- verified

People

(Reporter: Jamie, Assigned: markosborne, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
This also applies to the Print dialog.
Summary: Accessible focus not fired after closing file chooser dialog → Accessible focus not fired after dismissing modal OS dialogs (e.g. file chooser and print dialogs)
Enn, I see neither DOM focus nor DOM blur events when system dialogs like Print dialog are open. Is it a bug?
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.
Enn, can you fix it or perhaps cc somebody who knows the area enough to get a fix?
Attached patch patchSplinter Review
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).
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #673382 - Flags: feedback?(surkov.alexander)
Comment on attachment 673382 [details] [diff] [review]
patch

bug is fixed, mochitest passes. thank you!
Attachment #673382 - Flags: feedback?(surkov.alexander) → feedback+
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.
Attachment #673382 - Flags: review?(jmathies)
Doesn't appear to have any negative side effects. I'd suggest ample bake time if possible.
Attachment #673382 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/b6089a8b78d3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 805813
Apparently this has destabilized flash on youtube for some reason.
Depends on: 805692
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla19 → ---
(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.
Neil, can you do one more shot please?
Status: REOPENED → NEW
Neil, it continues to hit AT. Do you have a chance to get back to this in foreseeable future?
Flags: needinfo?(enndeakin)
I don't.
Flags: needinfo?(enndeakin)
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.
Blocks: 2013q3a11y
Blocks: 2013q4a11y
No longer depends on: 2013q4a11y
(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?
I guess this has to do with windowed flash right?
it keeps hitting us and we get more and more reports
Severity: normal → major
Comment 3 and the patch describe more about the issues here if someone else wants to look at this.
Assignee: enndeakin → nobody
Mentor: enndeakin
Duplicate of this bug: 1229622

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.

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

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.

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

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

Assignee: nobody → markosborne
Status: NEW → ASSIGNED

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.

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.

Attached patch focuspatch.diffSplinter Review

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?

Flags: needinfo?(jteh)

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!

Flags: needinfo?(jteh)

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 <markosborne@pcblues.com>
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
$

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.

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

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.

Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f55c0e5d9cdc
Focus on last focussed control after dialog closed; r=Jamie,NeilDeakin
Status: ASSIGNED → RESOLVED
Closed: 8 years ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

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

Flags: needinfo?(jteh)

A new bug should be opened. Thanks.

Flags: needinfo?(jteh)

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

See Also: → 1606932
Summary: Accessible focus not fired after dismissing modal OS dialogs (e.g. file chooser and print dialogs) → Accessible focus not fired after dismissing OS print dialog
Flags: qe-verify+

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.

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