NODE_CHILD_OF/NODE_PARENT_OF relations are not always symmetrical

RESOLVED FIXED in mozilla29

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: jwei)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla29
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
we were reported an example:

<div aria-readonly="true" role="grid">
  <div role="row">
    <div role="gridcell">cell 1,1</div>
    <div role="gridcell">cell 1,2</div>
  </div>
  <div role="row">
    <div role="gridcell">cell 2,1</div>
    <div role="gridcell">cell 2,2</div>
  </div>
</div>

rows have NodeChildOf relations but grid doesn't have NodeParentOf relation. Technically in this specific case doesn't make to expose these relations at all since logical hierarchy corresponds to natural hierarchy.

Jonathan if you don't have windows machine still and you look for something then this one is good. You need to not expose those relations in this case (refer to ARIA spec to make sure you don't break anything else).
(Reporter)

Updated

4 years ago
Blocks: 475297
(Assignee)

Updated

4 years ago
Assignee: nobody → jwei
(Assignee)

Comment 1

4 years ago
Created attachment 8358578 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations.

I used IsConceptualParent as a general helper function for determining if an element's direct parent is also its logical parent so unneeded NODE_CHILD_OF relations were not added.  For removing NODE_PARENT_OF relations, I modified the ItemIterator and related helper functions, as it appears that this is the only usage of them.

Tests were updated to reflect the new behaviour.
Attachment #8358578 - Flags: feedback?(surkov.alexander)
(Reporter)

Updated

4 years ago
Attachment #8358578 - Flags: review?(trev.saunders)
(In reply to alexander :surkov from comment #0)
> we were reported an example:
> 
> <div aria-readonly="true" role="grid">
>   <div role="row">
>     <div role="gridcell">cell 1,1</div>
>     <div role="gridcell">cell 1,2</div>
>   </div>
>   <div role="row">
>     <div role="gridcell">cell 2,1</div>
>     <div role="gridcell">cell 2,2</div>
>   </div>
> </div>
> 
> rows have NodeChildOf relations but grid doesn't have NodeParentOf relation.
> Technically in this specific case doesn't make to expose these relations at
> all since logical hierarchy corresponds to natural hierarchy.

its true it doesn't make sense to expose them in this particular case, but I think its probably simpler than trying to not expose them properly.

consider the case of the tree in relations/test_general.html that uses children + aria-owns.  I would say if you expose some children as targets of NODE_PARENT_OF then you should expose all of them as targets because it seems reasonable to assume if you iterate over those targets you get all the logical kids.  Which would mean the only time you shouldn't expose the relations is when they exactly match the accessible tree.  Checking that property will probably be atleast the same order of magnitude of difficulty as just always exposing them, and I'm pretty sure some of the time it will be harder.
(Reporter)

Comment 3

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

> consider the case of the tree in relations/test_general.html that uses
> children + aria-owns.  I would say if you expose some children as targets of
> NODE_PARENT_OF then you should expose all of them as targets because it
> seems reasonable to assume if you iterate over those targets you get all the
> logical kids.

Doing this may bring us into not symmetrical relations which is probably ok. That's the case when node_parent_of returns children+aria-owns but node_child_of doesn't return parent for explicit hierarchy children. If not then it makes us to expose relations *always* even in case of trivial hierarchy (at least until we introduce some tricky checks).

So probably a simpler case would be if relations ignored an explicit hierarchy and would deal with aria-owns + tricky hierarchies (like aria-level stuff). Perhaps that breaks AT and we'd need to figure that out.
Flags: needinfo?(jamie)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8358578 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations.

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

cancelling review until we figure out desired behavior
Attachment #8358578 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 5

4 years ago
here's feedback from our partner:

"I think I would keep the mixed hierarchies separate.  That is, retrieving the MSAA children gets the structural children and the NodeParentOf/NodeChildOf only gets the aria-owns nodes.
 
I can't think of an instance where I want to do this but in the future there may be some time where I want to know which nodes are which and if they are all mixed together, there won't be a way to know which node is from which relationship."

Comment 6

4 years ago
NVDA tries NODE_CHILD_OF before accParent because we do care about logical relationships there; e.g. controls inside a grouping. Interestingly, we don't bother with NODE_PARENT_OF at all, which does make for asymmetrical navigation in some cases. I guess we'd need to merge the two sets of children if we did.

In short, I think i agree with comment 5. We can cope with either approach, but for NODE_PARENT_OF, there needs to be clear documentation as to whether we have to merge both sets of children or just use NODE_PARENT_OF.

One concern I have with these relations is the possibility of loops, but that's probably off-topic. :)
Flags: needinfo?(jamie)
I seem to remember orca / linux stuff wanting the behavior from comment 2 fwiw.
Flags: needinfo?(jdiggs)

Comment 8

4 years ago
Orca looks for child nodes only within things implementing AtkTable and only for an object which claims STATE_EXPANDED. If both of those things are the case, it first tries NODE_PARENT_OF because that gives us all the child nodes immediately. If it finds that relation present, it returns the results. If that relation is absent, it falls back on examining the rows for NODE_CHILD_OF.

Not sure if that answers the question I've been NEEDINFOed for or not, so I won't clear that flag yet.
(In reply to Joanmarie Diggs from comment #8)
> Orca looks for child nodes only within things implementing AtkTable and only
> for an object which claims STATE_EXPANDED. If both of those things are the
> case, it first tries NODE_PARENT_OF because that gives us all the child
> nodes immediately. If it finds that relation present, it returns the
> results. If that relation is absent, it falls back on examining the rows for
> NODE_CHILD_OF.

I think that's sort of an answer, but see below.

> Not sure if that answers the question I've been NEEDINFOed for or not, so I
> won't clear that flag yet.

The question was more less comment 2.  Consider http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/relations/test_general.html?force=1#283  The question is should only the logical but not actual children from aria-owns be relation targets of NODE_PARENT_OF or should all the children including the actual children be targets?
(Reporter)

Comment 10

4 years ago
so it seems everybody on the same page, to summarize:

do not expose node_child/parent_of relations on children/parents if their logical parent-child relations correspond to parent-child relations in hierarchy

that means if it's an element like grid has aria-owns then aria-owns children only are exposed via those relations, if no aria-owns then no relations; if it's an element like tree having a tricky hierarchy then AT can rely on node_child/parent_of relations entirely to navigate the hierarchy.

sounds good?
(Reporter)

Updated

4 years ago
Flags: needinfo?(jdiggs)

Comment 11

4 years ago
As I understand it, that sounds good. :)

One possibly-related issue: Anything which has STATE_MANAGES_DESCENDANTS is seen as a case where all bets are off with respect to the hierarchy. So is there any possibility of there being something which claims to manage its descendants but which has a hierarchy you have determined is not "tricky"? If so, we might wish to open a new/spin-off bug to further consider that.
(Reporter)

Comment 12

4 years ago
Jonathan, let me know if your patch goes with comment #10 (i.e. it doesn't need an update)
(Assignee)

Comment 13

4 years ago
Created attachment 8361159 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v2

Hierarchies that may have trickier structures contain relations (list, tree).  Simple structures (e.g. grid) don't have the NODE_CHILD/PARENT_OF relations.  Row to cell/gridcell relations have been removed as well.  Elements that are added to the hierarchies through aria-owns still have reported relations.
Attachment #8358578 - Attachment is obsolete: true
Attachment #8358578 - Flags: review?(trev.saunders)
Attachment #8361159 - Flags: review?(trev.saunders)
(Reporter)

Comment 14

4 years ago
Comment on attachment 8361159 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v2

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

::: accessible/src/generic/Accessible.cpp
@@ +1989,5 @@
>        if (mRoleMapEntry && (mRoleMapEntry->role == roles::OUTLINEITEM ||
>                              mRoleMapEntry->role == roles::LISTITEM ||
> +                            mRoleMapEntry->role == roles::ROW)
> +                        && groupInfo->ReportNodeRelation()) {
> +        rel.AppendTarget(groupInfo->ConceptualParent());

why don't you keep ConceptualParent null instead?
(Assignee)

Comment 15

4 years ago
(In reply to alexander :surkov from comment #14)
> Comment on attachment 8361159 [details] [diff] [review]
> Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v2
> 
> Review of attachment 8361159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/Accessible.cpp
> @@ +1989,5 @@
> >        if (mRoleMapEntry && (mRoleMapEntry->role == roles::OUTLINEITEM ||
> >                              mRoleMapEntry->role == roles::LISTITEM ||
> > +                            mRoleMapEntry->role == roles::ROW)
> > +                        && groupInfo->ReportNodeRelation()) {
> > +        rel.AppendTarget(groupInfo->ConceptualParent());
> 
> why don't you keep ConceptualParent null instead?

Originally it was so that the behaviour from before is still available, but since it looks like AccGroupInfo is only used for NODE_CHILD_OF and NODE_PARENT_OF relations, it makes sense to adapt the whole thing to the current requirements.  I'll put up a new patch with simplified changes based on ConceptualParent.
(Assignee)

Comment 16

4 years ago
Created attachment 8361252 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v3

Replaced checks by using ConceptualParent instead.
Attachment #8361159 - Attachment is obsolete: true
Attachment #8361159 - Flags: review?(trev.saunders)
(Assignee)

Updated

4 years ago
Attachment #8361252 - Flags: review?(trev.saunders)
(In reply to alexander :surkov from comment #14)
> Comment on attachment 8361159 [details] [diff] [review]
> Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v2
> 
> Review of attachment 8361159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/Accessible.cpp
> @@ +1989,5 @@
> >        if (mRoleMapEntry && (mRoleMapEntry->role == roles::OUTLINEITEM ||
> >                              mRoleMapEntry->role == roles::LISTITEM ||
> > +                            mRoleMapEntry->role == roles::ROW)
> > +                        && groupInfo->ReportNodeRelation()) {
> > +        rel.AppendTarget(groupInfo->ConceptualParent());
> 
> why don't you keep ConceptualParent null instead?

actually can't we just remove the row role check at Accessible.cpp:1990 and the coresponding row / tree_table checks from the NODE_PARENT_OF case?
(Assignee)

Comment 18

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> (In reply to alexander :surkov from comment #14)
> > Comment on attachment 8361159 [details] [diff] [review]
> > Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v2
> > 
> > Review of attachment 8361159 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/generic/Accessible.cpp
> > @@ +1989,5 @@
> > >        if (mRoleMapEntry && (mRoleMapEntry->role == roles::OUTLINEITEM ||
> > >                              mRoleMapEntry->role == roles::LISTITEM ||
> > > +                            mRoleMapEntry->role == roles::ROW)
> > > +                        && groupInfo->ReportNodeRelation()) {
> > > +        rel.AppendTarget(groupInfo->ConceptualParent());
> > 
> > why don't you keep ConceptualParent null instead?
> 
> actually can't we just remove the row role check at Accessible.cpp:1990 and
> the coresponding row / tree_table checks from the NODE_PARENT_OF case?

I don't think so.  We don't want to have relations between rows in grids, for example, but we still wanted to keep the relations for trees, which this change would remove (since we're removing relations for all rows).
(In reply to Jonathan Wei [:jwei] from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > (In reply to alexander :surkov from comment #14)
> > > Comment on attachment 8361159 [details] [diff] [review]
> > > Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v2
> > > 
> > > Review of attachment 8361159 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: accessible/src/generic/Accessible.cpp
> > > @@ +1989,5 @@
> > > >        if (mRoleMapEntry && (mRoleMapEntry->role == roles::OUTLINEITEM ||
> > > >                              mRoleMapEntry->role == roles::LISTITEM ||
> > > > +                            mRoleMapEntry->role == roles::ROW)
> > > > +                        && groupInfo->ReportNodeRelation()) {
> > > > +        rel.AppendTarget(groupInfo->ConceptualParent());
> > > 
> > > why don't you keep ConceptualParent null instead?
> > 
> > actually can't we just remove the row role check at Accessible.cpp:1990 and
> > the coresponding row / tree_table checks from the NODE_PARENT_OF case?
> 
> I don't think so.  We don't want to have relations between rows in grids,
> for example, but we still wanted to keep the relations for trees, which this
> change would remove (since we're removing relations for all rows).

afaik aria only allows role=row as a child of grids / treegrids which I think are really grid like not tree like here.

oh, bleh, xul tree grids have row children and a funny structure.  Actually though that should be fine you can just override the generic handling of the relation types for the classes that implement xul tree grids, which it looks like is already halfway done.
(Assignee)

Comment 20

4 years ago
Created attachment 8364662 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v4

Removed row NODE_*_OF processing in general and added ARIAGridRowAccessible class inheriting from AccessibleWrap with an overridden RelationByType method.  I don't think anything platform specific needed to be added, so the various wrappers just have a typedef for now.
Attachment #8361252 - Attachment is obsolete: true
Attachment #8361252 - Flags: review?(trev.saunders)
Attachment #8364662 - Flags: review?(trev.saunders)
(In reply to Jonathan Wei [:jwei] from comment #20)
> Created attachment 8364662 [details] [diff] [review]
> Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v4
> 
> Removed row NODE_*_OF processing in general and added ARIAGridRowAccessible
> class inheriting from AccessibleWrap with an overridden RelationByType
> method.  I don't think anything platform specific needed to be added, so the
> various wrappers just have a typedef for now.

whaaat? why do you need to change anything about aria grids?  same as in comment 19  I think you just need to change xul ones.
(Assignee)

Comment 22

4 years ago
It falls through when it's a grid (to the base RelationByType) and handles the rows for treegrid.  Wouldn't we still want rows called out for treegrid, since rows can be the parent of another (different levels) without also being the parent in the DOM?

With regards to the XUL treegrid row handling, I had originally thought they would be covered, but on second look, it'll likely miss some edge cases.  I'll add in logic for that.
Comment on attachment 8361252 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v3

given tree grids being crazy lets do something like this after all, sorry!

>+++ b/accessible/src/base/AccGroupInfo.cpp
>@@ -43,17 +43,17 @@ AccGroupInfo::AccGroupInfo(Accessible* a
>     if (siblingLevel < level) {
>-      mParent = sibling;
>+      mParent = ShouldReportRelations(aRole, siblingRole) ? sibling : nullptr;

given this is a sibling we know its a non standard layout and so we should set mParent right?

>   // Otherwise it can be a direct child.
>   item = aContainer->FirstChild();
>-  if (IsConceptualParent(BaseRole(item->Role()), containerRole))
>+  if (IsConceptualParent(BaseRole(item->Role()), containerRole) &&
>+      item->GetGroupInfo()->ConceptualParent())

any reason not to use ShouldReportRelation function here?

>     return item;
> 
>-  return nullptr;
>+  return NextItemTo(item);

why this change? the only time we can get here that we couldn't before is when the first child is a direct child where we expect standard layouts, so we're not going to  return anything.  I think if you undo this change you don't need to change NextItemTo at all.

>+AccGroupInfo::ShouldReportRelations(role aRole, role aParentRole)

I wonder if we couldn't merge this with IsConceptualParent()?

>+  return !IsConceptualParent(aRole, aParentRole);

for the contexts this is called in you know IsConceptualParent() will always return true right? so just return false here and assert that if you like.

>-function testRelation(aIdentifier, aRelType, aRelatedIdentifiers)
>+function testRelation(aIdentifier, aRelType, aRelatedIdentifiers, aNegCheck)

in the spirit of bool arguments are bad and to be more consistant with other test functions can you just add a second function testNotRelationTargets() ?

>+      // Relations for owned children elements shouldn't exist.
>+      testRelation("simplegrid-row1", RELATION_NODE_CHILD_OF, "simplegrid",
>+                   true);

you mean hierarcal children right?
(Reporter)

Comment 24

4 years ago
I'm a bit skeptic to introduce special accessbile for ARIA row to calculate relations because people do quite crazy things about mixing HTML and ARIA markup like

<table role="grid">
  <tr>
    <td role="cell">cell</td>
  </tr>
</table>
(In reply to alexander :surkov from comment #24)
> I'm a bit skeptic to introduce special accessbile for ARIA row to calculate
> relations because people do quite crazy things about mixing HTML and ARIA
> markup like

yeah, I didn't ever advocate that, there was just a time when I thought you could always get the conceptual parent of a row by calling Parent() , but that theory was from the time when I thought aria was the way to enchantment and happiness, now I yurn for death ;)
(Assignee)

Comment 26

4 years ago
Created attachment 8365253 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v5

Raised points were addressed.  ShouldReportRelations should probably stay separate from IsConceptualParent since (at least for now) we only care about trees and lists.
Attachment #8364662 - Attachment is obsolete: true
Attachment #8364662 - Flags: review?(trev.saunders)
Attachment #8365253 - Flags: review?(trev.saunders)
(Reporter)

Comment 27

4 years ago
Comment on attachment 8365253 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v5

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

::: accessible/src/base/AccGroupInfo.cpp
@@ +108,5 @@
>      return;
>  
>    roles::Role parentRole = parent->Role();
> +  if (IsConceptualParent(aRole, parentRole) &&
> +      ShouldReportRelations(aRole, parentRole))

why don't you merge ShouldReportRelations into IsConceptualParent?
(Assignee)

Comment 28

4 years ago
(In reply to alexander :surkov from comment #27)
> Comment on attachment 8365253 [details] [diff] [review]
> Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v5
> 
> Review of attachment 8365253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/AccGroupInfo.cpp
> @@ +108,5 @@
> >      return;
> >  
> >    roles::Role parentRole = parent->Role();
> > +  if (IsConceptualParent(aRole, parentRole) &&
> > +      ShouldReportRelations(aRole, parentRole))
> 
> why don't you merge ShouldReportRelations into IsConceptualParent?

Oops, ShouldReportRelations is a subset of IsConceptualParent, and IsConceptualParent is always called together with ShouldReportRelations.  Not sure why I didn't notice that when Trevor called it out the first time.
(Assignee)

Comment 29

4 years ago
Created attachment 8365277 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v6

Replaced IsConceptualParent completely with ShouldReportRelations.
Attachment #8365253 - Attachment is obsolete: true
Attachment #8365253 - Flags: review?(trev.saunders)
Attachment #8365277 - Flags: review?(trev.saunders)
Comment on attachment 8365277 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v6

>+AccGroupInfo::ShouldReportRelations(role aRole, role aParentRole)
> {
>+  // We only want to report general node relations for items in tree or list

nit, general is kind of funny word, hierarchy based might make more sense.

>+  // form.  ARIA level/owns relations are still reported.

s/still/always/ ?

>+  return aParentRole == roles::OUTLINE && aRole == roles::OUTLINEITEM ||
>+         aParentRole == roles::TREE_TABLE && aRole == roles::ROW ||
>+         aParentRole == roles::LIST && aRole == roles::LISTITEM;

I tend to think the if based form was nicer to read.

>+ * @param aRelatedIdentifiers  [in] identifier or array of identifiers of
>+ *                             related accessibles that shouldn't exist for this

aUnRelatedIdentifiers?

>+  if (!relation || !relation.targetsCount || !aRelatedIdentifiers) {
>+    ok(true, "No relations exist or no accessibles given.");

why is no unrelated identifiers being provied ok? might as well be strict.

I can't help think we don't have enough tests for this, but we can fix it incrementally I guess.
Attachment #8365277 - Flags: review?(trev.saunders) → review+
(Reporter)

Comment 31

4 years ago
Comment on attachment 8365277 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v6

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

::: accessible/src/base/AccGroupInfo.cpp
@@ +177,1 @@
>      return item;

I agree that ShouldReportRelations looks good as part of Accessible::RelationByType but here it looks sort of funny. Maybe keep IsConceptualParent since it seems neutral enough?
(Assignee)

Comment 32

4 years ago
Created attachment 8366724 [details] [diff] [review]
Removed unneeded NODE_CHILD_OF and NODE_PARENT_OF relations v7
Attachment #8365277 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f37258b0cf
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/22f37258b0cf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.