Bug 127688 - AX: Support @scope in HTML tables
Summary: AX: Support @scope in HTML tables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-27 10:50 PST by chris fleizach
Modified: 2014-01-30 10:01 PST (History)
10 users (show)

See Also:


Attachments
patch (57.61 KB, patch)
2014-01-27 11:16 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (57.85 KB, patch)
2014-01-27 11:31 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (57.82 KB, patch)
2014-01-27 17:34 PST, chris fleizach
mario: review+
mario: commit-queue-
Details | Formatted Diff | Diff
patch (57.68 KB, patch)
2014-01-28 08:50 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (57.68 KB, patch)
2014-01-28 08:57 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2014-01-27 10:50:07 PST
We need to support scope in tables for AX

http://www.w3.org/TR/html401/struct/tables.html#adef-scope

<rdar://problem/6237955>
Comment 1 chris fleizach 2014-01-27 11:16:10 PST
Created attachment 222338 [details]
patch
Comment 2 chris fleizach 2014-01-27 11:31:24 PST
Created attachment 222340 [details]
patch
Comment 3 chris fleizach 2014-01-27 17:34:46 PST
Created attachment 222387 [details]
patch
Comment 4 Mario Sanchez Prada 2014-01-28 08:10:07 PST
Comment on attachment 222387 [details]
patch 

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

Assuming that the Objective-C bits are Ok (which do seem Ok to me, with the little knowledge I have about the language), the patch looks good to me.

I would only ask you for uploading a new version (no need to review it again) of the patch removing the uiElementArrayAttributeValue() empty stub from WKTR's AccessibilityUIElementAtk.cpp before landing it, so we make sure that such a change is enough to make EWS bots happy.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-93
> -#ifndef NSAccessibilityRowHeaderUIElementsAttribute
> -#define NSAccessibilityRowHeaderUIElementsAttribute @"AXRowHeaderUIElements"
> -#endif
> -

I guess this is intentional? Just double checking

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:828
> +JSValueRef AccessibilityUIElement::uiElementArrayAttributeValue(JSStringRef attribute) const
> +{
> +    // FIXME: implement
> +    return nullptr;
> +}
> +    

You don't need to add this empty method, it's already there (and the reason why it's failing to build in GTK and EFL)
Comment 5 chris fleizach 2014-01-28 08:49:14 PST
(In reply to comment #4)
> (From update of attachment 222387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222387&action=review
> 
> Assuming that the Objective-C bits are Ok (which do seem Ok to me, with the little knowledge I have about the language), the patch looks good to me.
> 
> I would only ask you for uploading a new version (no need to review it again) of the patch removing the uiElementArrayAttributeValue() empty stub from WKTR's AccessibilityUIElementAtk.cpp before landing it, so we make sure that such a change is enough to make EWS bots happy.
> 

Yep of course. I would have fixed these already but I'm no longer getting emails when builds fail

> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-93
> > -#ifndef NSAccessibilityRowHeaderUIElementsAttribute
> > -#define NSAccessibilityRowHeaderUIElementsAttribute @"AXRowHeaderUIElements"
> > -#endif
> > -
> 
> I guess this is intentional? Just double checking

Yep, this name has now been defined for 3 releases.

> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:828
> > +JSValueRef AccessibilityUIElement::uiElementArrayAttributeValue(JSStringRef attribute) const
> > +{
> > +    // FIXME: implement
> > +    return nullptr;
> > +}
> > +    
> 
> You don't need to add this empty method, it's already there (and the reason why it's failing to build in GTK and EFL)

Thanks!
Comment 6 chris fleizach 2014-01-28 08:50:36 PST
Created attachment 222449 [details]
patch
Comment 7 WebKit Commit Bot 2014-01-28 08:52:09 PST
Attachment 222449 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityTableCell.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 chris fleizach 2014-01-28 08:57:36 PST
Created attachment 222451 [details]
patch
Comment 9 chris fleizach 2014-01-28 17:00:50 PST
http://trac.webkit.org/changeset/162986
Comment 10 David Farler 2014-01-30 10:01:04 PST
This breaks the iOS build - filed https://bugs.webkit.org/show_bug.cgi?id=127917 for implementation of methods in AccessibilityUIElement for iOS.