Support activation behavior of link element

• Component: New Bugs
• Status: RESOLVED FIXED

[Dhi Aurrahman]

Support activation behavior of link element

[Dhi Aurrahman]

Created attachment 239605 [details]
Patch

[Benjamin Poulain]

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...

[Dhi Aurrahman]

(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!

[Benjamin Poulain]

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?

[Dhi Aurrahman]

Created attachment 239683 [details]
Patch

[Benjamin Poulain]

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.

[Chris Dumez]

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?

[Dhi Aurrahman]

Created attachment 239696 [details]
Patch

[Chris Dumez]

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.

[Dhi Aurrahman]

Created attachment 239698 [details]
Patch

[Dhi Aurrahman]

Created attachment 239699 [details]
Patch

[Chris Dumez]

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.

[Benjamin Poulain]

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.

[WebKit Commit Bot]

Comment on attachment 239699 [details]
Patch

Clearing flags on attachment: 239699

Committed r174637: <http://trac.webkit.org/changeset/174637>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dhi Aurrahman]

(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!

[Alexey Proskuryakov]

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

[Benjamin Poulain]

(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.

[Alexey Proskuryakov]

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.

[Brady Eidson]

Oh boy, do I ever have questions about this patch.

[Brady Eidson]

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?