Closed Bug 421922 Opened 12 years ago Closed 12 years ago

Tree Tables in Thunderbird and Firefox broken since March 6

Categories

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

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: MarcoZ, Assigned: evan.yan)

References

Details

(Keywords: access, regression)

Attachments

(2 files, 2 obsolete files)

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?
Flags: blocking1.9?
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.
(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?
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
Thank you Joanie, I can see the problem. But it shouldn't be related with mentioned bug.
Status: NEW → ASSIGNED
Summary: Tree Tables in Thunderbird and Firefox broken since fix for bug 418371 → Tree Tables in Thunderbird and Firefox broken since March 6
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #308573 - Flags: review?(Evan.Yan)
(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
Attached patch patch2Splinter Review
fixed misspeling
Attachment #308573 - Attachment is obsolete: true
Attachment #308574 - Flags: review?(Evan.Yan)
Attachment #308573 - Flags: review?(Evan.Yan)
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.
Target Milestone: mozilla1.9beta5 → mozilla1.9
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Attachment #308574 - Flags: review?(ginn.chen)
Attachment #308574 - Flags: review?(ginn.chen) → review+
Attachment #308574 - Flags: review?(Evan.Yan)
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.cpp,v  <--  nsXULTreeAccessible.cpp
new revision: 1.80; previous revision: 1.79
done

Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch a different fix (obsolete) — Splinter Review
Marco suggest it restores the Windows a11y hierarchy for xul:treetable.
Assignee: surkov.alexander → Evan.Yan
Status: REOPENED → ASSIGNED
Attachment #314250 - Flags: review?(surkov.alexander)
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 :))
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?
(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.
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.
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.
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.
(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?
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.
(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.
OK, I think I'm fine with not changing GetNextSibling() for now, since currently there is not a real case with multiple treecols.
Attachment #314250 - Attachment is obsolete: true
Attachment #314326 - Flags: review?(surkov.alexander)
Attachment #314250 - Flags: review?(surkov.alexander)
Comment on attachment 314326 [details] [diff] [review]
don't change GetNextSibling()

that's nice, r=me, go ahead and check in
Attachment #314326 - Flags: review?(surkov.alexander) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040907 Minefield/3.0pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.