[webkit-reviews] review requested: [Bug 35422] [Gtk] Layout tables should indicate that they are not data tables via an object attribute : [Attachment 71861] Patch proposal + Unit test

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 35422: [Gtk] Layout tables should indicate that they are not data tables
via an object attribute
https://bugs.webkit.org/show_bug.cgi?id=35422

Attachment 71861: Patch proposal + Unit test
https://bugs.webkit.org/attachment.cgi?id=71861&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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.