Bug 32007 - WAI-ARIA: implement support for ARIA drag and drop
Summary: WAI-ARIA: implement support for ARIA drag and drop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-11-30 22:18 PST by chris fleizach
Modified: 2009-12-01 20:54 PST (History)
2 users (show)

See Also:


Attachments
patch (16.10 KB, patch)
2009-11-30 22:29 PST, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2009-11-30 22:18:52 PST
WAI-ARIA: implement support for ARIA drag and drop

* NOTES
ARIA supports a few attributes to enable drag & drop interfaces to be accessible through a keyboard. The grabbed state indicates whether an element is grabbed for dragging (or whether it can be grabbed), and the dropeffect is applied to other elements during an active drag operation.

 • aria-grabbed
 • aria-dropeffect

http://www.w3.org/TR/wai-aria/#dragdrop
Comment 1 chris fleizach 2009-11-30 22:29:15 PST
Created attachment 44054 [details]
patch

This provides the basics needed for other platforms to implement ARIA DnD and does the best it can with what the spec provides for the Mac. VoiceOver will be able to adopt these changes.
Comment 2 WebKit Review Bot 2009-11-30 22:33:25 PST
style-queue ran check-webkit-style on attachment 44054 [details] without any errors.
Comment 3 Darin Adler 2009-12-01 11:21:25 PST
Comment on attachment 44054 [details]
patch

> +void AccessibilityRenderObject::determineARIADropEffects(Vector<String>& effects)
> +{
> +    String dropEffects = getAttribute(aria_dropeffectAttr).string();
> +    if (dropEffects.isEmpty())
> +        return;

Probably would be better to clear "effects" before returning.

> +        unsigned length = dropEffects.size();

The best type to use here would be size_t rather than unsigned.

> +        NSMutableArray* dropEffectsArray = [NSMutableArray array];

It's better for performance to use the arrayWithCapacity: method here.

> +        for (unsigned i = 0; i < length; ++i)

Again, size_t.

> +    // A space concatentated string of all the drop effects.
> +    JSStringRef ariaDropEffects() const;

Could this be a RetainPtr<JSStringRef> rather than a JSStringRef?

> +    id value = [m_element accessibilityAttributeValue:NSAccessibilityDropEffectsAttribute];
> +    if (![value isKindOfClass:[NSArray class]])
> +        return 0;
> +
> +    NSMutableString* dropEffects = [NSMutableString string];
> +    NSInteger length = [value count];
> +    for (NSInteger k = 0; k < length; ++k) {
> +        [dropEffects appendString:[value objectAtIndex:k]];
> +        if (k < length - 1)
> +            [dropEffects appendString:@" "];
> +    }
> +    
> +    return [dropEffects createJSStringRef];

Rebuilding the string with spaces makes it hard to detect bugs involving mis-parsing the string. It would be better if the separator here was not the same as the separator used in the syntax itself. Maybe ", " instead or " | " or something like that.

r=me -- please consider some of my suggested improvements above.
Comment 4 chris fleizach 2009-12-01 11:22:42 PST
will implement all your suggestions
Comment 5 chris fleizach 2009-12-01 20:36:17 PST
(In reply to comment #3)

> > +    // A space concatentated string of all the drop effects.
> > +    JSStringRef ariaDropEffects() const;
> 
> Could this be a RetainPtr<JSStringRef> rather than a JSStringRef?
> 

All the other methods that return JSStringRef do not return a retain'd ptr, so i don't want to break that trend with this patch
Comment 6 chris fleizach 2009-12-01 20:41:09 PST
http://trac.webkit.org/changeset/51582
Comment 7 chris fleizach 2009-12-01 20:52:12 PST
looks like i forgot to add the DumpRenderTree methods for windows and GTK.. will get to that ASAP
Comment 8 chris fleizach 2009-12-01 20:54:43 PST
hopefully fixed it with
http://trac.webkit.org/changeset/51583