Closed Bug 794041 Opened 12 years ago Closed 11 years ago

nsAccDocManager shouldn't create a RootAccessible for the hiddenWindow

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

part 3 - don't create accessibles for the hiddenWindow
2.54 KB, patch
Details | Diff | Splinter Review
fix test_docload.html to expect accessibles to come from people stuffing stuff in the hidden window, v2
2.45 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
The new tab page preloader injects an iframe into the hiddenWindow which makes accessible/events/test_docload.html fail because it expects only two but sees three accessibles. Everything contained in the hiddenWindow and the hiddenWindow itself should just be ignored and we shouldn't create any accessibles for it.
(In reply to Tim Taubert [:ttaubert] from comment #0)

I agree a11y doesn't need an accessible for hidden documents. But why nsIDocument::IsVisible() is true and they have a presshell and presumably a frame tree? (see conditions the accessible DOM document should comply - http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccDocManager.cpp#359)
Blocks: treea11y
(In reply to alexander :surkov from comment #4)
> I agree a11y doesn't need an accessible for hidden documents. But why
> nsIDocument::IsVisible() is true and they have a presshell and presumably a
> frame tree?

I don't know much about why it is that way. Maybe there's an easier fix somewhere in there?
(In reply to Tim Taubert [:ttaubert] from comment #5)
> (In reply to alexander :surkov from comment #4)
> > I agree a11y doesn't need an accessible for hidden documents. But why
> > nsIDocument::IsVisible() is true and they have a presshell and presumably a
> > frame tree?
> 
> I don't know much about why it is that way. Maybe there's an easier fix
> somewhere in there?

Roc might have ideas.
Looks like nsIDocument::IsVisible() is in sync with the pageshow and pagehide events. It's in fact a real document so they should be fired.
I think the same applies for the PresShell and the frame tree. The hiddenWindow needs to be handled like a real window in terms of layout etc., we just don't paint it.
(In reply to Tim Taubert [:ttaubert] from comment #8)
> I think the same applies for the PresShell and the frame tree. The
> hiddenWindow needs to be handled like a real window in terms of layout etc.,
> we just don't paint it.

there's specific reason I assume?

anyway is there non XPCOM way to know whether the window is hidden or not?
Note: I'm WFH today so away from my windows box, but yesterday I was unable to recreate a11y the test failure.

I'd like to hear from layout/roc before review (or maybe switch to surkov since has begun).
(In reply to David Bolter [:davidb] from comment #10)
> Note: I'm WFH today so away from my windows box, but yesterday I was unable
> to recreate a11y the test failure.

Did you run the test in single mode? That shouldn't fail because the preloader waits for about five seconds before initializing the iframe. Running all a11y/events/ mochitests should trigger it.
BTW, another solution would be to just call nsIAppShellService::GetHidden[DOM]Window() and compare it to the document's defaultView. Not sure if that's better/cheaper?
can you give me steps to reproduce the problem as well?
(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to David Bolter [:davidb] from comment #10)
> > Note: I'm WFH today so away from my windows box, but yesterday I was unable
> > to recreate a11y the test failure.
> 
> Did you run the test in single mode? That shouldn't fail because the
> preloader waits for about five seconds before initializing the iframe.
> Running all a11y/events/ mochitests should trigger it.

Oh hmm yeah I ran the test file on its own using runtests.py --a11y (as per IRC yesterday). That's interesting about the delay in initialization.
(In reply to David Bolter [:davidb] from comment #14)
> Oh hmm yeah I ran the test file on its own using runtests.py --a11y (as per
> IRC yesterday). That's interesting about the delay in initialization.

Sorry, I forgot to mention the delay. You could also set it to zero which should then make the test fail in single mode, too.
(In reply to alexander :surkov from comment #13)
> can you give me steps to reproduce the problem as well?

Sure. You need an up-to-date m-c checkout.

1) Flip 'browser.newtab.preload' to 'true' in browser/app/profile/firefox.js
2) Run 'TEST_PATH=accessible/events/ make -C $objdir mochitest-a11y'

This should give you two errors in 'test_docload.html':

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_docload.html | Different amount of expected children of [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]]. - got 3, expected 2

Here's a try run:

https://tbpl.mozilla.org/?tree=Try&rev=afc14b08d37a
(In reply to Tim Taubert [:ttaubert] from comment #8)
> I think the same applies for the PresShell and the frame tree. The
> hiddenWindow needs to be handled like a real window in terms of layout etc.,
> we just don't paint it.

As I understand what you want to do the point of this document is that you can move it to a different window, where it will be shown, and so we should have an accessible tree for it.  I'm not sure that right now we'll do processing to create an accessible tree when the document is moved.  So we may need to take care of that here too.
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> As I understand what you want to do the point of this document is that you
> can move it to a different window, where it will be shown, and so we should
> have an accessible tree for it.  I'm not sure that right now we'll do
> processing to create an accessible tree when the document is moved.  So we
> may need to take care of that here too.

Yeah, I didn't think of that. We probably should create accessibles for it then, right? The problem is that those accessibles should still not be created for the hiddenWindow but they should be available for the frame contained in the hiddenWindow once we swap the frame to the currently active tab.

Should we rather change the test?
(In reply to Tim Taubert [:ttaubert] from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > As I understand what you want to do the point of this document is that you
> > can move it to a different window, where it will be shown, and so we should
> > have an accessible tree for it.  I'm not sure that right now we'll do
> > processing to create an accessible tree when the document is moved.  So we
> > may need to take care of that here too.
> 
> Yeah, I didn't think of that. We probably should create accessibles for it
> then, right? The problem is that those accessibles should still not be
> created for the hiddenWindow but they should be available for the frame
> contained in the hiddenWindow once we swap the frame to the currently active
> tab.

I think so? I'm not exactly clear what you mean be created for the window.

> Should we rather change the test?

I'm starting to think that's the right approach.
We don't really need an accessible for invisible stuffs. But if we do then those accessible should expose invisible state. If we don't create accessibles then we should make sure to fire document loading events when the document moves from hidden window to normal window. If accessibility can be notified about that then I'd say it's the way to go.
(In reply to alexander :surkov from comment #20)
> We don't really need an accessible for invisible stuffs. But if we do then
> those accessible should expose invisible state. If we don't create
> accessibles then we should make sure to fire document loading events when
> the document moves from hidden window to normal window. If accessibility can
> be notified about that then I'd say it's the way to go.

I guess, on the other hand its another special case, and the situation doesn't really seem all that different from the case of a background tab.

as for invisible state I'm sort of asuming layout will get that part right for us, but that would need to be tested.
(In reply to Trevor Saunders (:tbsaunde) from comment #21)

> I guess, on the other hand its another special case, and the situation
> doesn't really seem all that different from the case of a background tab.

background tab and hidden window must be a different things since AT might be interested in background tab changes but it doesn't seem AT needs to know about hidden windows. On API level: background tab accessibles have offscreen state, hidden window accessible (if we expose them) should have invisible state. 

Hidden window for me look closed to style=display:none.

> as for invisible state I'm sort of asuming layout will get that part right
> for us, but that would need to be tested.

you might be right and we have invisible state on them
(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> 
> > I guess, on the other hand its another special case, and the situation
> > doesn't really seem all that different from the case of a background tab.
> 
> background tab and hidden window must be a different things since AT might
> be interested in background tab changes but it doesn't seem AT needs to know
> about hidden windows. On API level: background tab accessibles have
> offscreen state, hidden window accessible (if we expose them) should have
> invisible state.

So, the purpose of this document is so that it can be made the current tab, so it seems very much if not the same case as a tab that was created in the background and hasn't yet been changed to yet.  So I think actually exposing offscreen state instead of invisible would be fine especially since the user can theoretically make it visible just by changing to a new tab so it gets moved out of the background into the foreground.

> Hidden window for me look closed to style=display:none.

For display: none things we don't create frames, but here we create pres shell and frames in preperation for moving the iframe out of the hiden window into a visible one.
(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> So, the purpose of this document is so that it can be made the current tab,
> so it seems very much if not the same case as a tab that was created in the
> background and hasn't yet been changed to yet.

Ok, simple example. Let's say we have a chat running in the background tab and in the hidden window. Should AT announce both of them?

> > Hidden window for me look closed to style=display:none.
> 
> For display: none things we don't create frames, but here we create pres
> shell and frames in preperation for moving the iframe out of the hiden
> window into a visible one.

ok, style=visibility:hidden
(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> 
> > So, the purpose of this document is so that it can be made the current tab,
> > so it seems very much if not the same case as a tab that was created in the
> > background and hasn't yet been changed to yet.
> 
> Ok, simple example. Let's say we have a chat running in the background tab
> and in the hidden window. Should AT announce both of them?

I tend to think so, but sounds sort of crazy thing to do to me.

For my earlier comparison does it make a difference in what you think should happen if the user has had the tab with the chat in the forground ever?

> > > Hidden window for me look closed to style=display:none.
> > 
> > For display: none things we don't create frames, but here we create pres
> > shell and frames in preperation for moving the iframe out of the hiden
> > window into a visible one.
> 
> ok, style=visibility:hidden


I'm not clear on what difference is between them.
(In reply to Trevor Saunders (:tbsaunde) from comment #25)
 
> > ok, style=visibility:hidden
> 
> 
> I'm not clear on what difference is between them.

display:none doesn't have frames and doesn't affect layout. visibility:hidden has frames and affects layout.
Comment on attachment 664457 [details] [diff] [review]
part 1 - provide a nsIXULWindow.isHiddenWindow attribute

Well, I guess you can have this if you still feel you need it.
Attachment #664457 - Flags: review?(neil) → review+
Attachment #664459 - Flags: review?(dbolter) → review?(surkov.alexander)
Comment on attachment 664460 [details] [diff] [review]
part 3 - make nsAccDocManager not create a RootAccessible for the hiddenWindow

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

Trevor, Alexander, let's get agreement on what we want to do here as soon as possible, since we are blocking Tim. Can we take a patch fixing the test for now?
Attachment #664460 - Flags: review?(dbolter) → review?(surkov.alexander)
(In reply to David Bolter [:davidb] from comment #28)
> Comment on attachment 664460 [details] [diff] [review]
> part 3 - make nsAccDocManager not create a RootAccessible for the
> hiddenWindow
> 
> Review of attachment 664460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Trevor, Alexander, let's get agreement on what we want to do here as soon as
> possible, since we are blocking Tim. Can we take a patch fixing the test for
> now?

well, the question seems to be if just changing the test is the right fix.  I say it is, but I'm not sure Alex agrees yet.
(In reply to David Bolter [:davidb] from comment #28)

> Trevor, Alexander, let's get agreement on what we want to do here as soon as
> possible, since we are blocking Tim. Can we take a patch fixing the test for
> now?

sure: fix a test and keep this bug open to figure out right solution
(In reply to Trevor Saunders (:tbsaunde) from comment #25)

> > Ok, simple example. Let's say we have a chat running in the background tab
> > and in the hidden window. Should AT announce both of them?
> 
> I tend to think so, but sounds sort of crazy thing to do to me.

How can I play with hidden windows? Maybe I miss something.

> For my earlier comparison does it make a difference in what you think should
> happen if the user has had the tab with the chat in the forground ever?

Sorry, can you rephrase the question (I must misunderstand it)?
(In reply to alexander :surkov from comment #30)
> (In reply to David Bolter [:davidb] from comment #28)
> 
> > Trevor, Alexander, let's get agreement on what we want to do here as soon as
> > possible, since we are blocking Tim. Can we take a patch fixing the test for
> > now?
> 
> sure: fix a test and keep this bug open to figure out right solution

Tim do you have a test fix?
Comment on attachment 664459 [details] [diff] [review]
part 2 - add a nsCoreUtils::IsHiddenWindowDocument() helper

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

::: accessible/src/base/nsCoreUtils.cpp
@@ +479,5 @@
> +  nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
> +  docShellTreeItem->GetTreeOwner(getter_AddRefs(treeOwner));
> +  if (!treeOwner) {
> +    return false;
> +  }

Surprised (but happy) to see the brackets.
Attachment #664459 - Flags: review?(surkov.alexander) → review+
Comment on attachment 664460 [details] [diff] [review]
part 3 - make nsAccDocManager not create a RootAccessible for the hiddenWindow

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

Does the trial build test out okay? (Can you get an f+ from Marco?)

Sorry for the delay in review.
Attachment #664460 - Flags: review?(surkov.alexander) → review+
Would be good to get approach agreement from Trevor before landing; I reviewed the patches for correctness.
In general I like delaying processing for stuff that might never be presented. I'm not 100% sure that is a common case for special hidden windows. I'd worry more about this patch if we knew it broke anything or if it impacted perf.
My limited research on this hiddenWindow property (via mxr) makes me believe they are meant to be sort of left alone.

Neil why doesn't  nsXULWindow::GetVisibility return *aVisibility = false for these special hidden windows?
Flags: needinfo?(enndeakin)
(In reply to David Bolter [:davidb] from comment #33)
> Comment on attachment 664459 [details] [diff] [review]
> part 2 - add a nsCoreUtils::IsHiddenWindowDocument() helper
> 
> Review of attachment 664459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsCoreUtils.cpp
> @@ +479,5 @@
> > +  nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
> > +  docShellTreeItem->GetTreeOwner(getter_AddRefs(treeOwner));
> > +  if (!treeOwner) {
> > +    return false;
> > +  }
> 
> Surprised (but happy) to see the brackets.

if we decide this is right approach please remove those

(In reply to David Bolter [:davidb] from comment #35)
> Would be good to get approach agreement from Trevor before landing; I
> reviewed the patches for correctness.

I'm not really sure how you decide if something is correct before you decide what correct behavior is in the first place.
Sigh. I chatted with Trevor on IRC but to clarify here: my r+ is conditional on agreement on the approach. I'm not strictly against the approach but my r+ is not a sagely endorsement of it. Consider it a yes-I-think-this-patch-does-what-you-intend+ flag (but we don't have those). I need to understand what hiddenWindow's are for before I can weigh in on the approach and especially, to have answers to comment 4 (and dupe question in comment 37).

Trevor please note there is a dangling question in comment 31.
Sorry Tim I felt it prudent to remove my review requests here, there are more unknowns than I realized.

But I have good news! I talked to Trevor and can say we all agree fixing the test is the way to go right now and should unblock you. You can do that here or file a spin off bug but please be sure to mention it here.

Let us know if you want help with the test fix.
(In reply to David Bolter [:davidb] from comment #37)
> Neil why doesn't  nsXULWindow::GetVisibility return *aVisibility = false for
> these special hidden windows?

I believe the hidden window is actually made visible but offscreen.

You don't need to create accessibility objects for hidden windows. The only time the hidden window UI is of relevance is on Mac when no windows are open so that menubar still works, but I assume we don't need to implement any special behaviour as Mac should handle any accessibility usage for the built-in menubar already.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #41)
> (In reply to David Bolter [:davidb] from comment #37)
> > Neil why doesn't  nsXULWindow::GetVisibility return *aVisibility = false for
> > these special hidden windows?
> 
> I believe the hidden window is actually made visible but offscreen.
> 
> You don't need to create accessibility objects for hidden windows. The only
> time the hidden window UI is of relevance is on Mac when no windows are open
> so that menubar still works, but I assume we don't need to implement any
> special behaviour as Mac should handle any accessibility usage for the
> built-in menubar already.

Thanks Neil. I think you are correct that the Mac system handles the mac menubar a11y.
Sorry for being so unresponsive. My priorities completely shifted in the past weeks...

(In reply to Neil Deakin from comment #41)
> You don't need to create accessibility objects for hidden windows. The only
> time the hidden window UI is of relevance is on Mac when no windows are open
> so that menubar still works, but I assume we don't need to implement any
> special behaviour as Mac should handle any accessibility usage for the
> built-in menubar already.

Does this mean we'd want the patches I submitted to not create accessibility objects for hidden windows?

(In reply to David Bolter [:davidb] from comment #40)
> But I have good news! I talked to Trevor and can say we all agree fixing the
> test is the way to go right now and should unblock you. You can do that here
> or file a spin off bug but please be sure to mention it here.
> 
> Let us know if you want help with the test fix.

How would I go about fixing the test? It doesn't really know whether there's a preloaded new tab page sitting in the background or where these Accessibles come from, no?
(In reply to Tim Taubert [:ttaubert] from comment #43)
> Sorry for being so unresponsive. My priorities completely shifted in the
> past weeks...
> 
> (In reply to Neil Deakin from comment #41)
> > You don't need to create accessibility objects for hidden windows. The only
> > time the hidden window UI is of relevance is on Mac when no windows are open
> > so that menubar still works, but I assume we don't need to implement any
> > special behaviour as Mac should handle any accessibility usage for the
> > built-in menubar already.
> 
> Does this mean we'd want the patches I submitted to not create accessibility
> objects for hidden windows?

yes but it doesn't seem enough (comment #20).
nsFrameLoader::SwapWithOtherLoader() fires PageShow/Hide events. Isn't this something we could hook onto? nsAccDocManager::HandleEvent() handles PageHide already. Could that just do the same as nsAccDocManager::HandleDOMDocumentLoad()?

For both documents pagehide is fired and the accessibles are destroyed. After swapping pageshow is fired. While the formerly-visible-and-now-hidden document doesn't re-create accessibles because of [part 3] (we should make sure that works) the now-visible document creates them.

Does that approach sound feasible?
If we listen pageshow event for such documents and do HandleDOMDocumentLoad for them then we'd need to avoid that processing for "normal" documents, otherwise HandleDOMDocumentLoad triggers twice.
Sorry if it is obvious to others but the way forward isn't clear to me. Alexander are you thinking comment 20 or bust?
It seems the current approach to recreate the tree (see comment #45)
Talked to Trevor and Ehsan yesterday about this and figured that we should rather fix the test suite than try to not generate accessibles in the hidden window. The hidden window should not be very different from a background (or hidden) tab in that it's not an active part but definitely has accessibles.

This patch makes a11y tests ignore hiddenWindow.document's accessible. That way, if someone ever fixes hidden stuff to not generate accessibles the test will still succeed.
Attachment #664457 - Attachment is obsolete: true
Attachment #664459 - Attachment is obsolete: true
Attachment #664460 - Attachment is obsolete: true
Attachment #709100 - Flags: review?(dbolter)
(In reply to Tim Taubert [:ttaubert] from comment #49)
> Created attachment 709100 [details] [diff] [review]
> fix a11y tests to ignore accessibles from the hiddenWindow
> 
> The hidden window should not be very different from a background (or
> hidden) tab in that it's not an active part but definitely has accessibles.

Expect that screen readers would want to announce the changes in background tabs but in hidden windows probably not (note, VoiceOver even doesn't want accessibles for background tabs).

> This patch makes a11y tests ignore hiddenWindow.document's accessible. That
> way, if someone ever fixes hidden stuff to not generate accessibles the test
> will still succeed.

true but if somebody fixes hidden stuff then this code is rather useless
Comment on attachment 709100 [details] [diff] [review]
fix a11y tests to ignore accessibles from the hiddenWindow

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

Hi Tim, sorry for the delay. This doesn't quite seem like the right approach to me.

I'm curious how many test files have failures without this patch?
Attachment #709100 - Flags: review?(dbolter) → review-
Basically, what I'd prefer to this approach (for now) is to fix any failing tests to expect the hidden window to be in the a11y tree. I can help if you'd like.
(In reply to David Bolter [:davidb] from comment #52)
> I'm curious how many test files have failures without this patch?

It's just accessible/events/test_docload.html that has two failures.

(In reply to David Bolter [:davidb] from comment #53)
> Basically, what I'd prefer to this approach (for now) is to fix any failing
> tests to expect the hidden window to be in the a11y tree. I can help if
> you'd like.

I thought a bit about your suggestion and I'm not sure if that's really the right way to do it. New tab page preloading is to be enabled by default but may as well be disabled again because we might discover a serious bug or what not. It doesn't sound like a good way then to always anticipate accessibles from the hiddenWindow.

Also I would rather not check if browser.newtab.preload=true because that just creates unnecessary dependencies. There are other parts of the code out there (like the social API and the thumbnail service) that (will) access and make use of the hiddenWindow and may or may not also come with prefs. Wouldn't it be easier to filter out accessibles from the hiddenWindow *if* they're present? Like my patch does?
(In reply to Tim Taubert [:ttaubert] from comment #54)

> Also I would rather not check if browser.newtab.preload=true because that
> just creates unnecessary dependencies.

you could add a dependency on Services.appShell.hiddenDOMWindow which you used in your patch like

if (Services.appShell.hiddenDOMWindow) {
  var obj = { role: DOCUMENT }
  accTree.push(obj);
}

and you could wrap it by helper method like addHiddenWindowIfAny(aAccTree);

the difference between your approach and this one is we don't ignore hidden windows in testing what should be better from the point of view of test coverage.

but perhaps making the hidden window inaccessible will take the same time as you spent on fixing the mochitest already ;)
Great. Seems as if nsIDocShell.isOffScreenBrowser has existed for a while without me knowing that this is exactly what we need. It sets the docShell to invisible so we don't create accessibles. Tests are green (try run pending) and we seem to not regress bug 875257.
Attachment #709100 - Attachment is obsolete: true
Attachment #754492 - Flags: review?(jaws)
Try is green:

https://tbpl.mozilla.org/?tree=Try&rev=f792b771ba33

(Important are the m-oth runs. The oranges in m-bc are probably related to new tab page preloading being on by default.)
(In reply to Tim Taubert [:ttaubert] from comment #56)
> Great. Seems as if nsIDocShell.isOffScreenBrowser has existed for a while
> without me knowing that this is exactly what we need. It sets the docShell
> to invisible so we don't create accessibles. Tests are green (try run
> pending) and we seem to not regress bug 875257.

Hmm, this doesn't sound quite right. isOffScreenBrowser was added a long time ago for Fennec, when it was using hidden browsers rendered via <canvas>. The idea was that it made the docShell's "visibility" state not depend on the actual visibility of the <browser> (because the browser was being rendered some other way). So it made nsDocShell::GetVisibility() return true regardless of the actual visibility of the <browser> element (or its ancestors).

Given that, I'm a bit confused about how it would be having an impact on whether we create accessibles - seems like it's worth digging a bit deeper here to understand this.
Comment on attachment 754492 [details] [diff] [review]
mark preloading browser as off-screen so that it doesn't create accessibles

Too bad. I forgot to enable preloading by default locally so that's why tests were green. I have no clue at all why try would be green as well? Anyway, that's not the way to go.
Attachment #754492 - Attachment is obsolete: true
Attachment #754492 - Flags: review?(jaws)
Attachment #664457 - Attachment is obsolete: false
Attachment #664459 - Attachment is obsolete: false
Ok, back to the plan described in comment #45 and comment #46. This patch prevents the DocManager from creating accessibles for anything in the hiddenWindow. Those accessibles will be created when we receive a pageshow event after the docShell has been moved out of the hiddenWindow. The logic is as follows:

* Error pages don't fire pageshow or load events. This is handled by the DOMContentLoaded listener that creates accessibles and notifies accessibility event listeners.

* Normal pages will be handled by pageshow which is fired before the onStateChange handler is called. pageshow will only create accessibles but not fire any accessibility event. The accessibility event will be fired by the onStateChange handler (which also checks for a DocAccessible but should never need to create it because that's been done by pageshow). Only the onStateChange handler knows if the load has been retargeted so that's why I left the event firing part there.

The way it's implemented now, the docShell that is moved out of the hiddenWindow (and therefore receives a pageshow event) will not fire an accessibility load event because that's only done by the onStateChange handler. Is this the correct behavior or would we need to detect when there is a pageshow event creating an accessible for a document that did not fire an a11y load event before?

If you think that's the right way to go then I'll also add a couple of tests to make sure this all is working as expected.
Attachment #754761 - Flags: feedback?(surkov.alexander)
(In reply to Tim Taubert [:ttaubert] from comment #60)
> Created attachment 754761 [details] [diff] [review]
> part 3 - don't create accessibles for the hiddenWindow
> 
> Ok, back to the plan described in comment #45 and comment #46. This patch
> prevents the DocManager from creating accessibles for anything in the
> hiddenWindow. Those accessibles will be created when we receive a pageshow
> event after the docShell has been moved out of the hiddenWindow. The logic
> is as follows:

I still think special casing the hidden window in a11y code is the wrong thing to do here.
(In reply to Trevor Saunders (:tbsaunde) from comment #61)
> (In reply to Tim Taubert [:ttaubert] from comment #60)
> > Created attachment 754761 [details] [diff] [review]
> > part 3 - don't create accessibles for the hiddenWindow
> > 
> > Ok, back to the plan described in comment #45 and comment #46. This patch
> > prevents the DocManager from creating accessibles for anything in the
> > hiddenWindow. Those accessibles will be created when we receive a pageshow
> > event after the docShell has been moved out of the hiddenWindow. The logic
> > is as follows:
> 
> I still think special casing the hidden window in a11y code is the wrong
> thing to do here.

Why is it the wrong thing to do here? Isn't the hidden window in and of itself a special case?
We can't use the pref's to decide if there should be another accessible because of the delay before stuff is loaded into the hidden window.  If we did use the pref there would be a race between the test and when the delay ended.  Also it would mean that running the one test on its own wouldn't work which would suck.
Attachment #755046 - Flags: review?(surkov.alexander)
(In reply to Jared Wein [:jaws] from comment #62)
> (In reply to Trevor Saunders (:tbsaunde) from comment #61)
> > (In reply to Tim Taubert [:ttaubert] from comment #60)
> > > Created attachment 754761 [details] [diff] [review]
> > > part 3 - don't create accessibles for the hiddenWindow
> > > 
> > > Ok, back to the plan described in comment #45 and comment #46. This patch
> > > prevents the DocManager from creating accessibles for anything in the
> > > hiddenWindow. Those accessibles will be created when we receive a pageshow
> > > event after the docShell has been moved out of the hiddenWindow. The logic
> > > is as follows:
> > 
> > I still think special casing the hidden window in a11y code is the wrong
> > thing to do here.
> 
> Why is it the wrong thing to do here? Isn't the hidden window in and of
> itself a special case?

sure its a special snow flake (that's involved in all sorts of things it shouldn't be).  What I probably should have said is that we shouldn't have special cases for what gets an accessible tree, the rule should just be if it gets a pres shell and the rest of layout then it gets an a11y tree too.
(In reply to Trevor Saunders (:tbsaunde) from comment #64)
> sure its a special snow flake (that's involved in all sorts of things it
> shouldn't be).  What I probably should have said is that we shouldn't have
> special cases for what gets an accessible tree, the rule should just be if
> it gets a pres shell and the rest of layout then it gets an a11y tree too.

That sounds reasonable to me. I really tried coming up with a patch like yours and as suggested in comment #55 but I had a really hard time understanding the a11y test code.

Anyway, your patch works great on my machine and I'd be happy to take that. Thanks for helping out!
Attachment #664457 - Attachment is obsolete: true
Attachment #664459 - Attachment is obsolete: true
Attachment #754761 - Attachment is obsolete: true
Attachment #754761 - Flags: feedback?(surkov.alexander)
(In reply to Trevor Saunders (:tbsaunde) from comment #64)

> sure its a special snow flake (that's involved in all sorts of things it
> shouldn't be).  What I probably should have said is that we shouldn't have
> special cases for what gets an accessible tree, the rule should just be if
> it gets a pres shell and the rest of layout then it gets an a11y tree too.

there's no reason to have an accessible for inaccessible thing
Comment on attachment 755046 [details] [diff] [review]
bug 794041 - fix test_docload.html to expect accessibles to come from people stuffing stuff in the hidden window.

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

::: accessible/tests/mochitest/events/test_docload.html
@@ +23,5 @@
> +
> +function maybeAddHiddenWindowAccessibles(aAccTree)
> +{
> +  // Front end stuff sometimes likes to stuff things in the hidden window in
> +  // which case there's accessibles for that content. 

nit: a space at the end of line

@@ +24,5 @@
> +function maybeAddHiddenWindowAccessibles(aAccTree)
> +{
> +  // Front end stuff sometimes likes to stuff things in the hidden window in
> +  // which case there's accessibles for that content. 
> +  if (gAccRetrieval.getAccessibleFor(Services.appShell.hiddenDOMWindow.document))

do I understand right that only one hidden window is allowed per Firefox instance?
Comment on attachment 754761 [details] [diff] [review]
part 3 - don't create accessibles for the hiddenWindow

removing obsolete flag (the patch might be still useful)
Attachment #754761 - Attachment is obsolete: false
(In reply to alexander :surkov from comment #67)
> do I understand right that only one hidden window is allowed per Firefox
> instance?

There is only one hiddenWindow and another hiddenPrivateWindow (that is fairly new) per instance. I think we need to include the private one as well. That's just:

Services.appShell.hiddenPrivateDOMWindow
(In reply to alexander :surkov from comment #66)
> (In reply to Trevor Saunders (:tbsaunde) from comment #64)
> 
> > sure its a special snow flake (that's involved in all sorts of things it
> > shouldn't be).  What I probably should have said is that we shouldn't have
> > special cases for what gets an accessible tree, the rule should just be if
> > it gets a pres shell and the rest of layout then it gets an a11y tree too.
> 
> there's no reason to have an accessible for inaccessible thing

I'd rather not discuss this yet again, but that assumes that it should be inaccessible, and if you do decide that it still doesn't mean a11y is the right place to fix it.
(In reply to Trevor Saunders (:tbsaunde) from comment #70)

> > there's no reason to have an accessible for inaccessible thing
> 
> I'd rather not discuss this yet again,

agree but it'd be good to have an agreement on it

> but that assumes that it should be
> inaccessible, and if you do decide that it still doesn't mean a11y is the
> right place to fix it.

I assume hidden windows have frame tree to make them shown fast and smooth. We don't need that in accessibility. So a11y seems to be a quite right place to fix this. On the another hand a hidden window can be never shown if the user doesn't hit it so we could save some resources making the accessibility engine skipping them.
> > but that assumes that it should be
> > inaccessible, and if you do decide that it still doesn't mean a11y is the
> > right place to fix it.
> 
> I assume hidden windows have frame tree to make them shown fast and smooth.

I don't know the history but I'd suspect its more like nobody thought about not doing layout for the hidden window or maybe the mac menu craziness they were created for required it or something, but I doubt it had anything to do with allowing people to preload stuff there.

> We don't need that in accessibility. So a11y seems to be a quite right place

that doesn't really make sense.  If the goal is to do as little work as possible to make document visible then shouldn't a11y as well as layout do what is possible ahead of time?

> to fix this. On the another hand a hidden window can be never shown if the
> user doesn't hit it so we could save some resources making the accessibility
> engine skipping them.

I think the same argument holds for layout.
(In reply to Trevor Saunders (:tbsaunde) from comment #72)
> I don't know the history but I'd suspect its more like nobody thought about
> not doing layout for the hidden window or maybe the mac menu craziness they
> were created for required it or something, but I doubt it had anything to do
> with allowing people to preload stuff there.

I'm pretty sure that this is how it happened. The hidden window is used by the add-on SDK and by add-ons for various purposes, including docShell swapping like the newtab page preloading feature.

FTR, the hidden window has been used for quite a while now but did never break any tests because this stuff isn't contained in any of our test suites or shipped by default. The newtab page preloader is just the first feature unlucky enough to hit it.
(In reply to Tim Taubert [:ttaubert] from comment #73)
> (In reply to Trevor Saunders (:tbsaunde) from comment #72)
> > I don't know the history but I'd suspect its more like nobody thought about
> > not doing layout for the hidden window or maybe the mac menu craziness they
> > were created for required it or something, but I doubt it had anything to do
> > with allowing people to preload stuff there.
> 
> I'm pretty sure that this is how it happened. The hidden window is used by
> the add-on SDK and by add-ons for various purposes, including docShell
> swapping like the newtab page preloading feature.

it seems this confirms my saying that they do hidden windows layout for fast rendering

(In reply to Trevor Saunders (:tbsaunde) from comment #72)

> > We don't need that in accessibility. So a11y seems to be a quite right place
> 
> that doesn't really make sense.  If the goal is to do as little work as
> possible to make document visible then shouldn't a11y as well as layout do
> what is possible ahead of time?

a11y can be much much slower than visual rendering to keep the user satisfied.
As this is blocking bug 791670 which in turn blocks bug 843853 (a perf P1) can we find a quicker solution for now? There's lots of code out there that already makes use of the hiddenWindow and causes us to create accessibles for it, without any obvious breakage (I don't know of any related bug reports).

I totally understand that the a11y team wants to find and discuss a proper solution and I wonder if we could move this to a follow-up bug? It would be great to unblock bug 791670 with Trevor's patch for now and implement the correct solution later.
Assignee: ttaubert → nobody
I'm totally fine to take Trev's fix as a temporary workaround of the problem as long as the patch is corrected
Ok, re-assigning back to me and stealing Trevor's great spadework.

I hit two problems:

1) getAccessibleFor() forces the creation of accessibles. That means if we use that method we must actually always expect them. As this will cause a duplicate 'reorder' event it must be done before loading 'events.js'. Alternatively we could use getAccessibleFromCache() but this doesn't work for the hiddenPrivateDOMWindow because:

2) The hiddenPrivateDOMWindow is created lazily and thus will emit a load event that causes a duplicate 'reorder' event even if we just access Services.appShell.hiddenPrivateDOMWindow.


The easiest way to solve all this is basically to force the creation of the hiddenPrivateDOMWindow (by accessing) and then force the creation of accessibles for both of these hidden windows (by using getAccessibleFor). That way we can just always expect to see those two extra children.

This patch now also works when preloading the newtab page is disabled, which the previous patch didn't.
Assignee: nobody → ttaubert
Attachment #755046 - Attachment is obsolete: true
Attachment #755046 - Flags: review?(surkov.alexander)
Attachment #757918 - Flags: review?(surkov.alexander)
Comment on attachment 757918 [details] [diff] [review]
fix test_docload.html to expect accessibles to come from people stuffing stuff in the hidden window, v2

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

r=me as long as code is portable, i.e it doesn't fail for example on seamonkey or mobile
Attachment #757918 - Flags: review?(surkov.alexander) → review+
> I hit two problems:
> 
> 1) getAccessibleFor() forces the creation of accessibles. That means if we
> use that method we must actually always expect them. As this will cause a
> duplicate 'reorder' event it must be done before loading 'events.js'.

sorry I didn't catch that when I was testing, I saw that happen once the first time, but it worked after that so I assumed it was just a strange fluke.

Thanks for finsihing it up :)
(In reply to alexander :surkov from comment #78)
> r=me as long as code is portable, i.e it doesn't fail for example on
> seamonkey or mobile

I don't expect any failures as the hidden window has been a core feature for quite some time now.

(In reply to Trevor Saunders (:tbsaunde) from comment #79)
> Thanks for finsihing it up :)

Thank you for guiding me into the right direction!

Pushed:
https://hg.mozilla.org/integration/fx-team/rev/fb85074593db
https://hg.mozilla.org/mozilla-central/rev/fb85074593db
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 879741
Filed bug 879741 as follow-up to decide on Accessibles and hidden windows.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: