Bug 87052 – AX: Contenteditable tables should always be exposed as data tables

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

The AX code has heuristics that try to avoid exposing layout tables as data tables. However, when a section of the document is contentEditable, all tables should be treated as data tables, otherwise users may not be able to work with rich text editors that allow creating and editing tables.

Firefox already works this way (if it's contenteditable, all tables are data tables), I suggest WebKit follow that lead to keep things simple, unless there are any good arguments not to.

[Alice Boxhall]

Created attachment 143428 [details]
Patch

[WebKit Review Bot]

Attachment 143428 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Boxhall]

Created attachment 143432 [details]
Patch

[Dominic Mazzoni]

Don't you need to update the expected file in platform/mac/accessibility?

[chris fleizach]

Comment on attachment 143432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143432&action=review

looks like we're missing an expected file for the table-detection test

> Source/WebCore/accessibility/AccessibilityTable.cpp:99
> +    if (m_renderer->node() && m_renderer->node()->isContentEditable())

add a comment explaining why you're doing this please

> Source/WebCore/accessibility/AccessibilityTable.cpp:100
> +        return true;

you should be able to call just node() && node()->isContentEditable()

> LayoutTests/accessibility/table-detection.html:105
> +          <td nowrap="1" class="labelPol_080915_Lehman_Contributions">

probably unnecessary to include class="labelPol_080915_Lehman_Contributions"

[Dominic Mazzoni]

(In reply to comment #5)
> probably unnecessary to include class="labelPol_080915_Lehman_Contributions"

I think this was so that it could copy a table above. My preference would be to copy something more trivial, like a table with one cell or a table with two cells but no borders - rather than this more complicated example.

[Alice Boxhall]

Comment on attachment 143432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143432&action=review

>> Source/WebCore/accessibility/AccessibilityTable.cpp:99
>> +    if (m_renderer->node() && m_renderer->node()->isContentEditable())
> 
> add a comment explaining why you're doing this please

Done.

>> Source/WebCore/accessibility/AccessibilityTable.cpp:100
>> +        return true;
> 
> you should be able to call just node() && node()->isContentEditable()

Done.

>> LayoutTests/accessibility/table-detection.html:105
>> +          <td nowrap="1" class="labelPol_080915_Lehman_Contributions">
> 
> probably unnecessary to include class="labelPol_080915_Lehman_Contributions"

Done as per Dominic's suggestion.

[Alice Boxhall]

Created attachment 143481 [details]
Patch

[chris fleizach]

Comment on attachment 143481 [details]
Patch

looks good

[WebKit Review Bot]

Comment on attachment 143481 [details]
Patch

Clearing flags on attachment: 143481

Committed r118143: <http://trac.webkit.org/changeset/118143>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.