1299957 - Coalescence is incorrect when changing aria-owns target from a parent to its child

Status: RESOLVED DUPLICATE of bug 1294853

(Core :: Disability Access APIs, defect)

Description

[Michael Li]

If you apply the patches from bug 1296420 and bug 1295603 and run tests/browser/e10s/browser_treeupdate_ariaowns.js, you'll get an "ID already in use" assertion. This is because in the second test, we change t1_container's aria-owns attribute from "t1_button t1_checkbox" (what it was originally in the html) to "t1_button t1_subdiv" here: https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/e10s/browser_treeupdate_ariaowns.js#30
i.e., t1_checkbox doesn't have aria-owns anymore, and its child t1_subdiv gets aria-owns. t1_button still has aria-owns.
After t1_button's and t1_subdiv's events are added to the EventTree, the EventTree looks like this:

reordering for: 0x7f010cb03cf0; role: section, idx: 0, element node: 0x7f010cbbd350, div@id='t1_container'
container: 0x7f010cb03cf0; role: section, idx: 0, element node: 0x7f010cbbd350, div@id='t1_container'
hidden: 0x7f010cbf18d0; role: pushbutton, idx: 0, element node: 0x7f010cbbd470, div@id='t1_button'
shown: 0x7f010cbf18d0; role: pushbutton, idx: 0, element node: 0x7f010cbbd470, div@id='t1_button'
shown: 0x7f010cbf1860; role: section, idx: 1, element node: 0x7f010cbbd7d0, div@id='t1_subdiv'
  container: 0x7f010cbf17f0; role: checkbutton, idx: 2, element node: 0x7f010cbbd590, div@id='t1_checkbox'
  hidden: 0x7f010cbf1860; role: section, idx: 1, element node: 0x7f010cbbd7d0, div@id='t1_subdiv'

Then after t1_checkbox gets processed, the tree becomes:
container: 0x7f010cb03cf0; role: section, idx: 0, element node: 0x7f010cbbd350, div@id='t1_container'
hidden: 0x7f010cbf18d0; role: pushbutton, idx: 1, element node: 0x7f010cbbd470, div@id='t1_button'
shown: 0x7f010cbf18d0; role: pushbutton, idx: 1, element node: 0x7f010cbbd470, div@id='t1_button'
shown: 0x7f010cbf1860; role: section, idx: 2, element node: 0x7f010cbbd7d0, div@id='t1_subdiv'
hidden: 0x7f010cbf17f0; role: checkbutton, idx: 0, element node: 0x7f010cbbd590, div@id='t1_checkbox'
shown: 0x7f010cbf17f0; role: checkbutton, idx: 0, element node: 0x7f010cbbd590, div@id='t1_checkbox'

t1_subdiv's hide event was incorrectly coalesced since its container matched the Accessible that we were adding a show/hide event for, t1_checkbox; this behaviour comes from here: https://dxr.mozilla.org/mozilla-central/source/accessible/base/EventTree.cpp#426-435
This hide event is needed since t1_subdiv was already shown when the document loaded, so we need to hide t1_subdiv before we show it again under t1_container; the lack of hide event causes t1_subdiv to be shown again, causing the "ID already in use assertion."

A simple solution to this could be to not add the show/hide events for moving nodes that stay under the old parent, as they're not really "moving" anywhere; this would get rid of the events for t1_button and t1_checkbox, and t1_subdiv's hide event wouldn't get coalesced in this case.

Comment 1

[Michael Li]

Alex, just wanted to make you aware of this bug. I know that the lack of hide for t1_subdiv is intended behaviour right now as shown in the mochitest here (https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_ariaowns.html#28-36), however this lack of hide for t1_subdiv is causing t1_subdiv to be shown twice without being hidden as described above, causing assertions in e10s.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Michael Li from comment #0)

> t1_subdiv's hide event was incorrectly coalesced since its container matched
> the Accessible that we were adding a show/hide event for, t1_checkbox; 

I would argue that this is correct coalescence, because 't1_subdiv' and its parent were hidden, so only hide event on the parent is required to update the tree on AT side. Even if you didn't coalesce the hide event for 't1_subdiv', then it would be delivered after its show event, and you would still had a problem. So it sounds like you deal here with ordering problem, and if a show event for 't1_subdiv' was delivered before 'checkbutton' hide event, then it would be a fix. In other words, this bug looks like another use case of bug 1294853. Sounds about right?

> A simple solution to this could be to not add the show/hide events for
> moving nodes that stay under the old parent, as they're not really "moving"
> anywhere; this would get rid of the events for t1_button and t1_checkbox,
> and t1_subdiv's hide event wouldn't get coalesced in this case.

this depends, if we deal with moving only, then I'd say hide/show and text events are needed. I think even e10s part won't update the tree properly, if no events was sent.

Comment 3

[Michael Li]

Yes that sounds right. Do you mean it would be fixed if the show for t1_subdiv comes after the hide of the checkbutton? Since the checkbutton's hide would remove t1_subdiv from the cache too. Right now the show for t1_subdiv comes before the hide for the checkbutton.