576838 - nsIAccessibleTable is broken for crazy tables

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

<div style="display:table;" id="table6">
    <input type="checkbox">
    <a href="bar">Bad checkbox</a>
  </div>

div is table accessible and GetCellAt(0, 0) called on it returns div itself. Row and column count is 1.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: tmp patch

it prevents Orca hangs but Orca is somehow borken. Any way it's better than we have now. Later we should get real fix.

Comment 2

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-952ed0f1db93/

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 455931 [details] [diff] [review]
tmp patch

r=me.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 455931 [details] [diff] [review]
tmp patch

r=me thanks!

Comment 5

[alexander :surkov (:asurkov)]

asking for blocking since this makes Orca hang while Orca processes crazy HTML tables because firefox exposes wrong information.

Comment 6

[alexander :surkov (:asurkov)]

Bernd, could you look at this bug please? The table from bug description has one row and one column and GetCellDataAt() of nsITableLayout returns div element (a table itself) for (0, 0) cell.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 455931 [details] [diff] [review]
tmp patch

Major for Orca screen reader, trivial fix.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/24e5ac894021

Comment 9

[alexander :surkov (:asurkov)]

bug isn't fixed still, table interface is broken, the patch prevents hangs of Orca, nothing else.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> bug isn't fixed still, table interface is broken, the patch prevents hangs of
> Orca, nothing else.

We should decide whether we should keep this bug in release blocking status. Orca is still broken with this patch and patch from bug 420993 (I think because of broken nsIAccessibleTable interface). Once bug 420993 is fixed then we should ask Marco/Joanie for the help to define this bug majority.

Comment 11

[Joanmarie Diggs]

To be honest, with "Everything Else Going On," I've not yet had time to test this change. (Sorry!) However, if it stops a hang in Orca, I for one would love to see it retain it's blocking status. :-)

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Ah I just read the last few comments after filing bug 587529. Yes I agree nsIAccessibleTable appears broken in some way and I'm fine with unblocking here since we really should do deeper investigating/fixing on a separate bug IMO.

Comment 13

[alexander :surkov (:asurkov)]

Joanie, could try nightly firefox build and say how the current behavior affects on orca please?

Comment 14

[Joanmarie Diggs]

I'm still seeing some navigation issues, which I've not yet had time to dig deeper into. And I still see at least one instance of a dead accessible as I commented in bug 420993. However, things are improving. :-)

Thanks Alex!

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

OK, Fernando is going to re-triage this one to assess blocking status.

Comment 16

[Fernando Herrera]

So this is not an issue anymore with orca as we have a workaround. Maybe we should open a bug against layout because of these tables returning themselves when querying the content.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> So this is not an issue anymore with orca as we have a workaround. Maybe we
> should open a bug against layout because of these tables returning themselves
> when querying the content.

We have a wallpaper fix in a11y code so we should get back it once real problem is fixed. I think it's ok to keep this bug for both issues.

But actually we should get feedback from table layout people like Bernd.

Comment 18

[Bernd]

>But actually we should get feedback from table layout people like Bernd.
What is the question?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> >But actually we should get feedback from table layout people like Bernd.
> What is the question?

See comment #6. My concern is GetCellDataAt should never return table itself.

Comment 20

[Bernd]

For anonymous table frame creation you should not expect that  nsTableFrame::GetCellDataAt will return in side the nsIDomElement a HTMLTableCellElement

The correct text in nsHTMLTableAccessible.cpp would be

Due to anonymous table frame creation we are not guaranteed that this will be the real table cell content, it can also be the content which is responsible for the need to create a cell frame.  

instead of referencing "crazy tables"

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)

> Due to anonymous table frame creation we are not guaranteed that this will be
> the real table cell content, it can also be the content which is responsible
> for the need to create a cell frame.  

Can be the layout fixed so we are guaranteed if there's a cell frame then there's its own cell content? Since accessible tree is created based on DOM tree then we get a problem here because there's no cell accessible while it should be.

Comment 22

[Bernd]

IMHO, your requirement does not match what http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes requires and what we implement.

Comment 23

[Bernd]

you will always have cell frames with out a dom table cell object

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> IMHO, your requirement does not match what
> http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes requires and what we
> implement.

they said "anonymous boxes in visual table layout" but does it deny us to create native anonymous content for this? If Gecko allows the following:
table
  native anonymous row
   native anonymous cell
     explicit child

but iirc it doesn't allow this (XBL does but that's not a case for XBL usage I think). If that's correct then perhaps we should fix nsIContent::GetChildren() method for tables making it crawl through frame tree rather than DOM tree or think of different way of accessible tree creation.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Is there a way we can cope with comment 23 on the a11y side?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> Is there a way we can cope with comment 23 on the a11y side?

see comment #24.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Sounds like we don't know.

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

I mean we don't know the right way to go forward. (?)

Comment 29

[Bernd]

Boris will definitely know if anonymous content nodes are possible /feasible/desirable. For me the answer to all 3 options is no. But that is a half informed guess.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Anonymous content nodes corresponding to anonymous child frames would require a good bit of surgery on DOM and layout code (think wholesale changes to the way anonymous content and things like parent nodes work) and aren't really desirable, in my opinion, even if they were feasible.  I'm not at all sure they're feasible, by the way; I suspect trying to do that would be a multi-year bug-hunting project.

If accessibles need to represent pure-layout information not directly tied to content, then it seems like we need to allow accessibles to exist that map to a particular frame, even if it's not the primary frame for a node.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Hmm.  I guess we could do something "xbl like", but I'm not sure it's worth it still...

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> If accessibles need to represent pure-layout information not directly tied to
> content, then it seems like we need to allow accessibles to exist that map to a
> particular frame, even if it's not the primary frame for a node.

I agree, that's what needs to happen here.

nsIAccessible doesn't seem to require that there be one nsIAccessible per DOM node, so it should be fine to construct one nsIAccessible per table part frame.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

However, you don't want to have a frame pointer stored persistently in your accessibles, that would be bad.

I see we don't have nsIAccessibleTableRow/RowGroup/Column. That simplifies the problem.

Maybe we could define a new nsAnonymousTableCellAccessible which just stores its parent (table) accessible and a row/column index. getTableCellAt would return an nsIAccessible for the table cell element if there is one, otherwise it would find or construct a nsAnonymousTableCellAccessible and return it.

Alternatively, the nsAnonymousTableCellAccessible could store a reference to the first child node of the cell.

Both of these alternatives would break in different ways under some kinds of DOM mutations. So would alternatives that hold onto a frame pointer, though. I'm not sure there's a reasonable way to track the identity of anonymous cells through all kinds of DOM manipulation. I'd probably just go with the row/column index approach.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(which would be relatively simple to implement, and would work perfectly in all cases except where someone inserts or removes a table row or column before an anonymous table-cell, while the AT is using the cell's nsIAccessible.)

Comment 35

[David Bolter [:davidb] (NeedInfo me for attention)]

Alex do you still need this bug open?

Comment 36

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #35)
> Alex do you still need this bug open?

I think this is a valid issue still.

Comment 37

[James Teh [:Jamie]]

After bug 1007975, we no longer create table accessibles for CSS tables. So, this bug is no longer relevant.