356347 - Don't expose invisible table cell of xul tree-table

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Lynn Monsanto]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060921 Ubuntu/dapper-security Firefox/1.5.0.7
Build Identifier: version 3 alpha 1 (20061010)

The accessibility information provided by the address book tree-table is incorrect. The table header count is the number of visible table headers, but the number of table cells returned is all the table cells, not just the ones for columns with visible headers. The table header information should include all the columns (both visible and not-visible). The accessible state should indicate whether the headers and columns are visible or not.

Reproducible: Always

Steps to Reproduce:
1.Run thunderbird and open the address book
2.Run at-poke and expand the address book subtree.
3.Notice that the tree-table header list only includes the visible table headers.

Actual Results:  
Only visible header information is returned

Expected Results:  
Informtion about all table headers should be returned

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> for columns with visible headers. The table header information should include
> all the columns (both visible and not-visible). The accessible state should
> indicate whether the headers and columns are visible or not.

By the address book subtree do you mean the left-hand pane, or the right-hand results pane?

Does the main mail window in thunderbird exhibit the same problems?

Comment 2

[Lynn Monsanto]

The bug refers to the right-hand tree-table. The message-header tree-table has a similar problem.  See bug 356045.  This is an accessibility bug. Please contact [e-mail] for more information.

Comment 3

[Evan Yan]

if we expose the invisible table headers, would Orca read the invisible table cells? and is that expected?

Comment 4

[Willie Walker]

In GAIL, invisble things in the hierarchy are exposed without the SHOWING/VISIBLE states set and Orca uses those states as an indicator of whether or not to present information about the object.

Comment 5

[Evan Yan]

The cause of this bug:

nsAccessibilityService::GetAccessible() returns ERROR here when the domNode is invisible.
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#1216

Comment 6

[Evan Yan]

Attachment: patch

1). create accessible object for invisible XUL node;
2). return INVISIBLE state for invisible column header.

Comment 7

[Mark Banner (:standard8)]

No need for the address book QA contact still to be on this bug...

Comment 8

[Aaron Leventhal]

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

Currently we simply don't expose hidden objects in the nsIAccessible tree. It would take a lot of work to change that.

The real problem here is possibly that we're exposing the wrong table information. We should treat the tree table as if it has the same number of columns are are currently visible.

I would be open to the solution of exposing the column headers as invisible, but then the invisible cells also need to be exposed with that state. Also, don't make such a big change for all of XUL. Your nsAccessibilityService change makes it so all visibility:hidden  XUL objects will now be exposed.

Comment 9

[Evan Yan]

(In reply to comment #8)
> Currently we simply don't expose hidden objects in the nsIAccessible tree. It
> would take a lot of work to change that.
> 
should we expose invisible xul tree-table columns? according to comment #4, GAIL did.
I mean, do you think this bug is INVALID?

> The real problem here is possibly that we're exposing the wrong table
> information. We should treat the tree table as if it has the same number of
> columns are are currently visible.
> 
The current situation is, we only expose visible column headers while expose all the table cells(both of visible and invisible)

> I would be open to the solution of exposing the column headers as invisible,
> but then the invisible cells also need to be exposed with that state.

Agree. if the invisible cells are not exposed with that state, that would be a bug.

> Also, don't make such a big change for all of XUL. Your nsAccessibilityService change
> makes it so all visibility:hidden  XUL objects will now be exposed.
> 
so we need to write some code in nsAccessibilityService::GetAccessibleFor() to treat xul column header particularly, right?

Comment 10

[Aaron Leventhal]

(In reply to comment #9)
> so we need to write some code in nsAccessibilityService::GetAccessibleFor() to
> treat xul column header particularly, right?
Yes, either that or just don't expose invisible columns.

Even though GAIL does that, we tend not to expose invisible things, so maybe it's better if we're consistent about it. You can ask Will Walker if it matters to him. 

Comment 11

[Evan Yan]

Attachment: patch v2

deal with xul table column header particularly, for Orca team still want them be exposed.

Comment 12

[Aaron Leventhal]

Comment on attachment 246779 [details] [diff] [review]
patch v2

Evan, I spoke with Will Walker and he agrees -- we should not special-case tree column headers. In Mozilla, if something is programmatically hidden, we don't expose it. If we're going to change that we'll do it for everything, but only after it's proven that we need to change that, and not in this bug.

So, it's apparently not the tree column headers that are exposed wrong, but the body of the table which needs to match the tree column headers.

Let me know if that creates too much work, but I'd like to stay consistent instead of treating tree column headers differently.

Comment 13

[Evan Yan]

Will & Aaron,

I agree that it would be better to stay consistent, if Orca team can live with it.

So we're going to hide invisible table cell? or just expose them without SHOWING state?
I prefer the latter, which we don't need to change a lot of code.

Will, please give input from Orca team.

Thanks.

Comment 14

[Willie Walker]

I've tried several times to post a reply to this bug, but Firefox kept crashing on me (well...not crashing, but consuming all of my CPU and RAM, which causes a 10 minute or so period of time where I need to work on killing the errant process - it often causes me to forget what I was working on).  Seems as though I managed to forget I was editing a reply.  I'll try again.

I just want to add a couple points of clarification about what "hidden" means.  In some cases, such as a scroll pane, and object might be scrolled out of view.  In this case, these objects really aren't 'hidden' and Orca would still like to be able to get to those objects.  My mistake in responding to this bug report was that this is what people were referring to.

In other cases, the program might be doing something as honoring a user preference to completely hide an entire column of a table.  In this case, Orca doesn't really want to see the information.  Not that in this case, the table implementation should be accurate - the number of rows/cols (and their headers) should be the number of rows/cols that are not programmatically hidden and the indeces of the rows/cols (and their headers) should be consistent with what is programmatically not hidden.  That is, as far as Orca and the at-spi are concerned, there should be no notion that a programmatically hidden object even exists.

Comment 15

[Aaron Leventhal]

Evan, so if the table cell is programmatically hidden (not scrolled away), it should not even be exposed. Let me know if the amount of programming effort is extremely large to fix that.

Comment 16

[Evan Yan]

Attachment: patch v3

Skip table cells in hidden columns.

Comment 17

[Aaron Leventhal]

Comment on attachment 247514 [details] [diff] [review]
patch v3

+  if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::hidden,
+                           nsAccessibilityAtoms::_true, eCaseMatters)) {
+    return true;
+  }
+  return false;
+}
That can be simplified to:
return content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::hidden,
+                           nsAccessibilityAtoms::_true, eCaseMatters);

Comment 18

[Aaron Leventhal]

One more thing. I don't think the static method needs to be inline. That won't get you much performance for the code size increase.

As a C++ style thing, I think most of our static methods are still done as class methods. But I'm not really sure if that makes any difference.

Comment 19

[Ginn Chen]

Comment on attachment 247514 [details] [diff] [review]
patch v3

I think member function or local function are both OK.
I think there's no difference in performance.

Yes, these functions are too long to be inlined.

Aaron, which style do you prefer?

Comment 20

[Aaron Leventhal]

For some reason I like member functions. But, I don't have a good reason that I can think of for that. So, it doesn't matter really.

Comment 21

[Aaron Leventhal]

Actually we're going to need GetFirstVisibleColumn() to be a public static method.

We need access to it in order to fix bug 360297.

That's one reason to make the static methods into members. It's easier to make them public later when you need access to them.

Comment 22

[Aaron Leventhal]

Actually we need access to GetNextVisibleColumn() as well.

Could you wrap this into a new GetNextVisibleColumn() public static method? You repeat this code twice anyway, so the reuse will help your code be smaller as well.
  // Skip hidden columns.
  while (column && IsColumnHidden(column)) {
    nsCOMPtr<nsITreeColumn> tempColumn;
    column->GetNext(getter_AddRefs(tempColumn));
    column.swap(tempColumn);
  }

Comment 23

[Evan Yan]

Attachment: patch v4

addressing Aaron's comments.

Comment 24

[Alfred Peng]

Checking in accessible/src/xul/nsXULTreeAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.h,v  <--  nsXULTreeAccessible.h
new revision: 1.21; previous revision: 1.20
done
Checking in accessible/src/xul/nsXULTreeAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.cpp,v  <--  nsXULTreeAccessible.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in accessible/src/atk/nsXULTreeAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsXULTreeAccessibleWrap.cpp,v  <--  nsXULTreeAccessibleWrap.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in accessible/src/base/nsAccessibilityAtomList.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v  <--  nsAccessibilityAtomList.h
new revision: 1.48; previous revision: 1.47
done

=> FIXED

Comment 25

[Lynn Monsanto]

I just downloaded version 3 alpha 1 (20061211) from trunk. Neither Mike Pedersen nor I can hear any change in behavior when arrowing down through the mail header entries.  AT-POKE continues to indicate the the mail header only contains 3 columns (subject, sender and date).  This ignores all the column headers that are not showing, and all the "Click to" column headers that *are* displayed. This problem continues to make Thunderbird 3 unusable with assistive technologies. 

Comment 26

[Evan Yan]

Lynn,
please note that, according to comment #14 from Will, we are not going to expose the invisible column headers, but to hide the invisible table cell. That's how we make the table column and table cell consistent.

Comment 27

[Lynn Monsanto]

(In reply to comment #26)
> Lynn,
> please note that, according to comment #14 from Will, we are not going to
> expose the invisible column headers, but to hide the invisible table cell.
> That's how we make the table column and table cell consistent.
> 

I agree with you that you hide the invisible table cells. There is an additional problem, which I described in comment #25. I filed a new bug (363646) for the additional problem.