[Webkit-unassigned] [Bug 35422] [Gtk] Layout tables should indicate that they are not data tables via an object attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35422


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71628|0                           |1
        is obsolete|                            |
  Attachment #71861|                            |review?
               Flag|                            |




--- Comment #7 from Mario Sanchez Prada <msanchez at igalia.com>  2010-10-26 04:03:47 PST ---
Created an attachment (id=71861)
 --> (https://bugs.webkit.org/attachment.cgi?id=71861&action=review)
Patch proposal + Unit test

Attaching a new patch

(In reply to comment #6)
> (From update of attachment 71628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71628&action=review
> 
> i agree with the refactoring. just have some questions inline

That's a good start!

> > WebCore/accessibility/AccessibilityTable.cpp:87
> > +    return m_isAccessibilityTable;
> 
> where does m_isAccessibilityTable get set now that isTableExposableThroughAccessibility() is const

It's still set in the same place than before, in the constructor:

   AccessibilityTable::AccessibilityTable(RenderObject* renderer)
       : AccessibilityRenderObject(renderer),
       m_headerContainer(0)
   {
   #if ACCESSIBILITY_TABLES
       m_isAccessibilityTable = isTableExposableThroughAccessibility();
   #else
       m_isAccessibilityTable = false;
   #endif
   }

Other than that, is also redefined in subclasses, such as AccessibilityARIAGrid.

> > WebCore/accessibility/AccessibilityTable.cpp:95
> > +    // don not consider it a data table is it has an ARIA role
> 
> spelling. end sentence with period. capitalize start of sentence

Fixed. Sorry.

> > WebCore/accessibility/AccessibilityTable.cpp:100
> > +    // Only "data" tables should be exposed as tables.
> 
> sentence should start with capital letter

Fixed.

> > WebCore/accessibility/AccessibilityTable.cpp:258
> > +
> 
> fix this comment (even though it's not yours to look more like a real sentence

No problem. Fixed.

> > WebCore/accessibility/AccessibilityTable.cpp:263
> > +    // expose it as a table, unless, of course, the aria role is a table
> 
> ditto

Fixed.

> > WebCore/accessibility/AccessibilityTable.cpp:266
> > +
> 
> does this still account for role="table"?

Not sure what you mean here... what I can tell is that it basically keeps the same behaviour than before, but I'm afraid I didn't enter in such a level of detail to answer this question :-/

> > WebCore/accessibility/AccessibilityTable.cpp:272
> > +
> 
> you can probably do this in one line
> 
> return tableNode && tableNode->hasTagName(tableTag)

Sure. Fixed.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:336
> > +    if (coreObject) {
> 
> you should use an early return here if !coreObject)

Fixed.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:344
> > +        // Technologies know when an exposed table is not data table
> 
> end the sentence with a period

Fixed.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:345
> > +        if (coreObject->isAccessibilityTable() && !coreObject->isDataTable()) {
> 
> not sure why you want an AXTable, but not a dataTable... wouldn't that mean the layout-guess is false, because you know it's an accessibility table.
> can you explain to me why this is right?

Yes. The point is that in the GTK port we expose to accessibiltity every table, no matter it's a layout or a data table, so isAccessibilityTable() is going to return always 'true' as long as the coreObject is an isntance of AccessibilityTable. However, and this is the main reason behind splitting the former isDataTable() function, we need to know when those accessibility tables are not data tables, according to the heuristic used. When that happens we just set an extra 'layout-guess' attribute to 'true' so ATs can treat them like that (if you have more doubts about why to use this mechanism instead of doing it in other way, I guess Joanmarie'd better answer that).

Hope this helps.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:347
> > +            attributeSet = addAttributeToSet(attributeSet, "layout-guess", value.utf8().data());
> 
> can you not do 
> 
> 
> attributeSet = addAttributeToSet(attributeSet, "layout-guess", "true");
> ?

Definitely. Fixed

Asking for review again, then.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.