421922 - Tree Tables in Thunderbird and Firefox broken since March 6

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

Thunderbird and Firefox builds on Linux experience problems in tree tables (such as the Messages View in Thunderbird). The regression build is March 6, 2008, and the one suspicious bugfix is bug 418371 which was checked in on March 5. Surkov, can the nsIAccessibleTable interface cause problems in tree tables? Do some methods need to be redefined for tree tables?

Comment 1

[Aaron Leventhal]

Unless a fix is found quickly, I suggest backing out bug 418371 and reopening it until a new patch fixes it without causing issues. Bug 418371 is not crucial as far as I know, -- I think xul:listbox only exists in one place in the options.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> Thunderbird and Firefox builds on Linux experience problems in tree tables
> (such as the Messages View in Thunderbird). The regression build is March 6,
> 2008, and the one suspicious bugfix is bug 418371 which was checked in on March
> 5. Surkov, can the nsIAccessibleTable interface cause problems in tree tables?
> Do some methods need to be redefined for tree tables?
> 

Marco, what is tree tables, is it xul:tree or xul:listbox? bug 418371 changed only multicolumn xul:listboxes, is problem related with them? What do "experience problems" mean?

Comment 3

[Joanmarie Diggs]

Alexander, if you look at the bookmarks "Library", the multicolumn object on the right with column headers of Name, Tags, and Location is a tree table.

Looking at it in Accerciser (I know, I know, at CSUN, mochitests, I promise :-) ), I would expect to see a hierarchy along the lines of:

-Tree Table
    - List
        - Column Header
        - Column Header
        -[...]
    - Table Cell
    - Table Cell
    - [...]

Instead I see:

-Tree Table
    - List
        - Column Header
        - Column Header
        -[...]
    - Unknown
        - Unknown
    - Unknown
        - Unknown
    - [...]

HTH

Comment 4

[alexander :surkov (:asurkov)]

Thank you Joanie, I can see the problem. But it shouldn't be related with mentioned bug.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

do not try sibling accessibles for tree table because nothing can be here excepitng treeitems of course.

I think original problem is we create accessible for treerows, it means probably our getAccessibleFor() logic has been changed.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Created an attachment (id=308573) [details]
> patch
> 
> do not try sibling accessibles for tree table because nothing can be here
> excepitng treeitems of course.

spelling/tree table/tree table columns

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

fixed misspeling

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

TM --> mozilla1.9, this won't block beta 5. If you disagree, please reset the TM to beta5 and explain why it needs to block beta.

Comment 9

[alexander :surkov (:asurkov)]

/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.cpp,v  <--  nsXULTreeAccessible.cpp
new revision: 1.80; previous revision: 1.79
done

Comment 10

[Marco Zehe (:MarcoZ)]

I'd like to take Evan's patch in attachment 314095 [details] [diff] [review] to fix this bug instead of the current one. That patch reverses the patch from this bug and adds the better fix for it (see bug 415704, and bug 424656 Comment 26.

Comment 11

[Evan Yan]

Attachment: a different fix

Marco suggest it restores the Windows a11y hierarchy for xul:treetable.

Comment 12

[alexander :surkov (:asurkov)]

Sounds like frame to content restores hierarchy but I don't think we should change NextSibling() logic in that way. Either you should add ASSERTION/FAILURE when base class method returns not null or we should override CacheChildren() like I did in linked bug.

(Btw, I don't get why we have the discussion about one problem in two bugs. It's not very comfortable :))

Comment 13

[alexander :surkov (:asurkov)]

I would take the first patch of the fix. Evan could you describe more the difference? Is it safe for those accessibles that have different frames but shares the one content?

Comment 14

[Marco Zehe (:MarcoZ)]

(In reply to comment #12)
> (Btw, I don't get why we have the discussion about one problem in two bugs.
> It's not very comfortable :))

Because there are actually 2 different problems here:
This bug regressed as a result of checkin for bug 415704.

Bug 424656 came about because of the checkin for bug 418371 I think. nsXULTree descends from nsXULSelectable, and when the table stuff got in there, trees broke once their a11y creation was restored. The two bugs, bug 415704 and 418371, happened roughly at the same time.

So, this bug here is to fix a regression caused by bug 415704, and I think Evan's fix is the better one for this bug. That's why I moved that part of the discussion here.

Comment 15

[Marco Zehe (:MarcoZ)]

I just tried applying just the first hunk of evan's patch, and that already fixes the a11y hierarchy on Windows.

And I agree with Surkov that the original patch should stay in, since it makes things more safe. So, in addition, we should take the first hunk and close this bug.

Comment 16

[Evan Yan]

I don't think we should keep the change of GetNextSibling(). There could be more than one <treecols>, so that only the next sibling of the last <treecols> is tree table cell.

Comment 17

[Evan Yan]

The difference between the current patch and the former patch is, we don't create extra accessibles.

You can check the accessible tree in DOMi, after bug 415704, we created accessibles for treerows and treechildren. If changes is made to GetNextSibling(), we still create extra accessibles, but just not expose it.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> I don't think we should keep the change of GetNextSibling(). There could be
> more than one <treecols>, so that only the next sibling of the last <treecols>
> is tree table cell.
> 

How does such tree look? How does the tree looks when it contains any foreign element?

Comment 19

[Evan Yan]

I don't know whether multiple <treecols> is ever used in Firefox/Thunderbird, but xul:tree doesn't restrict <treecols> to be only one. And I don't see a good reason to change GetNextSibling(), it has nothing to do with fixing this regression.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> I don't know whether multiple <treecols> is ever used in Firefox/Thunderbird,
> but xul:tree doesn't restrict <treecols> to be only one. And I don't see a good
> reason to change GetNextSibling(), it has nothing to do with fixing this
> regression.
> 

Since tree can contain anything then we need something complex than this. In the meantime r+ for the first patch of the bug (content->IsFocusable() -> frame->IsFocusable()). Please don't do changes in GetNextSibling because we should avoid to trash cvs history without strong reason.

Comment 21

[Evan Yan]

OK, I think I'm fine with not changing GetNextSibling() for now, since currently there is not a real case with multiple treecols.

Comment 22

[Evan Yan]

Attachment: don't change GetNextSibling()

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 314326 [details] [diff] [review]
don't change GetNextSibling()

that's nice, r=me, go ahead and check in

Comment 24

[Marco Zehe (:MarcoZ)]

Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <--  nsAccessibilityService.cpp
new revision: 1.279; previous revision: 1.278
done

Checked in for Evan because it's freeze day.

Comment 25

[Marco Zehe (:MarcoZ)]

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040907 Minefield/3.0pre.