Tree Tables in Thunderbird and Firefox broken since March 6

VERIFIED FIXED in mozilla1.9

Status

()

Core
Disability Access APIs
P2
major
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: MarcoZ, Assigned: Evan Yan)

Tracking

({access, regression})

Trunk
mozilla1.9
x86
Linux
access, regression
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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?

Comment 1

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

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

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

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

Comment 5

10 years ago
Created attachment 308573 [details] [diff] [review]
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.
Attachment #308573 - Flags: review?(Evan.Yan)

Comment 6

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

10 years ago
Created attachment 308574 [details] [diff] [review]
patch2

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

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2

Updated

10 years ago
Attachment #308574 - Flags: review?(ginn.chen)

Updated

10 years ago
Attachment #308574 - Flags: review?(ginn.chen) → review+

Updated

10 years ago
Attachment #308574 - Flags: review?(Evan.Yan)

Comment 9

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

Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

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

Comment 11

10 years ago
Created attachment 314250 [details] [diff] [review]
a different fix

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)

Comment 12

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

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

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

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

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

Comment 19

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

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

Comment 21

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

Comment 22

10 years ago
Created attachment 314326 [details] [diff] [review]
don't change GetNextSibling()
Attachment #314250 - Attachment is obsolete: true
Attachment #314326 - Flags: review?(surkov.alexander)
Attachment #314250 - Flags: review?(surkov.alexander)

Comment 23

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

Comment 24

10 years ago
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
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 25

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