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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Lynn Monsanto, Assigned: Evan Yan)

Tracking

({access})

Trunk
x86
Linux
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

7.49 KB, patch
Aaron Leventhal
: review+
Ginn Chen
: review+
Details | Diff | Splinter Review
9.25 KB, patch
Aaron Leventhal
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
Keywords: access
(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?
(Assignee)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

12 years ago
Summary: tree-table accessibility information is incorrect → address book tree-table accessibility information is incorrect
(Reporter)

Comment 2

12 years ago
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 ginn.chen@sun.com for more information.
(Assignee)

Comment 3

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

Comment 4

12 years ago
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.
(Assignee)

Updated

12 years ago
Assignee: mscott → Evan.Yan
(Assignee)

Updated

12 years ago
Component: Address Book → Disability Access APIs
Product: Thunderbird → Core
Version: unspecified → Trunk
(Assignee)

Comment 5

12 years ago
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
(Assignee)

Comment 6

12 years ago
Created attachment 245192 [details] [diff] [review]
patch

1). create accessible object for invisible XUL node;
2). return INVISIBLE state for invisible column header.
Attachment #245192 - Flags: review?(aaronleventhal)
(Assignee)

Updated

12 years ago
Blocks: 333492
(Assignee)

Updated

12 years ago
Summary: address book tree-table accessibility information is incorrect → invisible column headers of xul tree-table are not exposed
No need for the address book QA contact still to be on this bug...
QA Contact: address-book → accessibility-apis

Comment 8

12 years ago
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.
Attachment #245192 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 9

12 years ago
(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

12 years ago
(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. 

(Assignee)

Comment 11

12 years ago
Created attachment 246779 [details] [diff] [review]
patch v2

deal with xul table column header particularly, for Orca team still want them be exposed.
Attachment #245192 - Attachment is obsolete: true
Attachment #246779 - Flags: review?(aaronleventhal)

Comment 12

12 years ago
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.
Attachment #246779 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 13

12 years ago
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

12 years ago
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

12 years ago
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.
(Assignee)

Comment 16

12 years ago
Created attachment 247514 [details] [diff] [review]
patch v3

Skip table cells in hidden columns.
Attachment #246779 - Attachment is obsolete: true
Attachment #247514 - Flags: review?(aaronleventhal)

Comment 17

12 years ago
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);
Attachment #247514 - Flags: review?(ginn.chen)
Attachment #247514 - Flags: review?(aaronleventhal)
Attachment #247514 - Flags: review+

Comment 18

12 years ago
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

12 years ago
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?
Attachment #247514 - Flags: review?(ginn.chen) → review+

Comment 20

12 years ago
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.

Updated

12 years ago
Blocks: 360297

Comment 21

12 years ago
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

12 years ago
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);
  }
(Assignee)

Comment 23

12 years ago
Created attachment 247802 [details] [diff] [review]
patch v4

addressing Aaron's comments.
Attachment #247802 - Flags: review?(aaronleventhal)

Updated

12 years ago
Attachment #247802 - Flags: review?(aaronleventhal) → review+

Comment 24

12 years ago
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
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 25

12 years ago
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. 
Severity: major → critical
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

12 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Summary: invisible column headers of xul tree-table are not exposed → Don't expose invisible table cell of xul tree-table
(Reporter)

Comment 27

12 years ago
(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.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 345481
You need to log in before you can comment on or make changes to this bug.