693948 - expose layout-guess: true object attribute on CSS table accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[steve faulkner]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928134238

Steps to reproduce:

viewed a test page that uses CSS display:table http://www.456bereastreet.com/lab/display-table/ further info: Using display:table has semantic effects in some screen readers http://www.456bereastreet.com/archive/201110/using_displaytable_has_semantic_effects_in_some_screen_readers/


Actual results:

div elements are represented in the accessibility tree as table elements. the following contains a screen shot of the accessibility tree in the DOM inspector for http://www.456bereastreet.com/lab/display-table/


Expected results:

the div elements should not be represented as table elements

Comment 1

[Marco Zehe (:MarcoZ)]

OK, so display:table style is ALWAYS for layout purposes? If so, we can easily fix that by ALWAYS treating structures that have display:table and associated as if they were layout tables, by setting the appropriate object attribute to tell NVDA and Orca to ignore the table semantics.

Comment 2

[steve faulkner]

(In reply to Marco Zehe (:MarcoZ) from comment #1)
> OK, so display:table style is ALWAYS for layout purposes? If so, we can
> easily fix that by ALWAYS treating structures that have display:table and
> associated as if they were layout tables, by setting the appropriate object
> attribute to tell NVDA and Orca to ignore the table semantics.

Hi Marco
you wrote;
 "OK, so display:table style is ALWAYS for layout purposes?"

I would suggest that the majority of uses are/will be to achieve a layout effect and we definitely should not encourage otherwise. It is also beneficial to make the change as it will bring firefox in line with how other browsers treat display:table.

Comment 3

[Marco Zehe (:MarcoZ)]

FTR: I suspect VoiceOver in Safari, and NVDA in IE, because they do DOM parsing mostly rather than accessible tree parsing, will just not pick up display:table; style information and treat these like divs, that's why they are not being picked up as tables in those combinations mentioned in the article. I definitely agree that we should put the object attribute in, and encourage to NOT use these CSS styles for real data tables.

Comment 4

[steve faulkner]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> FTR: I suspect VoiceOver in Safari, and NVDA in IE, because they do DOM
> parsing mostly rather than accessible tree parsing, will just not pick up
> display:table; style information and treat these like divs, that's why they
> are not being picked up as tables in those combinations mentioned in the
> article. I definitely agree that we should put the object attribute in, and
> encourage to NOT use these CSS styles for real data tables.

My understanding (from comments made by 	
Maciej Stachowiak of webkit) in regards to VoiceOver was that it didn't do any DOM parsing, taking all info from the accessibility API. Will follow up on that with the aplle folk.

Comment 5

[steve faulkner]

(In reply to steve faulkner from comment #4)
> (In reply to Marco Zehe (:MarcoZ) from comment #3)
> > FTR: I suspect VoiceOver in Safari, and NVDA in IE, because they do DOM
> > parsing mostly rather than accessible tree parsing, will just not pick up
> > display:table; style information and treat these like divs, that's why they
> > are not being picked up as tables in those combinations mentioned in the
> > article. I definitely agree that we should put the object attribute in, and
> > encourage to NOT use these CSS styles for real data tables.
> 
> My understanding (from comments made by 	
> Maciej Stachowiak of webkit) in regards to VoiceOver was that it didn't do
> any DOM parsing, taking all info from the accessibility API. Will follow up
> on that with the aplle folk.

FYI
James Craig confirmed that VoiceOver uses the Mac Accessibility API, not the DOM

Comment 6

[James Teh [:Jamie]]

As far as I know (and confirmed with a quick test), the layout-guess:true object attribute is already set for CSS display: table. In short, this allows a client to see the structure of the table via the a11y hierarchy if desired, while still notifying the client that it is a layout table.

By default, NVDA certainly honours this attribute in browse mode and when reporting focus ancestry.

Comment 7

[alexander :surkov (:asurkov)]

I agree with Jamie, the current approach is we allow AT to decide. So either we should take decision on our side and never expose tables having layout-guess:true object attribute or do not change anything.

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #6)
> By default, NVDA certainly honours this attribute in browse mode and when
> reporting focus ancestry.

Jamie, if you look at the very first table on the sample page Steve links to, you will see that NVDA, at least 2011.2 release, does indeed expose the full table semantics. Only the second table is being treated as a layout table and table semantics are suppressed. Both tables are built with CSS display:table; styles. Am I missing anything?

Comment 9

[James Teh [:Jamie]]

Damn. I was incorrect about display: table always exposing layout-guess: true. My test case was very simple and obviously triggered something else that caused Gecko to set layout-guess: true. I agree that layout-guess: true should *always* be exposed for CSS tables.

Comment 10

[alexander :surkov (:asurkov)]

Ok, all we need to make nsHTMLTableAccessible::IsProbablyForLayout to check the tag name. If it's different from HTML table then expose layout-guess:true.

Comment 11

[steve faulkner]

ok, so the question that remains in my mind is why does CSS change the roles of the elements in the first place? This seems wrong to me.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to steve faulkner from comment #11)
> ok, so the question that remains in my mind is why does CSS change the roles
> of the elements in the first place? This seems wrong to me.

CSS affects on visual presentation, the purpose of accessibility is to expose to AT what user sees on the screen. For sighted users it doesn't matter what way was used to create a tabular representation therefore we expose those the same way for blind users.

Comment 13

[James Teh [:Jamie]]

(In reply to steve faulkner from comment #11)
> ok, so the question that remains in my mind is why does CSS change the roles
> of the elements in the first place? This seems wrong to me.
I guess you could equally ask why CSS display: none affects accessibility or why CSS formatting is exposed to accessibility. As much as this is all presentation info, it is still potentially relevant to ATs. In this particular case, we don't care or use that info, but that doesn't mean someone mightn't want to in future.

Comment 14

[steve faulkner]

(In reply to James Teh [:Jamie] from comment #13)
> (In reply to steve faulkner from comment #11)
> > ok, so the question that remains in my mind is why does CSS change the roles
> > of the elements in the first place? This seems wrong to me.
> I guess you could equally ask why CSS display: none affects accessibility or
> why CSS formatting is exposed to accessibility. As much as this is all
> presentation info, it is still potentially relevant to ATs. In this
> particular case, we don't care or use that info, but that doesn't mean
> someone mightn't want to in future.

hi James, I am not questioning whether the information should be conveyed, only the manner in which it is conveyed (by changing the roles of the elements). Roger Johannson talks about this more in http://www.456bereastreet.com/archive/201110/using_displaytable_has_semantic_effects_in_some_screen_readers/

Comment 15

[alexander :surkov (:asurkov)]

As far as I can see Roger Johannson doesn't answer why display information should or shouldn't affect on the accessible role and hierarchy. All he said he didn't expect that it affects on accessible tree.

In Gecko we keep accessible tree closer to layout rather than DOM tree to make sure AT users gets same piece of information as sighted users do. ARIA is sort of exception of this rule, we apply it on top of accessible tree. But it's not clear with me that we should ignore some CSS to follow DOM closer.

Comment 16

[James Teh [:Jamie]]

(In reply to steve faulkner from comment #14)
> I am not questioning whether the information should be conveyed,
> only the manner in which it is conveyed (by changing the roles of the
> elements).
You can't convey table information without using roles and table interfaces. However, what we're proposing here is that Gecko expose it as a *layout* table. That's what the layout-guess:true attribute is for.

> Roger Johannson talks about this more in...
He talks about not wanting screen readers to treat them as tables when reporting them to the user. If layout-guess:true is exposed (as is being proposed in this bug), well-behaved screen readers *won't* treat them as tables when reporting them to the user.

Comment 17

[steve faulkner]

(In reply to James Teh [:Jamie] from comment #16)
> (In reply to steve faulkner from comment #14)
> > I am not questioning whether the information should be conveyed,
> > only the manner in which it is conveyed (by changing the roles of the
> > elements).
> You can't convey table information without using roles and table interfaces.
> However, what we're proposing here is that Gecko expose it as a *layout*
> table. That's what the layout-guess:true attribute is for.
> 
> > Roger Johannson talks about this more in...
> He talks about not wanting screen readers to treat them as tables when
> reporting them to the user. If layout-guess:true is exposed (as is being
> proposed in this bug), well-behaved screen readers *won't* treat them as
> tables when reporting them to the user.

hi James, alexander, 

"what we're proposing here is that Gecko expose it as a *layout* table. That's what the layout-guess:true attribute is for."

Sounds emminently reasonable to me. I am not arguing against this, only seeking to understand the reasoning, thanks for the replies!

Comment 18

[James Teh [:Jamie]]

All of this said, the name layout-guess:true is misleading in this case and afaik layout-guess is also non-standard. In future, we should perhaps consider a better named attribute (e.g. layout:{true,false,true-guess,false-guess}) and get it into the accessibility API standards.

Comment 19

[steve faulkner]

I am going to attempt document the IsProbablyForLayout algorithm any pointers to info?

Comment 20

[steve faulkner]

I should have said where I am going to document it: http://dev.w3.org/html5/html-api-map/#ta

Comment 21

[alexander :surkov (:asurkov)]

(In reply to steve faulkner from comment #19)
> I am going to attempt document the IsProbablyForLayout algorithm any
> pointers to info?

I afraid we don't have any documentation on this. All we have is source code. I think I should find some time to document it.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to steve faulkner from comment #19)
> I am going to attempt document the IsProbablyForLayout algorithm any
> pointers to info?

Steve, I put algorithm at my blog http://asurkov.blogspot.com/2011/10/data-vs-layout-table.html

Comment 23

[steve faulkner]

(In reply to alexander surkov from comment #22)
> (In reply to steve faulkner from comment #19)
> > I am going to attempt document the IsProbablyForLayout algorithm any
> > pointers to info?
> 
> Steve, I put algorithm at my blog
> http://asurkov.blogspot.com/2011/10/data-vs-layout-table.html

Hi alex, many thanks! can i use this as the baisis for the documentation here: http://dev.w3.org/html5/html-api-map/#ta

Comment 24

[alexander :surkov (:asurkov)]

(In reply to steve faulkner from comment #23)
> (In reply to alexander surkov from comment #22)
> > (In reply to steve faulkner from comment #19)
> > > I am going to attempt document the IsProbablyForLayout algorithm any
> > > pointers to info?
> > 
> > Steve, I put algorithm at my blog
> > http://asurkov.blogspot.com/2011/10/data-vs-layout-table.html
> 
> Hi alex, many thanks! can i use this as the baisis for the documentation
> here: http://dev.w3.org/html5/html-api-map/#ta

sure, that was a primary reason to blog about it to not make you to dig through our code

Comment 25

[Oussama BADR]

Hi everybody !!
let me to interfere with you in this discussion!!! 
to summing up your talks : we should check for CSS table to conclude that's for layout, but only for other HTML elements and none for HTML table, because (I think) in case of HTML table we should let the code to do its checks, unless we have strong reason for layout , right?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Ouss. BADR from comment #25)
> Hi everybody !!
> let me to interfere with you in this discussion!!! 
> to summing up your talks : we should check for CSS table to conclude that's
> for layout, but only for other HTML elements and none for HTML table,
> because (I think) in case of HTML table we should let the code to do its
> checks, unless we have strong reason for layout , right?

Yes.

Comment 27

[Oussama BADR]

after some analysing,  I think we should create a new file like "nsHTMLCSSAccessible.cpp", coz we have to work on the CSS side for "layout-guess", 
What do you suggest? 

another think, is the platform just for windows vista and x86 or is it for  all platform? coz I didn't expect the platform specified for this bug!!!!! O_o

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to Ouss. BADR from comment #27)
> after some analysing,  I think we should create a new file like
> "nsHTMLCSSAccessible.cpp", coz we have to work on the CSS side for
> "layout-guess", 
> What do you suggest? 

that's not neccessary just check mContent->Tag() == nsGkAtoms::tag (patch comming in a few minutes its trivial)> 
>
 another think, is the platform just for windows vista and x86 or is it for 
> all platform? coz I didn't expect the platform specified for this bug!!!!!
> O_o

no, just artifat from the person filing using windows and so that being the default

Comment 29

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 30

[Oussama BADR]

Hi Trevor!!

>  if (mContent->Tag() != nsGkAtoms::table)
>    RETURN_LAYOUT_ANSWER(true, "html table without table tag");

I think you should add a comment after this line like :
// Check for CSS table

...or something else which can explain what your lines are about ..

thks!!



thks !!!

Comment 31

[Oussama BADR]

.....sorry I mean before this line !

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 572407 [details] [diff] [review]
patch

Review of attachment 572407 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1387,5 @@
>      RETURN_LAYOUT_ANSWER(false, "Has role attribute, weak role, and role is table");
>    }
>  
> +  if (mContent->Tag() != nsGkAtoms::table)
> +    RETURN_LAYOUT_ANSWER(true, "html table without table tag");

maybe: table built by CSS display: table style?

::: accessible/tests/mochitest/table/test_layoutguess.html
@@ +100,5 @@
>  
> +      // css table with non-table tag
> +testAttrs("table23", attr, true);
> +testAttrs("table24", attr, true);
> +testAttrs("table24", attr, true);

wrong indentation

@@ +459,5 @@
> +
> +                                                                                                                    <!-- third css table with non-table tags -->
> +                                                                                                                                  <div id="table25" style="display:table">
> +                                                                                                                                          <div style="display:table-cell">Single-cell table</div>
> +                                                                                                                                              </div>

wrong indentation
btw, why do you need 3 tables instead one, i.e. what code paths you are checking?

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 572407 [details] [diff] [review]
patch

r=me with comments addressed

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch2 [for check in]

Comment 35

[alexander :surkov (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c5cce62078

Comment 36

[Oussama BADR]

Congratulation Alexander for fixing it...!!!
Even if busy, I don't hesitate to look around.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Ouss. BADR from comment #36)
> Congratulation Alexander for fixing it...!!!
> Even if busy, I don't hesitate to look around.

yep, we just need this bug fixed asap so since you can't be active these days then I decided to fix nits and land Trevor's patch.

Comment 38

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c9c5cce62078