Bug 141597 – REGRESSION: SVG does not support link dragging

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Daniel Bates]

We don't show a hyperlink drag-image bubble when dragging a SVG hyperlink as we do when dragging an HTML hyperlink. This functionality is tested by step 4 in the manual test <http://trac.webkit.org/browser/trunk/ManualTests/svg-links.svg?rev=100838>.

[Daniel Bates]

Created attachment 246580 [details]
Test case

A test case that can be used to demonstrate the issue. Compare the UI behavior of dragging an SVG hyperlink to dragging an HTML hyperlink. Notice that we show a bubble when dragging the HTML hyperlink.

[Daniel Bates]

Created attachment 246581 [details]
[Video] No SVG link dragging

A video that used the attached test case to demonstrate the link dragging behavior for an HTML hyperlink and a SVG hyperlink. Notice the bubble that is shown when dragging the HTML hyperlink. A comparable bubble is not shown when dragging the SVG hyperlink.

[Daniel Bates]

I am using Safari Version 8.0.3 (10600.3.13, r180087).

[Daniel Bates]

Created attachment 250761 [details]
Patch and layout test

I chose to define elementIsLiveLink() as a non-member function because it is a convenience function and is implemented in terms of public interfaces. I was unclear if we should generalize this function for Node objects. I chose to have this function take a reference to Element object instead of a reference to a Node object because the concept of a "live link" (a hyperlink that can be clicked even if it is inside an editable region - maybe "is clickable hyperlink" would be better way to describe this concept?) is more related to higher-order UI behavior than the core behavior of a DOM Node. Let me know if it is preferred to put this functionality in Node or if there is a more appropriate place to put this functionality (HitTestResult?).

[Daniel Bates]

Created attachment 250773 [details]
Patch and layout test

Rebased based following the landing of the patch for bug #143683.

[Alexey Proskuryakov]

Comment on attachment 250773 [details]
Patch and layout test

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

> Source/WebCore/dom/Element.cpp:156
> +bool elementIsLiveLink(const Element& element)

Should AREA match the definition of "live link"?

[Daniel Bates]

(In reply to comment #6)
> Comment on attachment 250773 [details]
> Patch and layout test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250773&action=review
> 
> > Source/WebCore/dom/Element.cpp:156
> > +bool elementIsLiveLink(const Element& element)
> 
> Should AREA match the definition of "live link"?

We could consider an HTML Area element as a "live link" and show a drag image for it when you drag it. Unless you are feel we should implement such a change in this patch, I suggest we address it in a separate bug.

[Alexey Proskuryakov]

I was just trying to understand the terminology, as it wasn't clear from the behavior how a "live link" was different from other links.

[Darin Adler]

Comment on attachment 250773 [details]
Patch and layout test

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

>>> Source/WebCore/dom/Element.cpp:156
>>> +bool elementIsLiveLink(const Element& element)
>> 
>> Should AREA match the definition of "live link"?
> 
> We could consider an HTML Area element as a "live link" and show a drag image for it when you drag it. Unless you are feel we should implement such a change in this patch, I suggest we address it in a separate bug.

I did a bit more research into the definition of this. The concept of "live link" seems to be in the drag code, in the code in EventHandler that deals with a rule about selecting on mouse down that is related to the drag code, and in unused Legacy WebKit1 SPI, WebElementLinkIsLiveKey, which should be removed once we are certain since no one depends on it. Searching both internally at Apple and on the Internet I can’t find anyone using WebElementLinkIsLiveKey.

Given that, I suggest we move this concept to DragController as much as possible rather than having it spread across so many source files. I suggest that over time:

1) We add the helper function for this to DragController.h/cpp; I don’t care if it’s free function or a static member function of DragController.
2) We get rid of HitTestResult::isLiveLink and have the two callers get the innerURLElement and then call our new function instead. Or have the DragController provide a convenience wrapper that takes a const HitTestResult& as well as the one that takes a const Element&.
3) We come up with a term other than “live link” that is makes clear what this concept is. The word “live” doesn’t obviously map to “is draggable”.
4) We remove WebElementLinkIsLiveKey.
5) As you said, we figure out whether <area> elements should participate.

> Source/WebCore/dom/Element.h:614
> +bool elementIsLiveLink(const Element&);

I think it’s a little strange to put this in Element.h but make it a non-member function. It would be nice to keep this "live link" concept local to the places that already know about it, which would be HitTestResult, HTMLAnchorElement, and DragController. I’d prefer not to add the concept to other source files.

[Daniel Bates]

(In reply to comment #9)
> Comment on attachment 250773 [details]
> Patch and layout test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250773&action=review
> 
> >>> Source/WebCore/dom/Element.cpp:156
> >>> +bool elementIsLiveLink(const Element& element)
> >> 
> >> Should AREA match the definition of "live link"?
> > 
> > We could consider an HTML Area element as a "live link" and show a drag image for it when you drag it. Unless you are feel we should implement such a change in this patch, I suggest we address it in a separate bug.
> 
> I did a bit more research into the definition of this. The concept of "live
> link" seems to be in the drag code, in the code in EventHandler that deals
> with a rule about selecting on mouse down that is related to the drag code,
> and in unused Legacy WebKit1 SPI, WebElementLinkIsLiveKey, which should be
> removed once we are certain since no one depends on it. Searching both
> internally at Apple and on the Internet I can’t find anyone using
> WebElementLinkIsLiveKey.
> 
> Given that, I suggest we move this concept to DragController as much as
> possible rather than having it spread across so many source files. I suggest
> that over time:
> 
> 1) We add the helper function for this to DragController.h/cpp; I don’t care
> if it’s free function or a static member function of DragController.

Will move elementIsLiveLink() from Element.h/cpp to DragController.h/cpp and will rename it to elementIsDraggableLink() to better describe its purpose.

> 2) We get rid of HitTestResult::isLiveLink and have the two callers get the
> innerURLElement and then call our new function instead. Or have the
> DragController provide a convenience wrapper that takes a const
> HitTestResult& as well as the one that takes a const Element&.

I will do the former.

> 3) We come up with a term other than “live link” that is makes clear what
> this concept is. The word “live” doesn’t obviously map to “is draggable”.

As aforementioned I will rename the function from elementIsLiveLink to elementIsDraggableLink.

> 4) We remove WebElementLinkIsLiveKey.

I filed bug #143846 to remove WebElementLinkIsLiveKey. 

> 5) As you said, we figure out whether <area> elements should participate.

I filed bug #143849 to address this issue.

> > Source/WebCore/dom/Element.h:614
> > +bool elementIsLiveLink(const Element&);
> 
> I think it’s a little strange to put this in Element.h but make it a
> non-member function. It would be nice to keep this "live link" concept local
> to the places that already know about it, which would be HitTestResult,
> HTMLAnchorElement, and DragController. I’d prefer not to add the concept to
> other source files.

As aforementioned I will move this function to DragController.h/cpp.

[Daniel Bates]

Created attachment 250953 [details]
Patch and layout test

Updated patch based on Darin Adler's feedback. Also, made the test look pretty in Firefox and IE8.

[Darin Adler]

Comment on attachment 250953 [details]
Patch and layout test

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

r=me but please fix the iOS build

> Source/WebCore/page/DragController.cpp:990
> +bool elementIsDraggableLink(const Element& element)

Strangely, the contents of this whole file is inside ENABLE(DRAG_SUPPORT), although the header file isn’t. This the broke the build on iOS, which seems to expose WebElementLinkIsLiveKey but has !ENABLE(DRAG_SUPPORT). So we compile successfully using the header file, but fail to link.

Simplest fix would be to move this function outside #if ENABLE(DRAG_SUPPORT) as long as this file is built on iOS. Maybe there are other fixes.

> Source/WebCore/page/DragController.cpp:996
> +    // FIXME: Implement support for dragging an HTML Area element. See <https://bugs.webkit.org/show_bug.cgi?id=143849>.

I’m not sure this justifies a FIXME.

> Source/WebCore/page/DragController.h:143
> +    WEBCORE_EXPORT bool elementIsDraggableLink(const Element&);

I don’t think this function needs element in its name. I think just isDraggableLink would be fine.

> Source/WebCore/page/EventHandler.cpp:601
> +    Element* urlElement = result.hitTestResult().URLElement();

The code below inside the if statement was already getting this into a local variable named URLElement. I suggest moving that up rather than doing this twice.

[Daniel Bates]

(In reply to comment #12)
> [...]
> > Source/WebCore/page/DragController.cpp:990
> > +bool elementIsDraggableLink(const Element& element)
> 
> Strangely, the contents of this whole file is inside ENABLE(DRAG_SUPPORT),
> although the header file isn’t. This the broke the build on iOS, which seems
> to expose WebElementLinkIsLiveKey but has !ENABLE(DRAG_SUPPORT). So we
> compile successfully using the header file, but fail to link.

I hope you do not mind that I defer cleaning up WebElementLinkIsLiveKey to bug #143846.

> 
> Simplest fix would be to move this function outside #if ENABLE(DRAG_SUPPORT)
> as long as this file is built on iOS. Maybe there are other fixes.

Will move function outside of #if ENABLE(DRAG_SUPPORT) section of the file before landing.

> 
> > Source/WebCore/page/DragController.cpp:996
> > +    // FIXME: Implement support for dragging an HTML Area element. See <https://bugs.webkit.org/show_bug.cgi?id=143849>.
> 
> I’m not sure this justifies a FIXME.
> 

Will remove FIXME comment before landing.

> > Source/WebCore/page/DragController.h:143
> > +    WEBCORE_EXPORT bool elementIsDraggableLink(const Element&);
> 
> I don’t think this function needs element in its name. I think just
> isDraggableLink would be fine.
> 

Will rename function from elementIsDraggableLink() to isDraggableLink() before landing.

> > Source/WebCore/page/EventHandler.cpp:601
> > +    Element* urlElement = result.hitTestResult().URLElement();
> 
> The code below inside the if statement was already getting this into a local
> variable named URLElement. I suggest moving that up rather than doing this
> twice.

Will keep the variable urlElement and remove the variable URLElement because the former conforms to the naming convention for variables that begin with an acronym per <http://www.webkit.org/coding/coding-style.html#names-basic>. I will substitute urlElement for URLElement throughout this function.

[Daniel Bates]

Committed r182957: <http://trac.webkit.org/changeset/182957>