Bug 137597 - Support activation behavior of link element
Summary: Support activation behavior of link element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-10 00:19 PDT by Dhi Aurrahman
Modified: 2015-06-01 11:05 PDT (History)
12 users (show)

See Also:


Attachments
Patch (10.54 KB, patch)
2014-10-10 00:29 PDT, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (16.37 KB, patch)
2014-10-11 13:35 PDT, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (16.02 KB, patch)
2014-10-11 20:58 PDT, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (16.07 KB, patch)
2014-10-11 21:56 PDT, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (16.07 KB, patch)
2014-10-11 22:06 PDT, Dhi Aurrahman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dhi Aurrahman 2014-10-10 00:19:36 PDT
Support activation behavior of link element
Comment 1 Dhi Aurrahman 2014-10-10 00:29:41 PDT
Created attachment 239605 [details]
Patch
Comment 2 Benjamin Poulain 2014-10-10 22:27:09 PDT
Comment on attachment 239605 [details]
Patch

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

The patch is great. The changelogs are great. The tests are really well done.

r- because of the duplicated isLinkClick() in HTMLLinkElement and HTMLAnchorElement. Otherwise this is nearly perfect.

> Source/WebCore/html/HTMLLinkElement.cpp:413
> +bool HTMLLinkElement::isLinkClick(Event* event)
> +{
> +    return event->type() == eventNames().clickEvent && (!is<MouseEvent>(*event) || downcast<MouseEvent>(*event).button() != RightButton);
> +}

Since this code is duplicated in HTMLAnchorElement, we should put it in a common place.

I suggest adding a static function "isRightClick" to the class MouseEvent. Then modify HTMLAnchorElement and HTMLLinkElement to use that function.

> Source/WebCore/html/HTMLLinkElement.cpp:420
> +    if (url.isNull())
> +        return;

I believe this is an interesting case, there should be a test for it if possible.

Maybe:
1) Create a click event with document.createEvent.
2) Send it to the <link> element with dispatchEvent()
3) Verify that the event has its default handled.
4) Set a time to 100ms. If the document has not navigated anywhere after the 100ms, finish the test with success.

This might even work when opened on Firefox...
Comment 3 Dhi Aurrahman 2014-10-11 07:06:52 PDT
(In reply to comment #2)
> (From update of attachment 239605 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239605&action=review
> 
> The patch is great. The changelogs are great. The tests are really well done.
> 
> r- because of the duplicated isLinkClick() in HTMLLinkElement and HTMLAnchorElement. Otherwise this is nearly perfect.
> 
> > Source/WebCore/html/HTMLLinkElement.cpp:413
> > +bool HTMLLinkElement::isLinkClick(Event* event)
> > +{
> > +    return event->type() == eventNames().clickEvent && (!is<MouseEvent>(*event) || downcast<MouseEvent>(*event).button() != RightButton);
> > +}
> 
> Since this code is duplicated in HTMLAnchorElement, we should put it in a common place.
> 
> I suggest adding a static function "isRightClick" to the class MouseEvent. Then modify HTMLAnchorElement and HTMLLinkElement to use that function.

I'm not sure with adding a static function "isRightClick", since the current "isLinkClick" i.e.

return event->type() == eventNames().clickEvent && (!is<MouseEvent>(*event) || !downcast<MouseEvent>(*event).button() != RightButton);

`true` if:

1. It is a click event and not a MouseEvent (it doesn't care about the button, since it is not a MouseEvent)
2. It is a click event and a MouseEvent and the clicked button is not RightButton.

I can however create a bool MouseEvent::isRightClick() as follows,

bool MouseEvent::isRightClick()
{
    return type() == eventNames().clickEvent && button() == RightButton;
}

But it won't help to much on HTMLLinkElement::isLinkClick(Event *) and HTMLAnchorElement::isLinkClick(Event *)

bool isLinkClick(Event* event)
{
    return event->type() == eventNames().clickEvent && (!is<MouseEvent>(*event) || !downcast<MouseEvent>(*event).isRightClick());
}

Am I missing something here?

> 
> > Source/WebCore/html/HTMLLinkElement.cpp:420
> > +    if (url.isNull())
> > +        return;
> 
> I believe this is an interesting case, there should be a test for it if possible.
> 
> Maybe:
> 1) Create a click event with document.createEvent.
> 2) Send it to the <link> element with dispatchEvent()
> 3) Verify that the event has its default handled.
> 4) Set a time to 100ms. If the document has not navigated anywhere after the 100ms, finish the test with success.
> 
> This might even work when opened on Firefox...

I'm ready with the test (https://gist.github.com/diorahman/b1c2da24d44da2648c36), but I have to confirm the "isRightClick" first, before sending another patch

Thanks!
Comment 4 Benjamin Poulain 2014-10-11 11:32:19 PDT
Oops, my bad, isRightClick is indeed wrong.

Let's think about this, what would be a good name for a click event that can either be synthetic or come from the mouse...

Maybe triggersActivationBehavior(Event*) would be a good name, what do you think?
Comment 5 Dhi Aurrahman 2014-10-11 13:35:40 PDT
Created attachment 239683 [details]
Patch
Comment 6 Benjamin Poulain 2014-10-11 16:59:09 PDT
Comment on attachment 239683 [details]
Patch

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

The page is good, the additional test looks good too.

I have a minor comment. I r+, cq-, which means the patch is good but it cannot be integrated as it is because of minor review comments.
If you upload a new updated patch, I'll cq+ that one. If I get the chance tonight I'll just update this version and land manually.

Great job on this bug.

> Source/WebCore/dom/MouseEvent.cpp:193
> +bool MouseEvent::triggerActivationBehavior(const Event* event)

Seeing this being used in practice, the name does not look right. The function name implies it triggers the Activation Behavior.

I think canTriggerActivationBehavior() would be better.

> Source/WebCore/dom/MouseEvent.h:102
> +    bool static triggerActivationBehavior(const Event*); 

Should be "static bool" instead of "bool static" for WebKit style.

> Source/WebCore/html/HTMLAnchorElement.h:-124
> -

No need to change this line.
Comment 7 Chris Dumez 2014-10-11 17:31:57 PDT
Comment on attachment 239683 [details]
Patch

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

>> Source/WebCore/dom/MouseEvent.cpp:193
>> +bool MouseEvent::triggerActivationBehavior(const Event* event)
> 
> Seeing this being used in practice, the name does not look right. The function name implies it triggers the Activation Behavior.
> 
> I think canTriggerActivationBehavior() would be better.

The argument should probably be a const Event& since it is expected to be non-null.

> Source/WebCore/html/HTMLLinkElement.cpp:410
> +void HTMLLinkElement::handleClick(Event* event)

Ditto. Argument could be an Event&

> Source/WebCore/html/HTMLLinkElement.cpp:420
> +    return;

Please omit this return.

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-element-click-expected.txt:9
> +PASS event.data is 'test:ok'

This should appear before the TEST COMPLETE, something is likely wrong with the test?

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-element-click.html:15
> +	shouldBe("event.data", "'test:ok'")

shouldBeEqualToString()

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-element-click.html:16
> +	if (window.testRunner)

finishJSTest();

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-element-click.html:22
> +if (window.testRunner) 

should be window.jsTestIsAsync = true;

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-mouse-click-expected.txt:9
> +PASS event.data is 'test:ok'

The fact that the PASS is after the TEST COMPLETE is a bad sign. There is likely something wrong with the test and there is a fair chance it will be flaky.

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-mouse-click.html:15
> +	shouldBe("event.data", "'test:ok'")

shouldBeEqualToString()

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-mouse-click.html:16
> +	if (window.testRunner)

finishJSTest();

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-mouse-click.html:22
> +if (window.testRunner) 

should be window.jsTestIsAsync = true;

> LayoutTests/fast/dom/html-link-element-activation-behavior-url-is-null.html:11
> +	if (window.testRunner)

should be a finishJSTest();

> LayoutTests/fast/dom/html-link-element-activation-behavior-url-is-null.html:16
> +shouldBe("target.dispatchEvent(mouseEvent)", "true");

shouldBeTrue()

> LayoutTests/fast/dom/html-link-element-activation-behavior-url-is-null.html:18
> +if (window.testRunner) 

should be window.jsTestIsAsync = true;

> LayoutTests/fast/dom/resources/html-link-element-activation-behavior-on-element-click-step1.html:15
> +		document.getElementById("target").click();

weird indentation

> LayoutTests/fast/dom/resources/html-link-element-activation-behavior-target.html:4
> +		window.parent.postMessage("test:ok", "*");

Weird indentation here. BTW, are you using tabs instead of spaces?
Comment 8 Dhi Aurrahman 2014-10-11 20:58:02 PDT
Created attachment 239696 [details]
Patch
Comment 9 Chris Dumez 2014-10-11 21:38:40 PDT
Comment on attachment 239696 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:402
> +    if (MouseEvent::canTriggerActivationBehavior(*event)) {

nit: We should probably add an ASSERT(event); at the beginning of this function.

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-element-click-expected.txt:6
> +FAIL event.data should be 'test:ok'. Was test:ok.

uh oh..

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-element-click.html:15
> +  shouldBeEqualToString("event.data", "'test:ok'")

Remove the single quotes around test:ok

> LayoutTests/fast/dom/html-link-element-activation-behavior-on-mouse-click.html:15
> +  shouldBe("event.data", "'test:ok'")

shouldBeEqualToString()

> LayoutTests/fast/dom/html-link-element-activation-behavior-url-is-null.html:15
> +shouldBe("target.dispatchEvent(mouseEvent)", "true");

shouldBeTrue()

> LayoutTests/fast/dom/resources/html-link-element-activation-behavior-target.html:4
> +  window.parent.postMessage("test:ok", "*");

nit: I think we use 4-space indentation in our JS usually.
Comment 10 Dhi Aurrahman 2014-10-11 21:56:57 PDT
Created attachment 239698 [details]
Patch
Comment 11 Dhi Aurrahman 2014-10-11 22:06:29 PDT
Created attachment 239699 [details]
Patch
Comment 12 Chris Dumez 2014-10-11 23:19:57 PDT
Comment on attachment 239699 [details]
Patch

Seems fine, thanks. I'll let Benjamin take a final look and land it as he already r+'d it.
Comment 13 Benjamin Poulain 2014-10-12 00:59:20 PDT
Comment on attachment 239699 [details]
Patch

This last update looks good to me too. Let's land it!

Thank you for your help on the review Christophe.
Comment 14 WebKit Commit Bot 2014-10-12 01:35:37 PDT
Comment on attachment 239699 [details]
Patch

Clearing flags on attachment: 239699

Committed r174637: <http://trac.webkit.org/changeset/174637>
Comment 15 WebKit Commit Bot 2014-10-12 01:35:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Dhi Aurrahman 2014-10-12 02:50:27 PDT
(In reply to comment #13)
> (From update of attachment 239699 [details])
> This last update looks good to me too. Let's land it!
> 
> Thank you for your help on the review Christophe.

Thank you Benjamin and Christophe! I have learned a lot from this process. Thank you. Thank you!
Comment 17 Alexey Proskuryakov 2014-10-14 13:46:59 PDT
This patch made fast/dom/remove-body-during-body-replacement2.html very flaky on Mac - it always fails on first try, and then passes on retry.

This may mean that some state leaks from a previous test that breaks this one.

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2Fremove-body-during-body-replacement2.html
Comment 18 Benjamin Poulain 2014-10-14 14:03:07 PDT
(In reply to comment #17)
> This patch made fast/dom/remove-body-during-body-replacement2.html very flaky on Mac - it always fails on first try, and then passes on retry.
> 
> This may mean that some state leaks from a previous test that breaks this one.
> 
> http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2Fremove-body-during-body-replacement2.html

Those tests look completely unrelated. Maybe a bug in frameloader.

Are you able to reproduce the flakiness locally? I just tried running fast/dom in debug and release and everything was fine.
Comment 19 Alexey Proskuryakov 2015-05-30 18:23:58 PDT
Comment on attachment 239699 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:419
> +    frame->loader().urlSelected(url, target(), PassRefPtr<Event>(&event), LockHistory::No, LockBackForwardList::No, MaybeSendReferrer);

Creating a PassRefPtr from a reference does not make sense.

We should eliminate PassRefPtr<Event> from FrameLoader, and just use pointers. There isn't transfer of ownership, and the code is not performance sensitive in the first place.
Comment 20 Brady Eidson 2015-06-01 11:00:55 PDT
Oh boy, do I ever have questions about this patch.
Comment 21 Brady Eidson 2015-06-01 11:05:27 PDT
Two primary questions for now:

1 - HTMLLinkElement, when clicked, handles the target attribute the same way HTMLAnchorElement does. But the spec makes absolutely no mention of the target attribute, or what it means in the context of a clickable link element. This seems wrong.

2 - HTMLAnchorElement does a lot more stuff in its click handler that HTMLLinkElement does *not* do. One example is running stripLeadingAndTrailingHTMLSpaces on the url string. Another seemingly critical example is running document().completeURL() on the string.

Why is it okay to do that differently?