Closed Bug 1289223 Opened 3 years ago Closed 3 years ago

Make EventTree process all accessibility show events

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: michael.li11702, Assigned: michael.li11702)

References

Details

Attachments

(1 file, 1 obsolete file)

EventTree should process all show events so that there's only one object firing show events, since the show events need to be in order so that the parent process in e10s can properly build the accessibility tree.
Currently, EventQueue processes some show events here: https://dxr.mozilla.org/mozilla-central/source/accessible/generic/DocAccessible.cpp#1448
EventTree is currently only processing show events for DocAccessible's invalidation list: https://dxr.mozilla.org/mozilla-central/source/accessible/generic/DocAccessible.cpp#1360
Depends on: 1272706
Blocks: 1272706
No longer depends on: 1272706
(In reply to Michael Li from comment #0)

> Currently, EventQueue processes some show events here:
> https://dxr.mozilla.org/mozilla-central/source/accessible/generic/
> DocAccessible.cpp#1448

as far as I can tell, there's no real need in these events, because as long as a screen reader gets document_loaded event, then it grabs the whole document. I'm curious if anything similar can be done for e10s.
(In reply to alexander :surkov from comment #1)
> (In reply to Michael Li from comment #0)
> 
> > Currently, EventQueue processes some show events here:
> > https://dxr.mozilla.org/mozilla-central/source/accessible/generic/
> > DocAccessible.cpp#1448
> 
> as far as I can tell, there's no real need in these events, because as long
> as a screen reader gets document_loaded event, then it grabs the whole
> document. I'm curious if anything similar can be done for e10s.

a client could choose to use the DOCUMENT_LOADED event, but I don't see any reason they should have to, and I'm not aware of any spec language saying this case is special as far as show and hide events go.  So I'd say its incorrect to not fire them there.
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> (In reply to alexander :surkov from comment #1)
> > (In reply to Michael Li from comment #0)
> > 
> > > Currently, EventQueue processes some show events here:
> > > https://dxr.mozilla.org/mozilla-central/source/accessible/generic/
> > > DocAccessible.cpp#1448
> > 
> > as far as I can tell, there's no real need in these events, because as long
> > as a screen reader gets document_loaded event, then it grabs the whole
> > document. I'm curious if anything similar can be done for e10s.
> 
> a client could choose to use the DOCUMENT_LOADED event, but I don't see any
> reason they should have to, and I'm not aware of any spec language saying
> this case is special as far as show and hide events go.  So I'd say its
> incorrect to not fire them there.

There's a lot of legacy in the screen readers word, and many stuff are not guided by any spec. If we haven't been asked for a feature, then we don't add it as a rule. So if there's no consumer wanting for these events, then it doesn't make much sense to fire them. But if you want show events that bad, then document accessible would be more correct target, rather than thousands of its children.
More patches to come in separate bugs.
Assignee: nobody → mili
Comment on attachment 8779018 [details]
Bug 1289223: Make EventTree process all accessibility show events

https://reviewboard.mozilla.org/r/70076/#review67304

::: accessible/generic/DocAccessible.cpp
(Diff revision 1)
> -  uint32_t childCount = ChildCount();
> +  ProcessChildShowEvents();
> -  for (uint32_t i = 0; i < childCount; i++) {
> -    Accessible* child = GetChildAt(i);
> -    RefPtr<AccShowEvent> event = new AccShowEvent(child);
> -  FireDelayedEvent(event);
> -  }

I'm not sure why you move the code into inline function, but otherwise it looks technically ok. However I would fire show event on a document as it's more performant, especially if you're going to extend the existing coalescence logic on each show event. I'm good if it goes in follow up though.
Attachment #8779018 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8779017 [details]
Bug 1289223: Make EventTree process all accessibility show events.

https://reviewboard.mozilla.org/r/70074/#review67306

::: accessible/base/EventTree.cpp:219
(Diff revision 1)
>    }
>  
>    mDependentEvents.Clear();
> +
> +  // Now that we're done processing the current EventTree, recurse into
> +  // processing the child's EventTree so that we process events from top down

could you remind me please why you change ordering?
(In reply to alexander :surkov from comment #7)
> Comment on attachment 8779018 [details]
> Bug 1289223: Make EventTree process all accessibility show events
> 
> https://reviewboard.mozilla.org/r/70076/#review67304
> 
> ::: accessible/generic/DocAccessible.cpp
> (Diff revision 1)
> > -  uint32_t childCount = ChildCount();
> > +  ProcessChildShowEvents();
> > -  for (uint32_t i = 0; i < childCount; i++) {
> > -    Accessible* child = GetChildAt(i);
> > -    RefPtr<AccShowEvent> event = new AccShowEvent(child);
> > -  FireDelayedEvent(event);
> > -  }
> 
> I'm not sure why you move the code into inline function, but otherwise it
> looks technically ok. However I would fire show event on a document as it's
> more performant, especially if you're going to extend the existing
> coalescence logic on each show event. I'm good if it goes in follow up
> though.

I figured making it inline would make it a bit faster, should I not make it inline?


(In reply to alexander :surkov from comment #8)
> Comment on attachment 8779017 [details]
> Make EventTree process events from top down instead of bottom up
> 
> https://reviewboard.mozilla.org/r/70074/#review67306
> 
> ::: accessible/base/EventTree.cpp:219
> (Diff revision 1)
> >    }
> >  
> >    mDependentEvents.Clear();
> > +
> > +  // Now that we're done processing the current EventTree, recurse into
> > +  // processing the child's EventTree so that we process events from top down
> 
> could you remind me please why you change ordering?

We want to process the events for the root's children first before processing the grandchildren, since if we try to show/hide the grandchildren first before the children are shown then parent thinks we're adding/hiding to an invalid root. Eventually though, the grandchildren shouldn't have show events; as for hide events, the hide events for moving grandchildren will be coalesced, but is it possible to have hide events after the document's loaded, say if a grandchild is moved after the document is loaded?
(In reply to Michael Li from comment #9)

> > I'm not sure why you move the code into inline function, but otherwise it
> > looks technically ok. However I would fire show event on a document as it's
> > more performant, especially if you're going to extend the existing
> > coalescence logic on each show event. I'm good if it goes in follow up
> > though.
> 
> I figured making it inline would make it a bit faster, should I not make it
> inline?

I meant to put the code right there without any functions.

> > > +  // Now that we're done processing the current EventTree, recurse into
> > > +  // processing the child's EventTree so that we process events from top down
> > 
> > could you remind me please why you change ordering?
> 
> We want to process the events for the root's children first before
> processing the grandchildren, since if we try to show/hide the grandchildren
> first before the children are shown then parent thinks we're adding/hiding
> to an invalid root. Eventually though, the grandchildren shouldn't have show
> events; as for hide events, the hide events for moving grandchildren will be
> coalesced, but is it possible to have hide events after the document's
> loaded, say if a grandchild is moved after the document is loaded?

Right. This probably deserves own bug and it'd be nice to have some testing. Otherwise I'd say it points out to a problem in the coalescence logic, because if subtree has been shown/hidden, then no events from its subtree are needed. I don't practically see any problems with firing events from top to bottom or vice versa, but since you wanted to introduce more coalescence, then won't that cover the problem you try to solve?
(In reply to alexander :surkov from comment #10)
> (In reply to Michael Li from comment #9)
> 
> > > I'm not sure why you move the code into inline function, but otherwise it
> > > looks technically ok. However I would fire show event on a document as it's
> > > more performant, especially if you're going to extend the existing
> > > coalescence logic on each show event. I'm good if it goes in follow up
> > > though.
> > 
> > I figured making it inline would make it a bit faster, should I not make it
> > inline?
> 
> I meant to put the code right there without any functions.

Right, I can do that, I just wanted it as a separate function since I had been testing moving it around.

> 
> > > > +  // Now that we're done processing the current EventTree, recurse into
> > > > +  // processing the child's EventTree so that we process events from top down
> > > 
> > > could you remind me please why you change ordering?
> > 
> > We want to process the events for the root's children first before
> > processing the grandchildren, since if we try to show/hide the grandchildren
> > first before the children are shown then parent thinks we're adding/hiding
> > to an invalid root. Eventually though, the grandchildren shouldn't have show
> > events; as for hide events, the hide events for moving grandchildren will be
> > coalesced, but is it possible to have hide events after the document's
> > loaded, say if a grandchild is moved after the document is loaded?
> 
> Right. This probably deserves own bug and it'd be nice to have some testing.
> Otherwise I'd say it points out to a problem in the coalescence logic,
> because if subtree has been shown/hidden, then no events from its subtree
> are needed. I don't practically see any problems with firing events from top
> to bottom or vice versa, but since you wanted to introduce more coalescence,
> then won't that cover the problem you try to solve?

Yes, I suppose when I get coalescence working then there shouldn't be a need to switch the EventTree's ordering since there won't be show/hide events for grandchildren. Just confirming though, do more show/hide events happen after the document finishes loading?
(In reply to Michael Li from comment #11)

> Yes, I suppose when I get coalescence working then there shouldn't be a need
> to switch the EventTree's ordering since there won't be show/hide events for
> grandchildren. Just confirming though, do more show/hide events happen after
> the document finishes loading?

sure, when the web page is changing, then more events are fired.
(In reply to alexander :surkov from comment #12)
> (In reply to Michael Li from comment #11)
> 
> > Yes, I suppose when I get coalescence working then there shouldn't be a need
> > to switch the EventTree's ordering since there won't be show/hide events for
> > grandchildren. Just confirming though, do more show/hide events happen after
> > the document finishes loading?
> 
> sure, when the web page is changing, then more events are fired.

Top-down vs. bottom-up might vary based on the situation then after the page loads. If a grandchild node gets moved to be a child of the root, then we'd need to hide it under its old parent before showing it under the root, which would mean we'd need to process bottom-up. I can't currently think of an instance when top-down would be needed, assuming grandchild events are coalesced properly.
Blocks: 1293380
Blocks: 1293443
(In reply to Michael Li from comment #13)

> Top-down vs. bottom-up might vary based on the situation then after the page
> loads. If a grandchild node gets moved to be a child of the root, then we'd
> need to hide it under its old parent before showing it under the root, which
> would mean we'd need to process bottom-up. I can't currently think of an
> instance when top-down would be needed, assuming grandchild events are
> coalesced properly.

if accessible is moved from root to grand children, events are not coalesced if they are in different subtrees. Something like that should make a trick:
<input id='i'><div id='a'></div>
<script>
document.getElementById('a').setAttribute('aria-owns', 'i');
</script>

you get hide for 'i' under the root, and show for 'i' under 'a' div. processing the tree from down-top will make you fire show event before hide.

anyway, this topic deserves own bug, so that you can finish with your first patch and close this bug.
Comment on attachment 8779018 [details]
Bug 1289223: Make EventTree process all accessibility show events

until you keep in your mind something specific for the 2nd reviewer, one review is enough for the accessibility module
Attachment #8779017 - Attachment is obsolete: true
Attachment #8779017 - Flags: review?(surkov.alexander)
Attachment #8779018 - Attachment is obsolete: true
Attachment #8779018 - Flags: review?(tbsaunde+mozbugs)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ac6cfd273b
Make EventTree process all accessibility show events. r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2ac6cfd273b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.