aria-owns should change the accessible tree

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug, {access})

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43+ wontfix, firefox44+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

4 years ago
I lean towards to take IE's approach, to insert aria-owned content under parent since it's getting too expensive to add hooks to make aria-owns honored by other code.

Some examples to demo the problem of current approach:
menu events - bug 933322
active descendant - bug 582024
group attributes - bug  499917
complex grids - http://nohuhu.org/aria-locking-grid2.html

A bad thing about aria-owns is it defines parent-child relation only and it doesn't define sibling relations so I cannot think of a better way than to append aria-owned elements into children. 

Evaluation of this idea that makes sense to me is to respect the order in aria-owns list what means if aria-owns to point to its own children then we can change their order in the accessible tree. That would make aria-owns more flexible than it's now. That would contradict to the spec [1] though:
"Authors SHOULD NOT use aria-owns as a replacement for the DOM hierarchy. If the relationship is represented in the DOM, do not use aria-owns." So we might want to drop the idea for now.

[1] http://www.w3.org/WAI/PF/aria/states_and_properties#aria-owns

Cons/pros? Thoughts?

Comment 1

4 years ago
I would say that the present wording in the spec that you quoted is somewhat ambiguous. I read it as follows: In cases when parent elements encapsulate children elements, aria-owns should not be used. In cases when structurally child elements are outside of the parent element hierarchy, aria-own should be used.

However the spec falls short in mixed cases. Consider the complex grids example above. On the left side of the grid, role="row" element contains role="gridcell" elements, and parent-child relationship is clearly established. However the same row should also contain gridcells in the other part of the grid, and these are outside of the DOM hierarchy for the row.

In that case, does aria-owns attribute add to the relationship, or replace it altogether? If aria-owned children are added, do they follow DOM children, or precede them? In what order? What if a parent need to own elements that are outside its DOM hierarchy but precede it in the document flow? What if a parent need to own elements both inside and outside its DOM hierarchy, both preceding and succeeding it in the document flow?

I have experimented with both "add" and "replace" approaches, and neither of them seem to work with the current Firefox implementation; I see that as an opportunity to come up with a sensible solution. As a JavaScript developer I would rather prefer the "replace" approach since it would help with establishing both relationship and child order with no possibility of misinterpretation.
In general, I do think aria-owns should affect the tree. However, I'm *very* concerned about possible loops, asymmetry and missing nodes due to edge cases or poor authoring.

I think the "replace" approach is pretty dangerous. Even if the spec is unclear, an author could believe (and validly argue) that they do not need to include DOM children in aria-owns, even if aria-owns is used for other elements. I think the best approach is probably to allow DOM children in aria-owns and use it for ordering, but to still include DOM children that are not listed in aria-owns. The only question is where. I think you could just make a decision to put unlisted DOM children before or after and be done with it. I realise this ordering thing could be difficult to implement, though.

Comment 3

4 years ago
I see what you mean by "replace" approach being dangerous; indeed it can be. However your proposal seems to introduce a third, "inclusive" option. That could be a very valid approach for us, and I think it would solve the problem I have at hand. In my case (complex grid) it does not really matter if aria-owns replaces DOM relationship or includes it as long as it allows establishing child order for accessible interface, which is of utter importance.

As for the ordering of DOM children that are not listed in aria-owns, to me it feels more natural for the explicit list of ids in the aria-owns attribute to have priority over unlisted DOM children. I would therefore expect unlisted DOM children to be added after aria-owns in their natural DOM order. This way it may be easier to implement, too.

Another option altogether would be to introduce a new, less ambiguous attribute for establishing ARIA relationships: aria-ownedby, pointing from the child to the parent. Since it could only contain one id there would be no possibility of ordering, which would follow the DOM order naturally; also there would be no implication of relationship exclusivity, and therefore no issue with it.

As a side benefit, aria-ownedby would simplify the JavaScript implementation *immensely*. I would only need to generate a parent id upfront and stamp it into each child node as I render them, instead of jumping through the hoops and collecting children ids to stamp into parent after the whole kalamazoo has been rendered into the DOM.

There is but a tiny problem with aria-ownedby: it's not in the spec, and therefore is not implemented in any browser. *sigh* I shall submit a comment to W3C ARIA working group shortly.
Assignee

Comment 4

4 years ago
aria-owns is not way perfect but this bug is probably not right place to discuss the spec. A reasonable way to implement 'parent-child' relation of aria-owns is to append (move) those children under the parent I think. At least that's how we've got usual to treat aria-owns. I agree we have to be very careful about looping. Not sure if we can do anything for author errors.
(In reply to Alex Tokarev from comment #3)
> As for the ordering of DOM children that are not listed in aria-owns, to me
> it feels more natural for the explicit list of ids in the aria-owns
> attribute to have priority over unlisted DOM children. I would therefore
> expect unlisted DOM children to be added after aria-owns in their natural
> DOM order.
Makes sense to me.
> This way it may be easier to implement, too.
I think the complexity is more the fact that a DOM child can be listed or unlisted. If it's listed, you need to preserve its order in aria-owns, but additionally exclude it from the unlisted DOM children later. That could be tricky depending on how things are implemented. Anyway, I'll leave it to Surkov to comment on that. :)

> Another option altogether would be to introduce a new, less ambiguous
> attribute for establishing ARIA relationships: aria-ownedby, pointing from
> the child to the parent. Since it could only contain one id there would be
> no possibility of ordering, which would follow the DOM order naturally
If you want ot be able to override parent relationships with aria-owns, might someone also want to override children order?
Assignee

Comment 6

4 years ago
(In reply to James Teh [:Jamie] from comment #5)
> (In reply to Alex Tokarev from comment #3)
> > As for the ordering of DOM children that are not listed in aria-owns, to me
> > it feels more natural for the explicit list of ids in the aria-owns
> > attribute to have priority over unlisted DOM children. I would therefore
> > expect unlisted DOM children to be added after aria-owns in their natural
> > DOM order.
> Makes sense to me.

> > This way it may be easier to implement, too.
> I think the complexity is more the fact that a DOM child can be listed or
> unlisted. If it's listed, you need to preserve its order in aria-owns, but
> additionally exclude it from the unlisted DOM children later. That could be
> tricky depending on how things are implemented. Anyway, I'll leave it to
> Surkov to comment on that. :)

I'm good to implement anything you think is reasonable :) But for me it feels that appending and prepending of aria-owns nodes is quite symmetrical. Currently we treat aria-owns nodes as they were appended.

I'm ok with the idea that aria-owns overrides children order, because I think 'parent-child' relation has to come with 'in-siblings' relation, otherwise the feature is not complete. aria-owns overriding the ordering suits well for that imo. However I read the spec that it doesn't allow that, Alex Tokarev seems thinking otherwise. Anyway the spec doesn't address it explicitly and it's good to move the discussion to WAI-ARIA group.
Assignee

Comment 7

4 years ago
I filed the suggestion at ARIA group [1]

[1] https://lists.w3.org/Archives/Public/public-pfwg/2015Feb/0034.html

Comment 8

4 years ago
(In reply to James Teh [:Jamie] from comment #5)

> I think the complexity is more the fact that a DOM child can be listed or
> unlisted. If it's listed, you need to preserve its order in aria-owns, but
> additionally exclude it from the unlisted DOM children later. That could be
> tricky depending on how things are implemented. Anyway, I'll leave it to
> Surkov to comment on that. :)

I'm not really acquainted with the browser implementation details but from my point of view this shouldn't be a big issue. At some point in the document lifecycle a DOM tree walk could be performed, assigning child elements to their ARIA parents; at that point it is possible to check if the child is already listed among the children to avoid adding it more than once.

> If you want ot be able to override parent relationships with aria-owns,
> might someone also want to override children order?

Perhaps but I cannot name a use case off the top of my head. For my complex grid example above I wouldn't have to order children elements explicitly if there was a way to point them to their parent element in their natural DOM order, but unfortunately that's not the case.

The problem with aria-owns is that it is kind of broken by design: semantically it should be used *only* for establishing parent-child relationship, but providing the list of ids will also impose order on them whether you want it or not. This is by analogy with aria-labelledby and aria-describedby where order of element ids does matter and is expected.

Allowing aria-owns to specify child order including natural DOM children seems to be the path of least resistance and it does the trick in my case, but the underlying problem needs a better solution IMO.
(In reply to James Teh [:Jamie] from comment #2)
> In general, I do think aria-owns should affect the tree. However, I'm *very*
> concerned about possible loops, asymmetry and missing nodes due to edge
> cases or poor authoring.

I mostly agree with that.  If we say that it is  an author error (the spec is not clear on this, but imo that's the only reasonable thing for it to be) then its not too terrible to never get in a loop.  We need to free up some spare bits to say this accessible has already been moved because of aria-owns, but that's not theoretically terrible.  That said we'll spend O(depth of accessible in tree) deciding each candidate to be owned isn't a ancestor of the parenting accessible, but if you care about performance do something reasonable instead of using aria-owns.

> I think the "replace" approach is pretty dangerous. 

agreed, and I don't really see a reason to do it.

(In reply to James Teh [:Jamie] from comment #5)
> (In reply to Alex Tokarev from comment #3)
> > As for the ordering of DOM children that are not listed in aria-owns, to me
> > it feels more natural for the explicit list of ids in the aria-owns
> > attribute to have priority over unlisted DOM children. I would therefore
> > expect unlisted DOM children to be added after aria-owns in their natural
> > DOM order.
> Makes sense to me.

I'm not really convinced.

> > This way it may be easier to implement, too.
> I think the complexity is more the fact that a DOM child can be listed or
> unlisted. If it's listed, you need to preserve its order in aria-owns, but
> additionally exclude it from the unlisted DOM children later. That could be
> tricky depending on how things are implemented. Anyway, I'll leave it to
> Surkov to comment on that. :)

I think performance wise you want to do DOM children first.  that way aria-owns only slows down cases where its present.

(In reply to Alex Tokarev from comment #3)
> Another option altogether would be to introduce a new, less ambiguous
> attribute for establishing ARIA relationships: aria-ownedby, pointing from
> the child to the parent. Since it could only contain one id there would be
> no possibility of ordering, which would follow the DOM order naturally; also
> there would be no implication of relationship exclusivity, and therefore no
> issue with it.

implementing that would be a performance nightmare, so not happening.
Assignee

Comment 10

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> > > This way it may be easier to implement, too.
> > I think the complexity is more the fact that a DOM child can be listed or
> > unlisted. If it's listed, you need to preserve its order in aria-owns, but
> > additionally exclude it from the unlisted DOM children later. That could be
> > tricky depending on how things are implemented. Anyway, I'll leave it to
> > Surkov to comment on that. :)
> 
> I think performance wise you want to do DOM children first.  that way
> aria-owns only slows down cases where its present.

I'm not sure I follow why there's performance difference in what who goes first.

> (In reply to Alex Tokarev from comment #3)
> > Another option altogether would be to introduce a new, less ambiguous
> > attribute for establishing ARIA relationships: aria-ownedby, pointing from
> > the child to the parent. Since it could only contain one id there would be
> > no possibility of ordering, which would follow the DOM order naturally; also
> > there would be no implication of relationship exclusivity, and therefore no
> > issue with it.
> 
> implementing that would be a performance nightmare, so not happening.

that shouldn't be so bad but that would complicate things of course both on web author and browser sides

Comment 11

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> ancestor of the parenting accessible, but if you care about performance do
> something reasonable instead of using aria-owns.

Do something reasonable, like what? If there's a way to create accessible tree in my case (locking grid) without using aria-owns and in more performant fashion, I would very much like to hear about that.

> I think performance wise you want to do DOM children first.  that way
> aria-owns only slows down cases where its present.

There are other things besides performance; ease of understanding and use is among top priorities for me. Let's say I'd be very surprised if DOM elements were prepended to the list of ARIA children for a given parent, trumping the explicit aria-owns that I declared. The browser zoo out there has enough surprises already, thank you very much.

> implementing that would be a performance nightmare, so not happening.

Very authoritative. Care to elaborate on why linking children to parent should be that much more expensive than going the opposite direction?
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > > > This way it may be easier to implement, too.
> > > I think the complexity is more the fact that a DOM child can be listed or
> > > unlisted. If it's listed, you need to preserve its order in aria-owns, but
> > > additionally exclude it from the unlisted DOM children later. That could be
> > > tricky depending on how things are implemented. Anyway, I'll leave it to
> > > Surkov to comment on that. :)
> > 
> > I think performance wise you want to do DOM children first.  that way
> > aria-owns only slows down cases where its present.
> 
> I'm not sure I follow why there's performance difference in what who goes
> first.

if you do DOM first you can run through the DOM kids adding them all, then in the common case check and find out aria-owns == "" and you are done.  If you do aria kids first then you have to make sure you don't already have an accessible for a dom child before adding it.

> > (In reply to Alex Tokarev from comment #3)
> > > Another option altogether would be to introduce a new, less ambiguous
> > > attribute for establishing ARIA relationships: aria-ownedby, pointing from
> > > the child to the parent. Since it could only contain one id there would be
> > > no possibility of ordering, which would follow the DOM order naturally; also
> > > there would be no implication of relationship exclusivity, and therefore no
> > > issue with it.
> > 
> > implementing that would be a performance nightmare, so not happening.
> 
> that shouldn't be so bad but that would complicate things of course both on
> web author and browser sides

I'd say more work on every attribute change, and another hash table full of arrays of elements is pretty bad.

(In reply to Alex Tokarev from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > ancestor of the parenting accessible, but if you care about performance do
> > something reasonable instead of using aria-owns.
> 
> Do something reasonable, like what? If there's a way to create accessible
> tree in my case (locking grid) without using aria-owns and in more
> performant fashion, I would very much like to hear about that.

I don't think I have time at the moment to think about that.

> > I think performance wise you want to do DOM children first.  that way
> > aria-owns only slows down cases where its present.
> 
> There are other things besides performance; ease of understanding and use is
> among top priorities for me. Let's say I'd be very surprised if DOM elements
> were prepended to the list of ARIA children for a given parent, trumping the
> explicit aria-owns that I declared. The browser zoo out there has enough
> surprises already, thank you very much.

it wouldn't suprise me, and its been the way gecko has behaved for years, and come to think of it the fact they've come last for a long time is probably good reason its own to leave it that way.

> > implementing that would be a performance nightmare, so not happening.
> 
> Very authoritative. Care to elaborate on why linking children to parent
> should be that much more expensive than going the opposite direction?

it should be rather obvious if your building a tree from the root you rather have all the children of a node listed in one place.

Comment 13

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> if you do DOM first you can run through the DOM kids adding them all, then
> in the common case check and find out aria-owns == "" and you are done.  If
> you do aria kids first then you have to make sure you don't already have an
> accessible for a dom child before adding it.

In the most common case you can check if aria-owns == "" *before* running through the DOM children all the same. No added cost here. However if aria-owns != "" you'll have to do dupe and other checks anyway regardless of whether DOM children go first or last.

> I'd say more work on every attribute change, and another hash table full of
> arrays of elements is pretty bad.

I see that cost as deferred until after the initial rendering. Usually attribute changes will stem from user interaction, which happens rarely enough not to be a concern from my point of view.

> > Do something reasonable, like what? If there's a way to create accessible
> > tree in my case (locking grid) without using aria-owns and in more
> > performant fashion, I would very much like to hear about that.
> 
> I don't think I have time at the moment to think about that.

If you could ever find time to help me with this particularly hard problem I would really appreciate that.

> it wouldn't suprise me, and its been the way gecko has behaved for years,
> and come to think of it the fact they've come last for a long time is
> probably good reason its own to leave it that way.

As of now it's not possible to mix DOM and aria-owns children for a parent element, so one can say that browser behavior is undefined. I don't see any good reason to leave it that way.

> it should be rather obvious if your building a tree from the root you rather
> have all the children of a node listed in one place.

This rosy world picture does not account for the possibility of mixed DOM and aria-owns children. To have all the children of a node listed in one place you need to compile that list; you can just as well do that when walking the DOM tree while building it.

The cost of doing one more attribute presence check for each DOM element should be negligible enough to disregard when aria-ownedby == null; when aria-ownedby != null the cost can also be deferred until the whole tree has been built. Then loop over the array of elements with aria-ownedby attribute defined; resolve relationships; discard the array.

That's just off the top of my head. I'm sure this approach is rather naïve and does not account for something important; however it doesn't look that scary to me. What does is the possibility of having to do all this stuff in *JavaScript*.
(In reply to Alex Tokarev from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> 
> > if you do DOM first you can run through the DOM kids adding them all, then
> > in the common case check and find out aria-owns == "" and you are done.  If
> > you do aria kids first then you have to make sure you don't already have an
> > accessible for a dom child before adding it.
> 
> In the most common case you can check if aria-owns == "" *before* running
> through the DOM children all the same. No added cost here. However if
> aria-owns != "" you'll have to do dupe and other checks anyway regardless of
> whether DOM children go first or last.

no, unless you have a totally separate path for aria-owns=''  you'll need to branch for each accessible since aria-owns may point at it.

> > I'd say more work on every attribute change, and another hash table full of
> > arrays of elements is pretty bad.
> 
> I see that cost as deferred until after the initial rendering. Usually
> attribute changes will stem from user interaction, which happens rarely
> enough not to be a concern from my point of view.

the important part is *your point of view*

> > it wouldn't suprise me, and its been the way gecko has behaved for years,
> > and come to think of it the fact they've come last for a long time is
> > probably good reason its own to leave it that way.
> 
> As of now it's not possible to mix DOM and aria-owns children for a parent
> element, so one can say that browser behavior is undefined. I don't see any
> good reason to leave it that way.

I don't think that's true iirc at least some places consider both.

> > it should be rather obvious if your building a tree from the root you rather
> > have all the children of a node listed in one place.
> 
> This rosy world picture does not account for the possibility of mixed DOM
> and aria-owns children. To have all the children of a node listed in one
> place you need to compile that list; you can just as well do that when
> walking the DOM tree while building it.

ok sure the list of explicit children + a list of ids isn't quiet 1 place, but its a lot closer than needing to scan the whole tree on the off chance a random node will want to point at the current one.    Building a mapping is a waste of time when that's not necesary in the first place, and worse you can need to go back and update things if you later on find a child of a already created accessible.

> The cost of doing one more attribute presence check for each DOM element
> should be negligible enough to disregard when aria-ownedby == null; when

If you want to make that claim you need numbers.suspect its not even true for very large pages.  

We've made mistakes like supporting this kind of thing in the past, and its a safe assumption I wont accept a patch repeating it.
Assignee

Comment 15

4 years ago
I would suggest to hold off somewhat hot discussions until we have feedback from ARIA group.

Comment 16

4 years ago
Jaws has created an implementation so that the below HTML can be navigated in Virtual PC Cursor mode using gestures similar to how a table is being navigated. A bug was filed with NVDA http://community.nvda-project.org/ticket/3742. 

The flexibility as result of having aria-owns as option when trying to make tables (divs) accessible seems high. Could it be implemented for role=grid?

       <div role="grid" aria-label="Label Testing" aria-readonly="true" aria-owns="row_1 row_2 row_3">
              <div role="row" id="row_1" aria-owns="hdr_1 hdr_2 hdr_3"> 

              </div>
              <div role="row" id="row_2" aria-owns="cel_2_1 cel_2_2 cel_2_3"> 

              </div>
              <div role="row" id="row_3" aria-owns="cel_3_1 cel_3_2 cel_3_3"> 

              </div>
       </div>
       
       <div role="columnheader" aria-readonly="true" id="hdr_1">Animal</div>
       <div role="columnheader" aria-readonly="true" id="hdr_2">Car</div>
       <div role="columnheader" aria-readonly="true" id="hdr_3">Plant</div>
       
       <div role="gridcell" aria-readonly="true" id="cel_2_1" aria-describedby="hdr_1">Dog</div>
       <div role="gridcell" aria-readonly="true" id="cel_2_2" aria-describedby="hdr_2">Toyota</div>
       <div role="gridcell" aria-readonly="true" id="cel_2_3" aria-describedby="hdr_3">Tree</div>
       
       <div role="gridcell" aria-readonly="true" id="cel_3_1" aria-describedby="hdr_1">Dog</div>
       <div role="gridcell" aria-readonly="true" id="cel_3_2" aria-describedby="hdr_2">Dodge</div>
       <div role="gridcell" aria-readonly="true" id="cel_3_3" aria-describedby="hdr_3">Flower</div>

Comment 17

4 years ago
FWIW, I've implemented this in Google Chrome, you can try it now on the Canary Channel.

See this page for some demos:

http://dmazzoni-google.github.io/aria-owns-demos/

You might also be interested in the unit tests I wrote:

https://code.google.com/p/chromium/codesearch#search/&q=file:LayoutTests/accessibility/aria-owns&sq=package:chromium&type=cs

Here are some of the decisions I made. I'm happy to reconsider any of these, but I wanted to just throw something out there useful as a starting point for discussions.

* If a node has natural children and also has aria-owns, the aria-owns children come last. Totally arbitrary but seems more useful than not allowing this. I don't think authors should depend on them coming last but I do think it should be allowed to do this if you don't care about the order.
* The order of aria-owns children is significant; you can use it to reorder the natural children of a node.
* It will not allow cycles. If you do author a cycle, you shouldn't count on consistent behavior; which nodes actually end up owned by other nodes within that cycle will be arbitrary and may change from one page load to another.
* It will not allow a node to own an ancestor of itself, whether part of a cycle or not.
* It ignores a node trying to own itself.
* If there are two elements with the same id, it will own whatever document.getElementById() returns.
* If two nodes try to own the same id, it's an author errors; it's arbitrary which one wins and may change from one page load to another.

Finally:
* It keeps track of ids that are referenced by aria-owns but whose ids haven't been found in the document. When an element appears in the document with an id or an element's id changes, it does a quick hash lookup to see if any elements with aria-owns need to be updated.
* The correct notifications are fired when a node gets reparented due to dynamically changing aria-owns.

I'm sure I missed something. Bug reports welcome!
Assignee

Comment 18

4 years ago
It looks good with me. Jamie, any objections?

Comment 19

4 years ago
(In reply to Dominic Mazzoni from comment #17)
> FWIW, I've implemented this in Google Chrome, you can try it now on the
> Canary Channel.

I can confirm that the Chrome enhancements look very promising! If only we could get more browser vendors on board with that. ;)
Overall, this sounds great. The only thing I'm unsure about is aria-owns children coming *after* natural children. I initially figured aria-owns children should come first, since the author has made an explicit decision about what they want as children, but I guess you could argue the author just wanted *additional* children. I guess it's not overly important, since you can explicitly choose to specify the order of all children and that forces authors to think about the order.

From a screen reader perspective, I'm quite concerned about nodes being dynamically re-parented. The spec certainly allows for this, but I'm just not sure how we're going to handle that both in terms of implementation and UX.

That leads me to a related question, though. Dominic, are you firing reorder/textInserted/textRemoved if the order of children changes? That definitely needs to happen.
Assignee

Comment 21

4 years ago
Posted patch patch (obsolete) — Splinter Review
existing aria-owns are still here, if everything is ok then I'll remove them in followup
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #8658831 - Flags: review?(yzenevich)

Comment 22

4 years ago
Yes, in Chrome we treat this the same as if a DOM node was deleted in one place and inserted somewhere else, so reorder/textInserted/textRemoved/hide/show will all be fired.

I don't expect authors would use this to dynamically move things around much - it's more that if a page is loading dynamically we need to handle the case where the aria-owns parent appears first, where the child appears first, and where they both appear at the same time.
Assignee

Comment 23

4 years ago
Posted patch patch2 (obsolete) — Splinter Review
mochitests fixed
Attachment #8658831 - Attachment is obsolete: true
Attachment #8658831 - Flags: review?(yzenevich)
Attachment #8659250 - Flags: review?(yzenevich)
Comment on attachment 8659250 [details] [diff] [review]
patch2

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

looks good to me, thanks, hopefully i didn't miss much.

::: accessible/base/TreeWalker.cpp
@@ +78,5 @@
>      if (!parentNode || !parentNode->IsElement())
>        return nullptr;
>  
>      nsIContent* parent = parentNode->AsElement();
> +    top = mStateStack.AppendElement(ChildrenIterator(parent, mChildFilter));

PushState(parent)?

@@ +101,5 @@
> +TreeWalker::Next(ChildrenIterator* aIter, Accessible** aAccesible,
> +                 bool* aSkipSubtree)
> +{
> +  nsIContent* childEl = aIter->mDOMIter.GetNextChild();
> +  if (!aAccesible)

Nit: {}

@@ +113,5 @@
> +      mDoc->GetAccessible(childEl) :
> +      GetAccService()->GetOrCreateAccessible(childEl, mContext, aSkipSubtree);
> +
> +    if (accessible) {
> +      if (accessible->IsRepositioned()) {

Comment here would be nice.

@@ +122,5 @@
> +    }
> +    return childEl;
> +  }
> +
> +  Accessible* parent = mDoc->GetAccessible(aIter->mDOMIter.Parent());

Comment for what's happenning with aria owned subtree would be nice.

::: accessible/generic/Accessible.h
@@ +905,5 @@
> +   * Get/set repositioned bit indicating that the accessible was moved in
> +   * the accessible tree, i.e. the accessible tree structure differs from DOM.
> +   */
> +  bool IsRepositioned() const { return mStateFlags & eRepositioned; }
> +  void SetRepositioned(bool aTrue)

nit: rename aTrue to aRepositioned

::: accessible/generic/DocAccessible.cpp
@@ +1590,5 @@
> +            }
> +
> +            if (relAttr == nsGkAtoms::aria_owns) {
> +              // Dependent content cannot point to other aria-owns content or
> +              // its parent. Ignore it if so.

s/its/their

@@ +1598,5 @@
> +                nsIContent* parentEl = owner->GetContent();
> +                while (parentEl && parentEl != dependentContent) {
> +                  parentEl = parentEl->GetParent();
> +                }
> +                if (parentEl) {

I think this invalidates the case X owns Y and Y owns Z which is not invalid. We definitely will need a test case for that. At least a comment would be nice (about the graph)

@@ +1610,5 @@
> +                while (parentEl && parentEl != dependentContent) {
> +                  parentEl = parentEl->GetParent();
> +                }
> +                if (parentEl) {
> +                  isvalid = false;

Since we are not breaking here, does it mean that other ids might be ok?

@@ +2040,5 @@
> +DocAccessible::ValidateARIAOwned()
> +{
> +  for (auto it = mARIAOwnsHash.Iter(); !it.Done(); it.Next()) {
> +    nsTArray<nsIContent*>* childEls = it.UserData();
> +    for (uint32_t idx = 0; idx < childEls->Length(); idx++) {

take childEls->ElementAt(idx) out as a variable
Attachment #8659250 - Flags: review?(yzenevich) → review+
Assignee

Comment 25

4 years ago
(In reply to Yura Zenevich [:yzen] from comment #24)

> >      nsIContent* parent = parentNode->AsElement();
> > +    top = mStateStack.AppendElement(ChildrenIterator(parent, mChildFilter));
> 
> PushState(parent)?

right

> I think this invalidates the case X owns Y and Y owns Z which is not
> invalid. We definitely will need a test case for that. At least a comment
> would be nice (about the graph)

yep, I'll add XXX section

> @@ +1610,5 @@
> > +                while (parentEl && parentEl != dependentContent) {
> > +                  parentEl = parentEl->GetParent();
> > +                }
> > +                if (parentEl) {
> > +                  isvalid = false;
> 
> Since we are not breaking here, does it mean that other ids might be ok?

correct
Assignee

Comment 26

4 years ago
Posted patch patch3Splinter Review
David, f? request just in case if you spot anything wrong
Attachment #8659250 - Attachment is obsolete: true
Attachment #8660304 - Flags: feedback?(dbolter)
Comment on attachment 8660304 [details] [diff] [review]
patch3

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

Does landing this add any work for our e10s a11y project?
Assignee

Comment 28

4 years ago
(In reply to David Bolter [:davidb] from comment #27)

> Does landing this add any work for our e10s a11y project?

shouldn't be so because all tree changes are accompanied by show/hide events
Comment on attachment 8660304 [details] [diff] [review]
patch3

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

OK I can't review this but I am worried about aria-owns and the aria-hidden work creating tricky bugs at a time we're struggling to stabilize the e10s work. f+ if you can add a nice informative commit message.
Attachment #8660304 - Flags: feedback?(dbolter) → feedback+
Assignee

Comment 30

4 years ago
(In reply to David Bolter [:davidb] from comment #29)

> OK I can't review this but I am worried about aria-owns and the aria-hidden
> work creating tricky bugs at a time we're struggling to stabilize the e10s
> work.

so I am; we may have regressions, probably not e10s specific though

> f+ if you can add a nice informative commit message.

like "make aria-owns to alter the accessible tree"?
(In reply to alexander :surkov from comment #30)
> (In reply to David Bolter [:davidb] from comment #29)
> 
> > OK I can't review this but I am worried about aria-owns and the aria-hidden
> > work creating tricky bugs at a time we're struggling to stabilize the e10s
> > work.
> 
> so I am; we may have regressions, probably not e10s specific though
> 
> > f+ if you can add a nice informative commit message.
> 
> like "make aria-owns to alter the accessible tree"?

More detail is better IMO. Show/hide events etc...
Assignee

Comment 32

4 years ago
(In reply to David Bolter [:davidb] from comment #31)
> (In reply to alexander :surkov from comment #30)
> > like "make aria-owns to alter the accessible tree"?
> 
> More detail is better IMO. Show/hide events etc...

usually a bug description is supposed to give explanation of the approaches, I'm good to give any commit message you like, but I'm not sure what will make it nice though. Something like "Make aria-owns to alter the accessible tree, fire show/hide mutation events as we do for the accessible tree alterations" sounds a tautology.
Assignee

Comment 36

4 years ago
(In reply to Wes Kocher (:KWierso) from comment #34)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0577b78a0aae for a11y
> crashes:
> 
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=14080472&repo=mozilla-
> inbound

not sure why try didn't show any problems, relanded
Flags: needinfo?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/a252f3d0a8c9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee

Updated

4 years ago
Depends on: 1205318
Assignee

Updated

4 years ago
Blocks: 1205341
Assignee

Updated

4 years ago
Blocks: 1205476
Depends on: 1206107
I would like us to back this out for aurora.  The merge happened this morning, but updates aren't on yet. If we back this out, you can concentrate on fixing it in 44 nightly, and then when it tests out ok and seems stable, we can uplift your new fix. 

Kwierso can you back this out from aurora? Thanks!
Flags: needinfo?(wkocher)
Backout: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e5c15ad8894
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I believe this is how the flags should be set at this point.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Flags: needinfo?(wkocher) → needinfo?(surkov.alexander)
Resolution: --- → FIXED
Alexander, do you want to still try to get this into aurora? 
Or should I wontfix this and bug 1207375 for 43?
Flags: needinfo?(dbolter)
Assignee

Comment 42

4 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #41)
> Alexander, do you want to still try to get this into aurora? 
> Or should I wontfix this and bug 1207375 for 43?

we can target it to Firefox 44.
Flags: needinfo?(surkov.alexander)
+1, no rush.
Flags: needinfo?(dbolter)
Fixing the target milestone flag to reflect that this was backed out of 43 and will ride the trains with 44.
Target Milestone: mozilla43 → mozilla44
You need to log in before you can comment on or make changes to this bug.