Bug 26262 - Implement HTML5 draggable
: Implement HTML5 draggable
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Erik Arvidsson
http://www.whatwg.org/specs/web-apps/...
:
Depends on:
Blocks: 32934
  Show dependency treegraph
 
Reported: 2009-06-08 16:17 PDT by Erik Arvidsson
Modified: 2009-12-25 08:35 PST (History)
1 user (show)

See Also:


Attachments
Patch (10.35 KB, patch)
2009-07-02 18:45 PDT, Erik Arvidsson
mjs: review-
Details | Formatted Diff | Diff
Uses parseMappedAttribute instead of the user style sheet (11.20 KB, patch)
2009-07-04 00:09 PDT, Erik Arvidsson
darin: review-
Details | Formatted Diff | Diff
Fixed Darin's comments. (11.09 KB, patch)
2009-07-04 20:56 PDT, Erik Arvidsson
mjs: review+
Details | Formatted Diff | Diff
Style nits fixed (11.08 KB, patch)
2009-07-06 14:32 PDT, Erik Arvidsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2009-06-08 16:17:07 PDT
Currently, to drag something in webkit it has to be a link or an image (or text selection). HTML5 defines a draggable attribute that allows any element to be draggable.

http://www.whatwg.org/specs/web-apps/current-work/#the-draggable-attribute
Comment 1 Erik Arvidsson 2009-06-30 18:43:55 PDT
This is actually trivial. All we need to do is add the following to the ua style sheet.

[draggable] {
    -webkit-user-drag: element;
    -webkit-user-select: none;
}
Comment 2 Erik Arvidsson 2009-07-02 18:45:05 PDT
Created attachment 32215 [details]
Patch
Comment 3 Sam Weinig 2009-07-02 20:57:37 PDT
Comment on attachment 32215 [details]
Patch

The style part should probably be implemented in parseMappedAttribute instead of the ua stylesheet.
Comment 4 Maciej Stachowiak 2009-07-03 00:57:06 PDT
Comment on attachment 32215 [details]
Patch

Hyatt confirms that "draggable" should be implemented in code via parseMappedAttribute rather than in the UA stylesheet, because in our CSS implementation it's fairly inefficient to have an attribute selector that's not scoped to particular tags. I believe HTMLElement::parseMappedAttribute would be the right place.

r- to fix this, patch otherwise looks good to me.
Comment 5 Erik Arvidsson 2009-07-04 00:09:42 PDT
Created attachment 32253 [details]
Uses parseMappedAttribute instead of the user style sheet

I also did some testing with Firefox 3.5 and it ignores the case of true and false so I made all the checks ignore the case. Also, added code to correctly handle when the attribute is set to something other than true or false and updated the test to reflect that.
Comment 6 Erik Arvidsson 2009-07-04 00:14:05 PDT
(In reply to comment #4)
> (From update of attachment 32215 [details])
> Hyatt confirms that "draggable" should be implemented in code via
> parseMappedAttribute rather than in the UA stylesheet, because in our CSS
> implementation it's fairly inefficient to have an attribute selector that's not
> scoped to particular tags. I believe HTMLElement::parseMappedAttribute would be
> the right place.

Thanks for the hint about parseMappedAttribute. It makes total sense to me now. I'll try to keep that in mind any time I think I need to touch the UA stylesheet.
Comment 7 Darin Adler 2009-07-04 11:23:12 PDT
Comment on attachment 32253 [details]
Uses parseMappedAttribute instead of the user style sheet

> +    if (equalIgnoringCase(getAttribute(draggableAttr), "true"))
> +        return true;
> +    if (equalIgnoringCase(getAttribute(draggableAttr), "false"))
> +        return false;

It would be more efficient to call getAttribute only once. A local variable of type const AtomicString& can be used to hold the attribute value.

> +void HTMLElement::setDraggable(MappedAttribute* attr)

I think it would be best to give this a different name, rather than using overloading. Perhaps it could be called parseDraggableAttribute.

> +void HTMLElement::setDraggable(const bool& value)

This should just be bool, not const bool&.

> +    void setDraggable(MappedAttribute*);

This newly-added member function should be private.

This looks good, but I'm going to say review- since I'd really like to see the name and visibility of the setDraggable function that takes an attribute changed.
Comment 8 Erik Arvidsson 2009-07-04 20:55:15 PDT
(In reply to comment #7)

Thanks for the review Darin

> (From update of attachment 32253 [details])
> > +    if (equalIgnoringCase(getAttribute(draggableAttr), "true"))
> > +        return true;
> > +    if (equalIgnoringCase(getAttribute(draggableAttr), "false"))
> > +        return false;
> 
> It would be more efficient to call getAttribute only once. A local variable of
> type const AtomicString& can be used to hold the attribute value.

Done.

> > +void HTMLElement::setDraggable(MappedAttribute* attr)
> 
> I think it would be best to give this a different name, rather than using
> overloading. Perhaps it could be called parseDraggableAttribute.

I ended up inlining it in parseMappedAttribute since the logic is pretty simple and other cases are more complicated already.

> > +void HTMLElement::setDraggable(const bool& value)
> 
> This should just be bool, not const bool&.

Done.
 
> > +    void setDraggable(MappedAttribute*);
> 
> This newly-added member function should be private.
> 
> This looks good, but I'm going to say review- since I'd really like to see the
> name and visibility of the setDraggable function that takes an attribute
> changed.

Another patch coming in a sec
Comment 9 Erik Arvidsson 2009-07-04 20:56:43 PDT
Created attachment 32266 [details]
Fixed Darin's comments.
Comment 10 Maciej Stachowiak 2009-07-05 04:22:30 PDT
Comment on attachment 32266 [details]
Fixed Darin's comments.

Looks to me like the comments are all addressed, and I do not see any new issues. Thanks for the contribution and the several revised versions! r=me
Comment 11 Eric Seidel 2009-07-06 14:14:39 PDT
Comment on attachment 32266 [details]
Fixed Darin's comments.

There are style issues in this patch:

Spacing:
+    for (var i = 16; i < 24; i++) {
+      elements[i].draggable = i < 20;
+    }

Extra {
 151         } else if (equalIgnoringCase(value, "false")) {
 152             addCSSProperty(attr, CSSPropertyWebkitUserDrag, CSSValueNone);
 153         }

Extra space:
 357     if (equalIgnoringCase(value , "true"))

Those certainly could be corrected when landing.  But having them here, makes this hard to land automatically.
Comment 12 Erik Arvidsson 2009-07-06 14:21:19 PDT
(In reply to comment #11)
> (From update of attachment 32266 [details])
> There are style issues in this patch:
> 
> Spacing:
> +    for (var i = 16; i < 24; i++) {
> +      elements[i].draggable = i < 20;
> +    }
> 
> Extra {
>  151         } else if (equalIgnoringCase(value, "false")) {
>  152             addCSSProperty(attr, CSSPropertyWebkitUserDrag, CSSValueNone);
>  153         }
> 
> Extra space:
>  357     if (equalIgnoringCase(value , "true"))
> 
> Those certainly could be corrected when landing.  But having them here, makes
> this hard to land automatically.

I'll just fix them and ask for another review.
Comment 13 Erik Arvidsson 2009-07-06 14:32:27 PDT
Created attachment 32327 [details]
Style nits fixed
Comment 14 Erik Arvidsson 2009-07-13 16:17:05 PDT
Can anyone submit this for me? I do not have commit rights.
Comment 15 Darin Adler 2009-07-13 17:42:20 PDT
This changes the result of fast/dom/domListEnumeration.html but the patch doesn't include new results. I had to fix that myself. I'm concerned, because this should have showed up when you ran the regression tests yourself.
Comment 16 Erik Arvidsson 2009-07-13 17:50:00 PDT
(In reply to comment #15)
> This changes the result of fast/dom/domListEnumeration.html but the patch
> doesn't include new results. I had to fix that myself. I'm concerned, because
> this should have showed up when you ran the regression tests yourself.

My bad. Feel free to r- and I'll look into it
Comment 17 Darin Adler 2009-07-13 18:06:13 PDT
http://trac.webkit.org/changeset/45851