1260598 - Investigate emitting events for page-tab selection changes prior to events for newly-active document

Status: RESOLVED FIXED

(Firefox :: Disability Access, enhancement, P2)

Description

[Joanmarie Diggs]

Steps to reproduce:
1. Load multiple pages in a single Firefox window
2. Place caret and/or focus within the documents
3. Using Alt+<page tab index> to switch from page to page

Desired results: An event for the page-tab selection change, followed by caret and/or focus events for the newly-active document.

Actual results: Caret and/or focus events for the newly-active document, followed by events for the page-tab selection change.

Rationale: Ideally, a screen reader would speak the newly-selected page tab and then speak where on that newly-active page the user is. If the event ordering were as described in "desired", that would JustWork(tm). Because it's not, accomplishing the ideal behavior requires things like checking if the page associated with the current caret-moved and/or focused event is not the same as the page associated with the previous page location. Given how often the caret moves and/or focus changes, this is a drag.

I was chatting with Trev in IRC about this:

<@tbsaunde> so, I think basically the cause is that the  UI page tab thingy whatever it is doesn't actually get selected until after we've changed documents that are being displayed
<@tbsaunde> so naturally the documents events are fired first
<@tbsaunde> so then the question is what can we do about that
<@tbsaunde> I kind of wonder if we can fix this with aria-selected
<@tbsaunde> that is first set aria-selected="true" (is that the actual attribute I forget ;p) then switch tabs, then select the page tab thingus in the ui

So I'm filing this as an "investigate" request, in case it is doable, for when Trev has some spare time.

Thanks!

Comment 1

[Trevor Saunders (:tbsaunde)]

Gijs this probably more your area than mine?

Comment 2

[:Gijs (he/him)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Gijs this probably more your area than mine?

Well, if I knew what was going on, which I don't... what kind of "page tab selection event" is this? Does this only happen when e10s is on?

Comment 3

[Joanmarie Diggs]

Ironically (which I just now discovered to answer the needinfo), it seems to work as I want when e10s is on. It's only when it's not on that the ordering is not what I want. So I guess that means once everyone is using e10s, problem solved. ;) (With other bugs, however, not sure when that will be the case.)

In answer to your other question: The page tabs are the widgets which display the page title when you have multiple pages open in a single window. Looks like a GtkNotebook.

When e10s is not on, we get the web document events first; then the chrome events. Ideally we'd get the chrome events first and then the web document events.

Comment 4

[:Gijs (he/him)]

(In reply to Joanmarie Diggs from comment #3)
> When e10s is not on, we get the web document events first; then the chrome
> events. Ideally we'd get the chrome events first and then the web document
> events.

Then this sounds like a backend ordering problem which I know nothing about. Trevor, we do async tab switching in e10s, but in non-e10s you select a tab, we update the selected tab and the selected browser at the same time. I don't know why you'd get events from the content first in that circumstance, and I don't know anything about our linux/gtk a11y infrastructure so I don't really know how to dig into that.

Is this a regression? It sounds like the kind of thing that should either have been noticeable since the dawn of time and that nobody bothered telling us about, or that regressed recently - usually you can tell which by how annoying a bug is (more annoying --> gets reported more quickly and more often!) but I can't really tell whether this is showstopper or last-thing-before-the-lights-off polish. It would be good to know if this was a regression, as that might help figure out what's wrong here.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Joanmarie Diggs from comment #3)
> > When e10s is not on, we get the web document events first; then the chrome
> > events. Ideally we'd get the chrome events first and then the web document
> > events.
> 
> Then this sounds like a backend ordering problem which I know nothing about.
> Trevor, we do async tab switching in e10s, but in non-e10s you select a tab,
> we update the selected tab and the selected browser at the same time. I
> don't know why you'd get events from the content first in that circumstance,

I suspect we flush layout for the content document before the chrome one, and that triggers firing queued events for content first.

As a guess in e10s its actually a race for which gets flushed first, but a race  the content process will basically never win?  Maybe not and something does actually make sure we flush layout in the main process before the content process starts changing tabs.

> and I don't know anything about our linux/gtk a11y infrastructure so I don't
> really know how to dig into that.

I suspect this should work similarly on all platforms, and I expect you can see it by listening for nsIAccessibleEvents in js.

Comment 6

[:Gijs (he/him)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> I suspect this should work similarly on all platforms, and I expect you can
> see it by listening for nsIAccessibleEvents in js.

This doesn't really mean anything to me. I tried looking in DXR and (based on https://dxr.mozilla.org/mozilla-central/source/accessible/jsat/EventManager.jsm#693 ) ran this in the browser console of my OS X build:

foo = (s, t, d) => { console.log(s); }; Services.obs.addObserver(foo, "accessible-event", false);

but nothing gets logged when I switch tabs or use caret browsing.

I think Eitan might be a better person to ask, maybe?

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > I suspect this should work similarly on all platforms, and I expect you can
> > see it by listening for nsIAccessibleEvents in js.
> 
> This doesn't really mean anything to me. I tried looking in DXR and (based
> on
> https://dxr.mozilla.org/mozilla-central/source/accessible/jsat/EventManager.
> jsm#693 ) ran this in the browser console of my OS X build:
> 
> foo = (s, t, d) => { console.log(s); }; Services.obs.addObserver(foo,
> "accessible-event", false);
> 
> but nothing gets logged when I switch tabs or use caret browsing.
> 
> I think Eitan might be a better person to ask, maybe?

That works for me. If you tab around a doc you get a ton of events. Enough that it froze my browser and I needed to restart. I would suggest filtering by event type of interest before writing to console.

Comment 8

[:Gijs (he/him)]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > I suspect this should work similarly on all platforms, and I expect you can
> > > see it by listening for nsIAccessibleEvents in js.
> > 
> > This doesn't really mean anything to me. I tried looking in DXR and (based
> > on
> > https://dxr.mozilla.org/mozilla-central/source/accessible/jsat/EventManager.
> > jsm#693 ) ran this in the browser console of my OS X build:
> > 
> > foo = (s, t, d) => { console.log(s); }; Services.obs.addObserver(foo,
> > "accessible-event", false);
> > 
> > but nothing gets logged when I switch tabs or use caret browsing.
> > 
> > I think Eitan might be a better person to ask, maybe?
> 
> That works for me. If you tab around a doc you get a ton of events. Enough
> that it froze my browser and I needed to restart. I would suggest filtering
> by event type of interest before writing to console.

I don't know why it works for you and not for me, but as long as it doesn't work for me, I can't investigate this because I don't know how... I just tried on Windows, and it seems to do nothing there, either.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #8)
> I don't know why it works for you and not for me, but as long as it doesn't
> work for me, I can't investigate this because I don't know how... I just
> tried on Windows, and it seems to do nothing there, either.

Maybe you don't have accessibility initialized?

Add this to your code before observing:
Cc['@mozilla.org/accessibleRetrieval;1'].getService(Ci.nsIAccessibleRetrieval).getAccessibleFor(document)

Comment 10

[Alex ARNAUD]

Dear all,

I have added Marco to CC list.

I'm a blind user and I'm using Debian GNU/Linux and Firefox ESR (45.4.0) and the problem seems to be not resolved yet.

It is a important waste of time to hear the current focused item before the tab title. 

Do you think it would be possible to fix this issue in the next release ?

Best regards.

Comment 11

[Alex ARNAUD]

(In reply to Joanmarie Diggs from comment #3)
> Ironically (which I just now discovered to answer the needinfo), it seems to
> work as I want when e10s is on. It's only when it's not on that the ordering
> is not what I want. So I guess that means once everyone is using e10s,
> problem solved. ;) (With other bugs, however, not sure when that will be the
> case.)

Dear Joanie and all,

I have switched to Firefox nightly (53.0a1) and I use the latest Orca from git but I don't have the title announced by Orca when I switch between tabs.

What do you do to have the title announced by Orca ?

Best regards.

Comment 12

[Raphaël POITEVIN]

Hello,

Even if this bug is solved with e10s, it would be grate if it could be solved without e10s if that mode is not considered so stable for accessibility. Is it planed to be treated soon.

Regards.

Comment 13

[Samuel Thibault]

Attachment: tabswitch

Hello,

Here are my findings:

- The focus event is generated from nsFocusManager::SetFocus, through
FocusManager::NotifyOfDOMFocus, DocAccessible::HandleNotification,
NotificationController::HandleNotification, FocusManager::DispatchFocusEvent,
which uses DocAccessible::FireDelayedEvent to queue the focus event. It is
processed later from EventQueue::ProcessEventQueue

- The tab change event is generated from nsFocusManager::SetFocus through
nsFocusManager::CheckIfFocusable, nsDocument::FlushPendingNotifications,
PresShell::DoFlushPendingNotifications, RestyleManager::ProcessPendingRestyles,
[...], nsCSSFrameConstructor::RecreateFramesForContent,
nsCSSFrameConstructor::ContentRangeInserted, ...
NotificationController::ScheduleContentInsertion. That latter function
only queues the content insertion. That is processed from
nsRefreshDriver::Tick through NotificationController::WillRefresh,
DocAccessible::ProcessContentInserted, which eventually queues with
NotificationController::QueueMutationEvent. That again only queues it, but
it's processed right after from NotificationController::WillRefresh through
NotificationController::ProcessMutationEvents.

So it seems the two are indeed not synchronized: the focus event is processed
from ProcessEventQueue(), while the tab even is processed from Tick(), i.e. from
times to times only, so no wonder it usually comes after :)

In the attached patch, I have added an explicit Tick() call in 
nsFocusManager::CheckIfFocusable, right after having called 
nsDocument::FlushPendingNotifications. That way, it will really process the 
tab events that got queued in ScheduleContentInsertion without waiting for 
next Tick(), and before returning from CheckIfFocusable(), thus before focus 
events are generated. This should thus provide the proper event ordering. It 
also kind of makes sense to me: before setting the focus, one should make sure 
that content changes should be applied.

Joanmarie, Alex, Raphaël, could you test it?
(I have put debian packages (version +tabswitch) on
deb http://people.debian.org/~sthibault/tmp/jessie ./
deb http://people.debian.org/~sthibault/tmp/stretch ./
deb http://people.debian.org/~sthibault/tmp/buster ./ )

Comment 14

[Samuel Thibault]

Attachment: setcaretoffset-removed

Hello,

Intensive testing showed that the patch I proposed triggers another bug which apparently was just unlikely before, and is now likely in some corner cases which we have seen on heavy-loaded pages such as http://www.voyages-sncf.com/

The attached patch fixes this, so for people to try the tabswitch patch, please also apply this patch to avoid crashes.

What happens is that

- Orca might call SetCaretOffset for objects which have disappeared.
- Normally, at-spi2-atk's impl_SetCaretOffset would in such case just not find the object,
- but there is a window between Accessible::Shutdown() (which resets the SelectionManager cache) and HyperTextAccessible::~HyperTextAccessible() during which the Accessible still exists.
- If the SetCaretOffset request comes during the window, when SetCaretOffset calls SelectionManager::UpdateCaretOffset, the object gets referenced in the SelectionManager cache.
- When the object actually gets destroyed from nsCycleCollector_doDeferredDeletion, ~HyperTextAccessible() is called but does nothing, so the object remains referenced from the SelectionManager cache.
- From then on, if Orca requests get CaretOffset, HyperTextAccessible::CaretOffset gets the object from the cache, and tries to call GetNode on it, which crashes.

The proposed patch modifies HyperTextAccessible::SetCaretOffset to avoid forcing the cache refresh when the object has been shut down, just not collected yet. I guess other strategies could be to remove the object from the SelectionManager cache, or to make Accessible::Shutdown() remove the object from sight of at-spi2-atk's.

Comment 15

[Samuel Thibault]

I have filed this issue as a separate bug report, with backtraces etc. in https://bugzilla.mozilla.org/show_bug.cgi?id=1406043

Comment 16

[Samuel Thibault]

Attachment: proposed fix

This is the same proposed fix, only the formatting changes

Comment 17

[Samuel Thibault]

After more testing, this change however seems to modify the behavior of Orca's speak on the toolbar: the title of the document is always spoken by Orca before toolbar elements. For instance:

- open firefox, the title of the default page on my system is "Lilo - Mozilla Firefox"
- press alt-d, to reach the URL toolbar element.
- press tab, to reach the search toolbar element.

Orca speaks "Lilo - Mozilla Firefox" before talking about the search toolbar element (also first describing that it is a search toolbar element etc.). Without the proposed fix, on pressing tab, only the content of the search toolbar element is spoken.

Joanmarie, do you have ideas on the Orca side as to why this could happen?  (My change only flushes some events earlier)

Comment 18

[Joanmarie Diggs]

I'll take a look, but right now I'm desperately trying to finalize implementation results for ARIA 1.1 and Core AAM 1.1 so they can transition to PR. Leaving the needinfo for now.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 8917324 [details] [diff] [review]
proposed fix

We probably should think of a fix on a11y side, but just in case, let's run the idea through Olli, if it has general applicability.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8917324 [details] [diff] [review]
proposed fix

This is against the HTML spec. And this would probably slow down speedometer quite a bit.

Comment 21

[Samuel Thibault]

Hello,

What is against the HTML spec exactly?

Samuel

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

running refresh driver tick at that time, which is basically the step 7 in
https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #0)
> Steps to reproduce:
> 1. Load multiple pages in a single Firefox window
> 2. Place caret and/or focus within the documents
> 3. Using Alt+<page tab index> to switch from page to page
> 
> Desired results: An event for the page-tab selection change, followed by
> caret and/or focus events for the newly-active document.
> 
> Actual results: Caret and/or focus events for the newly-active document,
> followed by events for the page-tab selection change.

We should think of event synchronization mechanism between chrome and content process, for example, if the chrome wants to, then the content should give it more time to finish the things.

Aaron, do you have any thoughts on this?

(cancelling Joanie ni? request, since the original approach, Joanie was pinged about, is a no-go, see comment 20).

Comment 24

[:Gijs (he/him)]

(In reply to alexander :surkov from comment #23)
> (In reply to Joanmarie Diggs from comment #0)
> > Steps to reproduce:
> > 1. Load multiple pages in a single Firefox window
> > 2. Place caret and/or focus within the documents
> > 3. Using Alt+<page tab index> to switch from page to page
> > 
> > Desired results: An event for the page-tab selection change, followed by
> > caret and/or focus events for the newly-active document.
> > 
> > Actual results: Caret and/or focus events for the newly-active document,
> > followed by events for the page-tab selection change.
> 
> We should think of event synchronization mechanism between chrome and
> content process, for example, if the chrome wants to, then the content
> should give it more time to finish the things.
> 
> Aaron, do you have any thoughts on this?

I expect :surkov meant to CC & ping Aaron... :-)

Comment 25

[alexander :surkov (:asurkov)]

(In reply to :Gijs (no reviews; PTO recovery mode) from comment #24)
> > Aaron, do you have any thoughts on this?
> 
> I expect :surkov meant to CC & ping Aaron... :-)

thank you :)

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to alexander :surkov from comment #23)
> We should think of event synchronization mechanism between chrome and
> content process, for example, if the chrome wants to, then the content
> should give it more time to finish the things.
> 
> Aaron, do you have any thoughts on this?

Content dispatches events by sending them to the parent process over IPDL, and then the parent actually fires the platform-specific event code on content's behalf. In other words, the parent process has access to both chrome and content events.

Content events are received by DocAccessibleParent::Recv*Event functions and then routed to the a11y::ProxyEvent function (which lives in each platform's AccessibleWrap).

I would think that you could reroute those content events into some kind of event management code to handle the coordination of the chrome and content events, and then dispatch them from there.

Comment 27

[Samuel Thibault]

Hello,

AIUI, in the matter at stake, that means, in DocAccessible::HandleNotification, to queue the notification on a queue instead of passing it immediately to NotificationController::HandleNotification, and actually process it later, in nsRefreshDriver::Tick() (actually, from NotificationController::WillRefresh), *after* the content update processing (loop around calling DocAccessible::ProcessContentInserted), is that right?

Comment 28

[Samuel Thibault]

Attachment: patch

Actually... There is already some synchronization queing implemented in NotificationController::HandleNotification: before immediately processing notifications, it calls IsUpdatePending, which looks at mContentInsertions, which is where the content insertion for the tab is queued, but... in a different NotificationController object, and thus IsUpdatePending returns false.

Indeed, "this" is different in the DocAccessible::ContentInserted for the tab content insertion and in DocAccessible::HandleNotification for the focus event: the former is the parent of the latter.

We could just make IsUpdatePending look at the parent's mContentInsertions too, as the attached patch does, but that pushes the issue only a bit further: events for the child happen to get processed before events for the parent. Perhaps we could change WillRefresh to process the parent's mContentInsertions first if there are any?

Comment 29

[Samuel Thibault]

Attachment: patch2

This additional patch makes WillRefresh look at its parent's content insertion queue, that does seem to be fixing the issue. What do you think?

Comment 30

[Samuel Thibault]

Olli, perhaps you have an opinion on these two patches?

Joanmarie, perhaps you can test them to make sure that Orca now gets the proper ordering?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

I'm not familiar enough with a11y code to say anything about the patches.
aklotz and surkov might have opinions.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #31)
> I'm not familiar enough with a11y code to say anything about the patches.
> aklotz and surkov might have opinions.

I suppose that WillRefresh on a child document doesn't guarantee us that parent is in "good" state.

Boris, as you probably know we listen for refresh driver's WillRefresh on each document to update accessible tree, at that point we suppose that layout is in good state to traverse frame tree and query frame info. Can we assume that if WillRefresh triggered on a child document, then layout tree of a parent document is in 'good' state to force a11y tree update on it?

Samuel, if the answer is no ^, then we need to come with something different, i.e. flush the queue on a child only if all parents queues are empty. That theoretically could make the child processing hanging forever, but shoudln't really happen in the wilds.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> Can we assume that if WillRefresh triggered on a child document, then layout tree of a parent document
> is in 'good' state 

I don't know what constitutes "good" state here.  Also, WillRefresh is called on refresh driver observers in general; it's not tied to a "parent" or "child" document....

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #33)
> > Can we assume that if WillRefresh triggered on a child document, then layout tree of a parent document
> > is in 'good' state 
> 
> I don't know what constitutes "good" state here.

Honestly I don't know much about layout to give a good definition, but I tried to give some explanation above: we listen for refresh driver's WillRefresh on each document (presshell) to update accessible tree, at that point we suppose that layout is in good state to traverse frame tree and query frame info. I suppose we are not allowed to poke at layout at random time, so we collect all changes the layout notifies us about, such us content inserted/removed and text changes, and wait for WillRefresh to process them.

So the question is if WillRefresh of PresShell associated with a child document was called, then can we process all collected notifications for a parent document?

>  Also, WillRefresh is
> called on refresh driver observers in general; it's not tied to a "parent"
> or "child" document....

what does 'in general' means here? If I register for refresh driver updates by PresShell->AddRefreshObserver() for all documents in a process, then all observers will be triggered one by one and synchronously?

Comment 35

[Boris Zbarsky [:bzbarsky]]

> at that point we suppose that layout is in good state to traverse frame tree and query frame info.

It... might be.  It's not any worse at that point than at any JS entrypoint where we do this without flushing script.

> So the question is if WillRefresh of PresShell associated with a child document was called, then can we
> process all collected notifications for a parent document?

WillRefresh does not belong to a particular presshell.  So the answer is "it's as safe as WillRefresh that you think is for the parent presshell".

> If I register for refresh driver updates by PresShell->AddRefreshObserver() for all documents in a process

Then what will happen is that you will get multiple calls to your observer when you perhaps don't need to.

Specifically, subframes share a refresh driver with their parents.  PresShell->AddRefreshObserver() just adds the observer to the refresh driver.  So if you do this for both a subframe and the parent, your observer will be added to the refresh driver's array of observers twice.  When the refresh driver ticks, it will call WillRefresh() on everything in that array before doing the corresponding flush type (style, layout, etc).  So in this hypothetical case your one refresh observer will get two WillRefresh calls and after that both presshells will get the corresponding flush.  Make sense?

Comment 36

[Samuel Thibault]

Attachment: patch

This version fixes indentation of the added code.

Comment 37

[Samuel Thibault]

Attachment: patch2

This version reduces the parent/child willrefresh dependency to only chrome vs document, to be less intrusive.

Comment 38

[Samuel Thibault]

Attachment: patch2-2

Hello,

I have given a try at making the document's WillRefresh just return if chrome still has pending contentinsertions, here is the patch.

It works for fixing the original issue, and does not seem to have bad effects.

On the contrary, after longer testing, the previous attempt (making the document WillRefresh force a chrome WillRefresh) seems to have some side effects. We have been having issues with e.g. this page: https://sport.francetvinfo.fr/tennis/coupe-davis-2017-les-coulisses-de-la-victoire-des-bleus , sometimes on first load, sometimes on reload, it would make the chrome interface not report a11y events any more, so it seems that that approach did pose some problems.

The approach proposed here (patch2-2) seems however very safe to me, if you agree I'll write the commit logs etc. and request formal reviews.

Samuel

Comment 39

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35)

> Specifically, subframes share a refresh driver with their parents. 
> PresShell->AddRefreshObserver() just adds the observer to the refresh
> driver.  So if you do this for both a subframe and the parent, your observer
> will be added to the refresh driver's array of observers twice.  When the
> refresh driver ticks, it will call WillRefresh() on everything in that array
> before doing the corresponding flush type (style, layout, etc).  So in this
> hypothetical case your one refresh observer will get two WillRefresh calls
> and after that both presshells will get the corresponding flush.  Make sense?

Yes, thank you. Each accessible document has its own processing queue, so having WillRefresh for each document should be fine, however their order is indeterminate, which this bug is about I believe.

I'm not clear though why if parent's WillRefresh is forced from a child's WillRefresh (Samuel's first patch), then it causes some problems (comment #38). The worse outcome I would expect is the parent's WillRefresh is called twice for a refresh driver notification.

Samuel, 2nd approach looks better, I wouldn't have NativeRole() check though as it's harder to justify and introduce more conditional constructs, which is harder to test.

Also, there's a 3d approach which is to have a single WillRefresh loop, and then traverse document hierarchy and trigger their processing, this is a bigger work, but should make things easier, for example, we can get rid of code that waits for a parent document in order to attach a child document to it.

If you go with 2nd approach, then please add a comment that there's 3d way we should try out.

Comment 40

[Samuel Thibault]

Yes, it is weird that problems happen, perhaps somehow the chrome part gets overwhelmed by everything that happens on that webpage (it includes a video player). It is really easily reproducible at least (though not on my own computer, so it's difficult to debug :/)

I'd rather go with the 2nd approach for now, since the 3rd approach seems much more invasive, I'm not sure I can get something working.

Comment 41

[Samuel Thibault]

Attachment: tabswitch

Hello,

Here is a patch implementing the 2nd approach, ready for review.

Btw, the issue from comment #38 was seen with firefox 52. Perhaps it is just due to some other firefox 52 bugs which were uncovered by that approach. We had the same issue with this 2nd approach, we had issues in our firefox 52 tests uncovered by this approach, but they were fixed somewhere between firefox 52 and 57 apparently. In the end we don't see user-visible regressions in firefox 57 with this 2nd approach.

Comment 42

[Samuel Thibault]

FTR, I ran the whole testsuite with the patch applied, it brings the same results as without the patch.

Comment 43

[alexander :surkov (:asurkov)]

Comment on attachment 8935820 [details] [diff] [review]
tabswitch

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

::: firefox-esr-52.5.0esr-1.orig/accessible/base/NotificationController.cpp
@@ +610,5 @@
>  
> +
> +  DocAccessible *parentdoc = mDocument->ParentDocument();
> +  if (parentdoc && mDocument->NativeRole() == roles::DOCUMENT
> +                && parentdoc->NativeRole() == roles::CHROME_WINDOW) {

Not sure why you need these checks.

@@ +616,5 @@
> +    // event and content event.
> +    // TODO: ideally we would have a single WillRefresh loop which traverses
> +    // the document hierarchy and processes events in the proper order.
> +    NotificationController *parent = parentdoc->mNotificationController;
> +    if (parent && parent != this && parent->mContentInsertions.Count() != 0) {

I would keep in sync IsUpdatePending with this part. It'd be nice if they shared a common method, for example, WaitingForParent().

Comment 44

[Samuel Thibault]

Attachment: patch

I believe I have found why we were having issues with the previous patches. Commit c2aeece5eb10 introduced a hash table layer, and the ScheduleContentInsertions function would always add a to the hash table, even if the added list is empty, in which case it will not schedule a refresh, and thus the child will wait for the parent to process the empty list, but the parent won't since it didn't schedule a refresh.

A quick-n-dirty workaround confirmed to work is the attached patch, a better fix will be to use RemoveAndForget when the list is empty, which I'll do.  I'm just wondering already: my original patch proposal is not the only place where mContentInsertions.Count() is used as an indication whether there are events to process, so perhaps it makes sense to make this fix a separate patch?

Comment 45

[Samuel Thibault]

Attachment: tabswitch

Hello,

Here is a reworked patch, which drops the indeed-useless test, includes the requested factorization, and includes the fix for the various issues we had been having with previous patches. We have no known issue with this and the testsuite goes fine.

Comment 46

[alexander :surkov (:asurkov)]

Comment on attachment 8936794 [details] [diff] [review]
tabswitch

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

r=me with comments addressed/fixed. thank you for the work!

::: accessible/base/NotificationController.cpp
@@ +438,5 @@
> +  if (list->Length() == 0) {
> +    // We added an array to the hash for nothing, remove it because below we
> +    // will not schedule processing (as an optimization), and leaving an empty
> +    // array in the hash would let children think we have work to do, but we
> +    // would actually not process this empty array in the close future.

good finding!

instead of modifying the hash which may go at cost of hash rebuilding, I would store all inserted elements in separate array and make LookupOrAdd() in the end if needsProcessing is true.

@@ +478,5 @@
>  
> +bool
> +NotificationController::WaitingForParent()
> +{
> +  DocAccessible *parentdoc = mDocument->ParentDocument();

nit: type* name

@@ +641,5 @@
>    }
>  
> +  if (WaitingForParent()) {
> +    // Wait for parent's notifications, to get proper ordering between e.g. tab
> +    // event and content event.

would be good to put this comment before if () {} statement

::: accessible/base/NotificationController.h
@@ +274,5 @@
>     */
>    bool IsUpdatePending();
>  
> +  /**
> +   * Return true if we should wait for processing from the parent.

maybe add: before we can process our own queue?

Comment 47

[Samuel Thibault]

> I would store all inserted elements in separate array and make LookupOrAdd() in the end if needsProcessing is true.

But there might already be some existing array in the hash, so we'd have to merge them.

Perhaps rather use Get, if it returns nullptr create a new array, and LookupOrAdd it only if we did create it and added elements to it?

Comment 48

[Samuel Thibault]

Attachment: tabswitch

I'm assuming that you are fine with that approach (since it avoids any kind of spurious work except new/delete of the empty list, which we want anyway). Here is the reworked patch, I believe it's ready for checkin-needed.

Comment 49

[Samuel Thibault]

Attachment: tabswitch

Sorry, I had misplaced the r=surkov tag, now it's in the right place.

Comment 50

[alexander :surkov (:asurkov)]

Comment on attachment 8936833 [details] [diff] [review]
tabswitch

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

::: accessible/base/NotificationController.cpp
@@ +452,5 @@
> +    } else {
> +      mContentInsertions.Put(aContainer, list);
> +    }
> +  }
> +

I would do that this way:

nsTArray<<nsCOMPtr<nsIContent>> list;
while() {
  if (insertedEl) {
    list->AppendElement(insertedEl);
    has_changes = true;
  }
}

if (has_changes) {
  mContentInsertions.LookupOrAdd(container)->AppendElements(list);
}

looks ok?

Comment 51

[Samuel Thibault]

Well, it's more expensive than needed: AppendElements has to copy from one array to the other.

Comment 52

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #51)
> Well, it's more expensive than needed: AppendElements has to copy from one
> array to the other.

It is, but it has to be very fast, your version looks complicated and harder for understanding, while, agreed, it may be more efficient. What do you think?

Comment 53

[Samuel Thibault]

Attachment: tabswitch

Right. When it's a question of balance between code complexity and efficiency, it's up to the maintainer to decide :)

Comment 54

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #53)
> Created attachment 8936893 [details] [diff] [review]
> tabswitch
> 
> Right. When it's a question of balance between code complexity and
> efficiency, it's up to the maintainer to decide :)

I would prefer mine one if you don't mind :) I don't think this code will ever be a bottleneck.

Comment 55

[Samuel Thibault]

Sure, that's what I uploaded.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e8486ad4c5
Make document events wait for chrome content insertion events r=surkov

Comment 57

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/67e8486ad4c5

Comment 58

[Wayne Mery (:wsmwk)]

(In reply to Samuel Thibault from comment #30)

...

Joanmarie, perhaps you can test them to make sure that Orca now gets the proper ordering?

presumable someone has already tested this