Closed Bug 1260598 Opened 4 years ago Closed 2 years ago

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

Categories

(Firefox :: Disability Access, enhancement, P2)

Unspecified
Linux
enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: jdiggs, Assigned: samuel.thibault, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 13 obsolete files)

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!
Gijs this probably more your area than mine?
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(jdiggs)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(jdiggs)
(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.
(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.
Flags: needinfo?(tbsaunde+mozbugs)
(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?
Flags: needinfo?(eitan)
(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.
Flags: needinfo?(eitan)
(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.
(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)
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.
(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.
Blocks: orca
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.
Priority: -- → P2
Attached patch tabswitch (obsolete) — Splinter Review
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 ./ )
Attached patch setcaretoffset-removed (obsolete) — Splinter Review
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.
I have filed this issue as a separate bug report, with backtraces etc. in https://bugzilla.mozilla.org/show_bug.cgi?id=1406043
Attachment #8913260 - Flags: a11y-review?(surkov.alexander)
Attachment #8913260 - Flags: review?(surkov.alexander)
Attached patch proposed fix (obsolete) — Splinter Review
This is the same proposed fix, only the formatting changes
Attachment #8913260 - Attachment is obsolete: true
Attachment #8913260 - Flags: review?(surkov.alexander)
Attachment #8913260 - Flags: a11y-review?(surkov.alexander)
Attachment #8917324 - Flags: review?(surkov.alexander)
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)
Flags: needinfo?(jdiggs)
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 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.
Attachment #8917324 - Flags: review?(surkov.alexander) → review?(bugs)
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.
Attachment #8917324 - Flags: review?(bugs) → review-
Hello,

What is against the HTML spec exactly?

Samuel
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
(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).
Flags: needinfo?(jdiggs)
(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... :-)
Flags: needinfo?(aklotz)
(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 :)
(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.
Flags: needinfo?(aklotz)
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?
Flags: needinfo?(aklotz)
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #8917324 - Attachment is obsolete: true
Attached patch patch2 (obsolete) — Splinter Review
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?
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?
Flags: needinfo?(jdiggs)
Flags: needinfo?(bugs)
I'm not familiar enough with a11y code to say anything about the patches.
aklotz and surkov might have opinions.
Flags: needinfo?(bugs)
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(aklotz)
> 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....
Flags: needinfo?(bzbarsky)
(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?
> 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?
Flags: needinfo?(surkov.alexander)
Attached patch patch (obsolete) — Splinter Review
This version fixes indentation of the added code.
Attachment #8929063 - Attachment is obsolete: true
Attached patch patch2 (obsolete) — Splinter Review
This version reduces the parent/child willrefresh dependency to only chrome vs document, to be less intrusive.
Attachment #8929088 - Attachment is obsolete: true
Attached patch patch2-2 (obsolete) — Splinter Review
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
(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.
Flags: needinfo?(surkov.alexander)
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.
Attached patch tabswitch (obsolete) — Splinter Review
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.
Attachment #8914851 - Attachment is obsolete: true
Attachment #8934438 - Attachment is obsolete: true
Attachment #8934439 - Attachment is obsolete: true
Attachment #8934462 - Attachment is obsolete: true
Attachment #8935820 - Flags: review?(surkov.alexander)
FTR, I ran the whole testsuite with the patch applied, it brings the same results as without the patch.
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().
Attached patch patch (obsolete) — Splinter Review
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?
Flags: needinfo?(surkov.alexander)
Attached patch tabswitch (obsolete) — Splinter Review
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.
Attachment #8935820 - Attachment is obsolete: true
Attachment #8936495 - Attachment is obsolete: true
Attachment #8935820 - Flags: review?(surkov.alexander)
Attachment #8936794 - Flags: review?(surkov.alexander)
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?
Attachment #8936794 - Flags: review?(surkov.alexander) → review+
> 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?
Attached patch tabswitch (obsolete) — Splinter Review
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.
Attachment #8936794 - Attachment is obsolete: true
Attached patch tabswitch (obsolete) — Splinter Review
Sorry, I had misplaced the r=surkov tag, now it's in the right place.
Attachment #8936831 - Attachment is obsolete: true
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?
Well, it's more expensive than needed: AppendElements has to copy from one array to the other.
(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?
Flags: needinfo?(surkov.alexander)
Attached patch tabswitchSplinter Review
Right. When it's a question of balance between code complexity and efficiency, it's up to the maintainer to decide :)
Attachment #8936833 - Attachment is obsolete: true
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
Sure, that's what I uploaded.
Assignee: nobody → samuel.thibault
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e8486ad4c5
Make document events wait for chrome content insertion events r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67e8486ad4c5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1425932
You need to log in before you can comment on or make changes to this bug.