[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


chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71628|review?                     |review-
               Flag|                            |




--- Comment #6 from chris fleizach <cfleizach at apple.com>  2010-10-25 11:57:03 PST ---
(From update of attachment 71628)
View in context: https://bugs.webkit.org/attachment.cgi?id=71628&action=review

i agree with the refactoring. just have some questions inline

> WebCore/accessibility/AccessibilityTable.cpp:87
> +    return m_isAccessibilityTable;

where does m_isAccessibilityTable get set now that isTableExposableThroughAccessibility() is const

> 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

> WebCore/accessibility/AccessibilityTable.cpp:100
> +    // Only "data" tables should be exposed as tables.

sentence should start with capital letter

> WebCore/accessibility/AccessibilityTable.cpp:258
> +

fix this comment (even though it's not yours to look more like a real sentence

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

ditto

> WebCore/accessibility/AccessibilityTable.cpp:266
> +

does this still account for role="table"?

> WebCore/accessibility/AccessibilityTable.cpp:272
> +

you can probably do this in one line

return tableNode && tableNode->hasTagName(tableTag)

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:336
> +    if (coreObject) {

you should use an early return here if !coreObject)

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:344
> +        // Technologies know when an exposed table is not data table

end the sentence with a period

> 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?

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

can you not do 


attributeSet = addAttributeToSet(attributeSet, "layout-guess", "true");
?

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