548472 - Column header accessibles are not ordered according to visual order

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100224 Minefield/3.7a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2pre) Gecko/20100224 Lanikai/3.1b1pre

In a tree table accessible (such as the message or thread list in Thunderbird), there can be a list of column headers as the first child. Although this is not yet keyboard accessible (which is another bug), it's possible to manipulate these column headers; e.g. reorder them. (This can be done in NVDA using a combination of object navigation and the mouse.) Unfortunately, when they are reordered, their new order is not reflected in accessibility, so it is very difficult to determine how the columns are currently ordered. Note that the cells in the table body *do* get reordered correctly.

Reproducible: Always

Steps to Reproduce:
1. Open a folder in Thunderbird and move to the message list.
2. Locate to the message list tree accessible and then to its first child to obtain the list of column headers. Observe the order of the column headers in the list.
3. Reorder the columns by dragging the column headers with the mouse.
4. Examine the order of the column headers in the list.
Actual Results:  
The order of the column headers after the reorder (step 4) is the same as the original order (step 2).

Expected Results:  
The order of the column headers after the reorder (step 4) should reflect the new order on screen.

Comment 1

[alexander :surkov (:asurkov)]

Jamie, would you expect an event pointing the children order has been changed, for example, reorder event on the tree accessible?

Comment 2

[James Teh [:Jamie]]

Practically speaking, we don't use such events at present in applications and don't have any plans to do so in the foreseeable future.

In theory, an event probably should be fired. I think it'd make most sense to fire it on each item, as it is the item that is changing the order of its children and not the tree itself. However, that would mean a hell of a lot of events every time you reorder columns, so I think your idea of firing it on the tree would be more realistic.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 4

[Aaron Smith]

This patch is over a year old, but the issue still exists for all screen reader issues. Did it resolve the issue? If so, can we get it put in a release build? Thanks!

Comment 5

[Marco Zehe (:MarcoZ)]

Alex, I was just pinged on this issue again by a community member.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

fire column_tree_reorder on atk, nothing on windows
let's get back when AT would need something in this case

Comment 7

[Hubert Figuiere [:hub]]

Comment on attachment 590581 [details] [diff] [review]
patch

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

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +175,5 @@
>  
>    /**
>     * Return index of the column.
>     */
> +  inline PRInt32 GetColumnIndex() const;

inline for a method that is implemented in the .cpp will be ignored.
if you really want it inline, move it from the .cpp to here.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #7)

> inline for a method that is implemented in the .cpp will be ignored.
> if you really want it inline, move it from the .cpp to here.

mm, ok, I suppose do you want new patch for that?

Comment 9

[Marco Zehe (:MarcoZ)]

Additionally, jamie filed the bug, so this is present on Windows. Why doesn't the patch deal with Windows also? I am thinking of a simple reorder event when the column headers change...

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 590581 [details] [diff] [review]
patch

>+PRInt32
>+nsCoreUtils::GetSensibleColumnIndex(nsITreeColumn *aColumn)
>+{
>+  PRInt32 index = 0;
>+  nsCOMPtr<nsITreeColumn> column = aColumn;
>+  while (column = nsCoreUtils::GetPreviousSensibleColumn(column))
>+    index++;
>+
>+  return index;
>+}

it's short, make it inlinable?

btw it looks like we might get substantially better perf from using nsTreeColumn / nsTreeColumns directly.

>+bool
>+nsXULTreeColumnsAccessible::IsDefunct() const
>+{
>+  return !mTree && nsXULColumnsAccessible::IsDefunct();

what does this get you? Shutdown() will ensure that mContent etc is also null so super class IsDefunct() should work or am I missing a way for  mTree to become NULL?  If you don't need it I'd prefer you get rid of it since I'd expect it makes it less likely  LTO  will  inline calls to IsDefunct().

>+  nsCOMPtr<nsITreeColumn> columnObj =

nit, when did we start sticking Obj on the end of things?

>+  if (treeView) {
>+    PRInt32 rowCount = 0;
>+    treeView->GetRowCount(&rowCount);
>+    if (rowCount > 0 && aOffset <= rowCount) {
>+      nsRefPtr<nsXULTreeAccessible> tree = do_QueryObject(Parent());
>+      if (tree)
>+        return tree->GetTreeItemAccessible(aOffset - 1);
>     }
>   }

nit, I'd probably prefer early returns here.

>+  } else {
>+    for (PRInt32 index = aOffset; index > 0 && columnObj; index--)
>+      columnObj = nsCoreUtils::GetNextSensibleColumn(columnObj);
>+  }
>+  if (!columnObj)

nit, blank line?

> class nsXULTreeColumnsAccessible : public nsXULColumnsAccessible
> {
> public:
>-  nsXULTreeColumnsAccessible(nsIContent *aContent, nsIWeakReference *aShell);
>+  nsXULTreeColumnsAccessible(nsIContent* aContent, nsIWeakReference* aShell);

add virtual destructor why your here?

>+  nsXULTreeColumnItemAccessible(nsIContent* aDOMNode, nsIWeakReference* aShell);

nit, aDOMNode?

Comment 11

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #8)
> (In reply to Hub Figuiere [:hub] from comment #7)
> 
> > inline for a method that is implemented in the .cpp will be ignored.
> > if you really want it inline, move it from the .cpp to here.
> 
> mm, ok, I suppose do you want new patch for that?

Without the other comments, I'd say you could fix this and commit. :-)

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #9)
> Additionally, jamie filed the bug, so this is present on Windows. Why
> doesn't the patch deal with Windows also? I am thinking of a simple reorder
> event when the column headers change...

see comment #9. Technically the things aren't so simple with simple reorder event from API point of view and from implementation point of view. I'd get back to that later when we have more data.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> >+PRInt32
> >+nsCoreUtils::GetSensibleColumnIndex(nsITreeColumn *aColumn)

> it's short, make it inlinable?

I think we could to move it into nsCoreUtils.h

> btw it looks like we might get substantially better perf from using
> nsTreeColumn / nsTreeColumns directly.

yeah, by copy/paste logic, that might be not so nice, if we're hitting perf then I'd say we need to consider something else than iterating columns.

> >+bool
> >+nsXULTreeColumnsAccessible::IsDefunct() const
> >+{
> >+  return !mTree && nsXULColumnsAccessible::IsDefunct();
> 
> what does this get you? Shutdown() will ensure that mContent etc is also
> null so super class IsDefunct() should work or am I missing a way for  mTree
> to become NULL?  If you don't need it I'd prefer you get rid of it since I'd
> expect it makes it less likely  LTO  will  inline calls to IsDefunct().

That's not super necessary. That could do a magic in case when there's no mTree at construction time, but that does a magic for XPCOM calls since internally we don't rely on defunct stuffs. I'd say that mTree initialization should happen at Init() time where we have an option to fail. That's what we should do I think.

> >+  nsCOMPtr<nsITreeColumn> columnObj =
> 
> nit, when did we start sticking Obj on the end of things?

I used the obj postfix to keep it clear that this is not a node or an accessible object.

> > class nsXULTreeColumnsAccessible : public nsXULColumnsAccessible
> > {
> > public:
> >-  nsXULTreeColumnsAccessible(nsIContent *aContent, nsIWeakReference *aShell);
> >+  nsXULTreeColumnsAccessible(nsIContent* aContent, nsIWeakReference* aShell);
> 
> add virtual destructor why your here?

yep

> >+  nsXULTreeColumnItemAccessible(nsIContent* aDOMNode, nsIWeakReference* aShell);
> 
> nit, aDOMNode?

yep

Comment 14

[Trevor Saunders (:tbsaunde)]

> 
> I think we could to move it into nsCoreUtils.h
> 
> > btw it looks like we might get substantially better perf from using
> > nsTreeColumn / nsTreeColumns directly.
> 
> yeah, by copy/paste logic, that might be not so nice, if we're hitting perf
> then I'd say we need to consider something else than iterating columns.

I don't follow this.

> > >+bool
> > >+nsXULTreeColumnsAccessible::IsDefunct() const
> > >+{
> > >+  return !mTree && nsXULColumnsAccessible::IsDefunct();
> > 
> > what does this get you? Shutdown() will ensure that mContent etc is also
> > null so super class IsDefunct() should work or am I missing a way for  mTree
> > to become NULL?  If you don't need it I'd prefer you get rid of it since I'd
> > expect it makes it less likely  LTO  will  inline calls to IsDefunct().
> 
> That's not super necessary. That could do a magic in case when there's no
> mTree at construction time, but that does a magic for XPCOM calls since
> internally we don't rely on defunct stuffs. I'd say that mTree
> initialization should happen at Init() time where we have an option to fail.
> That's what we should do I think.

moving failable stuff to Init() seems sort of reasonable, but I'm not sure I follow the rest of what you say here.

> > >+  nsCOMPtr<nsITreeColumn> columnObj =
> > 
> > nit, when did we start sticking Obj on the end of things?
> 
> I used the obj postfix to keep it clear that this is not a node or an
> accessible object.
ok

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > 
> > I think we could to move it into nsCoreUtils.h
> > 
> > > btw it looks like we might get substantially better perf from using
> > > nsTreeColumn / nsTreeColumns directly.
> > 
> > yeah, by copy/paste logic, that might be not so nice, if we're hitting perf
> > then I'd say we need to consider something else than iterating columns.
> 
> I don't follow this.

we need to have a way to traverse visible columns, so it's nicer to have helper methods for that instead of direct usage nsTreeColumn / nsTreeColumns. If you care about perf then we should consider some caching because columns traversal is much more expensive than a method call.

> > That's not super necessary. That could do a magic in case when there's no
> > mTree at construction time, but that does a magic for XPCOM calls since
> > internally we don't rely on defunct stuffs. I'd say that mTree
> > initialization should happen at Init() time where we have an option to fail.
> > That's what we should do I think.
> 
> moving failable stuff to Init() seems sort of reasonable, but I'm not sure I
> follow the rest of what you say here.

rest is why this is reasonable so you can skip it since you see why that's reasonable :)

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #11)
> (In reply to alexander :surkov from comment #8)
> > (In reply to Hub Figuiere [:hub] from comment #7)
> > 
> > > inline for a method that is implemented in the .cpp will be ignored.
> > > if you really want it inline, move it from the .cpp to here.
> > 
> > mm, ok, I suppose do you want new patch for that?
> 
> Without the other comments, I'd say you could fix this and commit. :-)

r- doesn't have commit option

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > > 
> > > I think we could to move it into nsCoreUtils.h
> > > 
> > > > btw it looks like we might get substantially better perf from using
> > > > nsTreeColumn / nsTreeColumns directly.
> > > 
> > > yeah, by copy/paste logic, that might be not so nice, if we're hitting perf
> > > then I'd say we need to consider something else than iterating columns.
> > 
> > I don't follow this.
> 
> we need to have a way to traverse visible columns, so it's nicer to have
> helper methods for that instead of direct usage nsTreeColumn /
> nsTreeColumns. If you care about perf then we should consider some caching
> because columns traversal is much more expensive than a method call.

I don't mean inline these functions where they're used, what I meant was using nsTreeColumn instead of nsITreeColumn because the concrete class has nice inline methods to do get next / prev column etc.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> I don't mean inline these functions where they're used, what I meant was
> using nsTreeColumn instead of nsITreeColumn because the concrete class has
> nice inline methods to do get next / prev column etc.

got it, thanks, I'll take a look

Comment 19

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> 
> > I don't mean inline these functions where they're used, what I meant was
> > using nsTreeColumn instead of nsITreeColumn because the concrete class has
> > nice inline methods to do get next / prev column etc.
> 
> got it, thanks, I'll take a look

I agree that'd be nicer but they don't have public methods and it sounds nsTreeColumns is nsITreeColumn oriented, I think I wouldn't want to touch these files.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 21

[Hubert Figuiere [:hub]]

Comment on attachment 593006 [details] [diff] [review]
patch2

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

r+ if you address the comment.

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +1248,5 @@
>  
>  PRInt32
>  nsXULTreeGridCellAccessible::GetColumnIndex() const
>  {
> +  return nsCoreUtils::GetSensibleColumnIndex(mColumn);

Maybe this could be inlined in the .h.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #21)
> >  PRInt32
> >  nsXULTreeGridCellAccessible::GetColumnIndex() const
> >  {
> > +  return nsCoreUtils::GetSensibleColumnIndex(mColumn);
> 
> Maybe this could be inlined in the .h.

I'm not sure if we want the header file depend on nsCoreUtils.h

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 593006 [details] [diff] [review]
patch2

additional review from Trevor since he had concenrs

Comment 24

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #22)
> (In reply to Hub Figuiere [:hub] from comment #21)
> > >  PRInt32
> > >  nsXULTreeGridCellAccessible::GetColumnIndex() const
> > >  {
> > > +  return nsCoreUtils::GetSensibleColumnIndex(mColumn);
> > 
> > Maybe this could be inlined in the .h.
> 
> I'm not sure if we want the header file depend on nsCoreUtils.h

Ok, make sense.

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #23)
> Comment on attachment 593006 [details] [diff] [review]
> patch2
> 
> additional review from Trevor since he had concenrs

yeah, but they were  r=me with comments fixed type stuff if I'd  felt like really looking at this carefully.

(In reply to alexander :surkov from comment #19)
> (In reply to alexander :surkov from comment #18)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > 
> > > I don't mean inline these functions where they're used, what I meant was
> > > using nsTreeColumn instead of nsITreeColumn because the concrete class has
> > > nice inline methods to do get next / prev column etc.
> > 
> > got it, thanks, I'll take a look
> 
> I agree that'd be nicer but they don't have public methods and it sounds
> nsTreeColumns is nsITreeColumn oriented, I think I wouldn't want to touch
> these files.

yeah, lets leave it for now and just remember if there's perf issues with this stuff in trees.  If so we can see if layout people are willing to make it or something else public.

Comment 26

[Trevor Saunders (:tbsaunde)]

Comment on attachment 593006 [details] [diff] [review]
patch2


>+  nsCOMPtr<nsITreeView> treeView;
>+  mTreeBoxObj->GetView(getter_AddRefs(treeView));
>+  if (treeView)
>+    return nsnull;

I assume this is supposed to be !treeView

>+nsXULTreeColumnItemAccessible::Init()
>+{
>+  nsCOMPtr<nsITreeBoxObject> treeBoxObj =
>+    nsCoreUtils::GetTreeBoxObject(mContent);
>+  if (treeBoxObj)
>+    treeBoxObj->GetColumns(getter_AddRefs(mColumnsObj));
>+
>+  return !!mColumnsObj;

I'm not sure !! is needed since its using a pointer as a bool not an integer.

>+  if (mColumnsObj) {

you don't need this any more do you?

>+nsXULTreeColumnItemAccessible::GetSiblingAtOffset(PRInt32 aOffset,
>+                                                  nsresult* aError) const
>+{
>+  if (!mColumnsObj) {
>+    if (aError)
>+      *aError = NS_ERROR_UNEXPECTED;

same

I don't for see any big issues here, but I haven't looked deeply into xul trees  tell me if you want me to.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #26)

> I don't for see any big issues here, but I haven't looked deeply into xul
> trees  tell me if you want me to.

that'd be really nice, the patch approach is not unambiguous

Comment 28

[[e-mail]]

Comment on attachment 593006 [details] [diff] [review]
patch2

Well, this looks vaguely reasonable, except I noticed that the existing check for a hidden column might possibly be simplified by checking for zero width.

Do you respond to the hidden attribute changing at all? If you do, is it possible to watch for the ordinal attribute changing in the same way? If you don't, then how do you deal with columns being hidden and shown?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #28)
> Comment on attachment 593006 [details] [diff] [review]
> patch2
> 
> Well, this looks vaguely reasonable, except I noticed that the existing
> check for a hidden column might possibly be simplified by checking for zero
> width.

I think it can be replaced on checking an accessible, i.e. if there's an accessible then column is not hidden.

> Do you respond to the hidden attribute changing at all?

No. I think this case falls into general approach where we listen frames creation/desctruction.

> If you do, is it
> possible to watch for the ordinal attribute changing in the same way?

We can listen ordinal attribute change. Do you think this is preferable way to TreeColsReordered DOM event? Does it mean a logic copy/sharing from nsTreeColumns in some way?

Comment 30

[alexander :surkov (:asurkov)]

alternatively I can copy the logic from nsTreeColumns::EnsureColumns() to create accessible children and put there (or into InvalidateColumns) a11y notification to rebuild children.

Comment 31

[alexander :surkov (:asurkov)]

I see we don't handle ordinal attribute at all, we should and then we should end up here with more generic solution

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 33

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.