Closed Bug 1440106 Opened 2 years ago Closed 2 years ago

Remove invisible/headless root windows from application accessible

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Currently there are two additional windows that are created when Firefox starts that are not visible to the user and are used for background tasks and processes.

They have a misleading "visible" state.

Running this python script in Linux:

import pyatspi

desktop = pyatspi.Registry.getDesktop(0)
for app in [x for x in desktop if x.name == "Firefox"]:
    print(app)
    for root in app:
        print("  ", root.queryDocument().getAttributeValue('DocURL'))

Returns this output:

[application | Firefox]
   resource://gre-resources/hiddenWindow.html
   chrome://browser/content/browser.xul
   chrome://extensions/content/dummy.xul

...

The first and last children of the app are hidden/dummy windows.
I'm not 100% certain this doesn't regress stuff.
Attachment #8952874 - Flags: review?(jteh)
Assignee: nobody → eitan
Comment on attachment 8952874 [details] [diff] [review]
Filter out hidden window. r?Jamie

This looks fine to me, though I don't have an in-depth understanding of DocShell stuff.

I tested on Windows. Without the patch, you can see these dummy "frame" accessibles as siblings of the top level Firefox frame accessible with NVDA object navigation. I've known about these for ages, but never thought to actually look into it. :) Anyway, these are gone with the patch, which is nice.

I also (briefly) tested context menus, select size="1", extension pop-up documents and the separate "Downloads" window, as these are all cases involving either pop-up windows or separate root windows. All of these work just fine. However, I think it'd be great if Marco can give this a more in-depth spin too.
Attachment #8952874 - Flags: review?(jteh) → review+
Marco, see comment 3. If you can give this a spin, it'd be greatly appreciated.
Works fine for me as well. Thanks!
Comment on attachment 8952874 [details] [diff] [review]
Filter out hidden window. r?Jamie

Review of attachment 8952874 [details] [diff] [review]:
-----------------------------------------------------------------

it'd be nice to add a comment and have Olli to thumb up for the approach.
Attachment #8952874 - Flags: feedback?(bugs)
Comment on attachment 8952874 [details] [diff] [review]
Filter out hidden window. r?Jamie

Review of attachment 8952874 [details] [diff] [review]:
-----------------------------------------------------------------

moving to Olli's review queue for better discoverability :)
Attachment #8952874 - Flags: feedback?(bugs) → review?(bugs)
Comment on attachment 8952874 [details] [diff] [review]
Filter out hidden window. r?Jamie

Looks reasonable to me.
Our IsInvisible handling can be a bit broken in iframes though, if I read the code correctly.
But I assume this a11y code is for top level case only, and if top level docshell is hidden, iframes won't get accessible object either.
Attachment #8952874 - Flags: review?(bugs) → review+
I just want to verify this doesn't blow up tests first
Keywords: checkin-needed
(In reply to Eitan Isaacson [:eeejay] from comment #9)
> I just want to verify this doesn't blow up tests first

oh, sorry, for some reason I assumed try-server were ran
Yeah, I just used try for a windows build above. I already needed to tweak one test. We'll see what happens with the rest:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90905fd9343d7ed34bffc247abcd122183353237
Updated two tests with the pruned tree.
Attachment #8952874 - Attachment is obsolete: true
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd20990e37d
Backed out changeset edc99bb899ac for OS X mochitest failures at docload/test_docload_root.html.
This looks specific to mac. I'm going to try to remedy it next week with a mac, if I fail I may ask to disable these tests on that platform.
Flags: needinfo?(eitan)
I see this comment in a/accessible/tests/mochitest/events/docload/test_docload_root.html:

18          // Front end stuff sometimes likes to stuff things in the hidden window(s)
19          // in which case there's accessibles for that content.

Should we get rid of that comment in this patch, given that we don't consider these windows any more?
(In reply to James Teh [:Jamie] from comment #17)
> I see this comment in
> a/accessible/tests/mochitest/events/docload/test_docload_root.html:
> 
> 18          // Front end stuff sometimes likes to stuff things in the hidden
> window(s)
> 19          // in which case there's accessibles for that content.
> 
> Should we get rid of that comment in this patch, given that we don't
> consider these windows any more?

I don't know how I missed that.. I'll get my hands on a mac again to see what happens if we don't explicitly create accessibles for hidden windows.
No longer blocks: 1386807
See Also: → 1386807
Depends on: 1456997
I think the mac issue has something to do with a different widget tree layout. Anyway, I spun off another bug for that.
Attachment #8954908 - Attachment is obsolete: true
Attachment #8971041 - Flags: review?(jteh)
Comment on attachment 8971041 [details] [diff] [review]
Filter out hidden window. r?Jamie

Review of attachment 8971041 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Happy with an issue addressed. Also, consider making the commit message a bit more descriptive; e.g. "Filter out hidden root windows from the accessibility tree."

::: accessible/tests/mochitest/events/docload/test_docload_root.html
@@ -24,5 @@
> -    gAccService.getAccessibleFor(doc);
> -
> -    // The private hidden window will be lazily created that's why we need to do
> -    // it here *before* loading '../../events.js' or else we'll have a duplicate
> -    // reorder event.

I think the part of this comment talking about loading it before events.js is either no longer relevant or needs to be given some consideration. Why do we care about a reorder event for this test? I'm guessing the former, in which case that bit should be removed.

::: accessible/tests/mochitest/events/docload/test_docload_shutdown.html
@@ -24,5 @@
> -    gAccService.getAccessibleFor(doc);
> -
> -    // The private hidden window will be lazily created that's why we need to do
> -    // it here *before* loading '../../events.js' or else we'll have a duplicate
> -    // reorder event.

Same issue as in other test.
Attachment #8971041 - Flags: review?(jteh) → review+
Ug. Sorry, I put that comment about the reorder event thing on the lines removed instead of their new location. <growls at side by side review> Comment still applies, though, just in the later part of the diff.
Nits addressed. Waiting to land this after freeze.
Attachment #8971041 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fcdac4cee2
Filter out hidden root windows from the accessibility tree. r=Jamie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84fcdac4cee2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: treea11y
Duplicate of this bug: 879741
You need to log in before you can comment on or make changes to this bug.