use return value of invoking event handlers to cancel the event

[jxs]

Fixes #8490.

@jdm having some questions on the beforeunload event: how can i check if return_value is null?
shoud i also check if "If the Event object E is a BeforeUnloadEvent object, and the Event object E's returnValue attribute's value is the empty string, then set the returnValue attribute's value to return value." ?

thanks!

[jdm]

We don't implement OnBeforeUnloadEventHandler or BeforeUnloadEvent yet; adding the new event interface is straightforward, but adding support for the special event handler will require work similar to #8430. I think doing that in a separate PR would make sense (or a separate commit in this one, if you'd like!), so it could just be a TODO in this one. As for checking whether it's null, there's an is_null_or_undefined method on JSVal: https://github.com/servo/rust-mozjs/blob/master/src/jsval.rs#L219

[jxs]

ok thanks, updated! Yeah i agree on a new PR so that this one can be closed first, what do you think?

[jdm]

That works for me!

[Ms2ger]

is_null_or_undefined does not sound correct; checking for null can be done with the aptly named is_null method.

It may be the case that the return value is never undefined here, but I don't think so. There's a similar issue that the return value is not necessarily a boolean in the other match arms. I would like to see tests for that; if you don't feel up for it, please let me know so I can write them.

Also, there's the problem of

/home/travis/build/servo/servo/components/script/dom/eventtarget.rs:150:35: 150:46 error: no rules expected the token `"mouseover"`
/home/travis/build/servo/servo/components/script/dom/eventtarget.rs:150                             atom!("mouseover") => {

which I assume means we need to add it to the list in the string-cache repo.

[eefriedman]

which I assume means we need to add it to the list in the string-cache repo.

Already done, and landed in Servo; this PR just needs to be rebased.

[jxs]

ok, updated to is_null and rebased

[jdm]

Whoops, we need to update the code to account for #8710 now.

[jxs]

ok, updated!

[jdm]

Sorry for the delay!
-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

components/script/dom/eventtarget.rs, line 130 [r1] (raw file):
We should use let return_value = RootedValue::new(cx, handler.Call_(object, or it's a GC hazard.

---

components/script/dom/eventtarget.rs, line 131 [r1] (raw file):
nit: these lines should be indented to match object on the previous line.

---

components/script/dom/eventtarget.rs, line 137 [r1] (raw file):
It's cleaner to use if let Ok(value) = return_value here, and we'll need the check to be value.is_boolean() && value.to_boolean() == true or we'll panic if an event handler returns a non-boolean value.

---

components/script/dom/eventtarget.rs, line 148 [r1] (raw file):
Same comment about RootedValue here.

---

components/script/dom/eventtarget.rs, line 149 [r1] (raw file):
Let's add a // Step 4 comment here.

---

components/script/dom/eventtarget.rs, line 151 [r1] (raw file):
Same as previous (if let and is_boolean).

---

components/script/dom/eventtarget.rs, line 152 [r1] (raw file):
nit: indent by four spaces

---

components/script/dom/eventtarget.rs, line 156 [r1] (raw file):
if let

---

components/script/dom/eventtarget.rs, line 161 [r1] (raw file):
if let and is_boolean.

---

components/script/dom/eventtarget.rs, line 171 [r1] (raw file):
We can remove this now.

---

Comments from the review on Reviewable.io

[jdm]

Don't forget to run ./mach test-tidy after addressing these review comments!
-S-awaiting-review +S-fails-tidy +S-needs-code-changes

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

components/script/dom/eventtarget.rs, line 130 [r1] (raw file):
Ah, sorry, I forgot about the Result<> that Call_ returns. We'll need

let return_value = handler.Call_(object, ...);
if let Some(return_value) = return_value {
    let return_value = RootedValue::new(cx, return_value);
    ...
}

---

components/script/dom/eventtarget.rs, line 156 [r1] (raw file):
This doesn't compile.

---

components/script/dom/eventtarget.rs, line 139 [r2] (raw file):
This doesn't compile, and doesn't use is_boolean. We'll need

if let Ok(return_value) = return_value {
    ...
    if return_value.is_boolean() && return_value.to_boolean() == true {
        ...
    }
}

---

components/script/dom/eventtarget.rs, line 149 [r2] (raw file):
This belongs above the match event.type_() line.

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

tests/wpt/web-platform-tests/html/webappapis/scripting/events/event-handler-processing-algorithm.html, line 15 [r5] (raw file):
I'm pretty sure if we run this test it won't pass now - the actual listener is still returning true.

---

tests/wpt/web-platform-tests/html/webappapis/scripting/events/event-handler-processing-algorithm.html, line 25 [r5] (raw file):
Same as previous.

---

Comments from the review on Reviewable.io

[bors-servo]

☔ The latest upstream changes (presumably #8903) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

The changes here appear to have reverted to an earlier revision that didn't build.
-S-awaiting-review +S-needs-code-changes -S-needs-rebase

---

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[bors-servo]

☔ The latest upstream changes (presumably #9062) made this pull request unmergeable. Please resolve the merge conflicts.

[KiChjang]

@jxs Are you still working on this?

[jdm]

This is totally waiting on me. The spec issue I filed doesn't really have resolution yet.

[jdm]

Rebased in #9755.