583775 - Use HTMLWhitespace to trim whitespace for script event handlers attributes

Status: NEW

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

IsHTMLWhitespace is checking for HTML5 whitespace which includes more characters  than IsAsciiSpace.
Very likely, TrimWhitespace<nsCRT::IsAsciiSpace> callers want to call TrimWhitespace<nsContentUtils::IsHTMLWhitespace>.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Might be a good time to make nsContentUtils::TrimCharsInSet consumers use
TrimWhitespace instead.

Comment 2

[Mounir Lamouri (:mounir)]

If you want to work on that, feel free to contact me on IRC (nick: volkmar) or by email.

Comment 3

[tsurdor]

Attachment: changed rellevent locations on for IsAscciSpace

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 537379 [details] [diff] [review]
changed rellevent locations on for IsAscciSpace

Review of attachment 537379 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with the changes you did but why didn't you change this call:
https://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMDataTransfer.cpp#330

f=me for the moment. Please ask back for a review if you didn't change the call I pointed for good reasons or with a new patch.

Comment 5

[Mounir Lamouri (:mounir)]

Changing the assignee given that we got a patch from someone else.
Chris, feel free to email me if you want to work on something else.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #4)
> Comment on attachment 537379 [details] [diff] [review] [review]
> changed rellevent locations on for IsAscciSpace
> 
> Review of attachment 537379 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> I agree with the changes you did but why didn't you change this call:
> https://mxr.mozilla.org/mozilla-central/source/content/events/src/
> nsDOMDataTransfer.cpp#330
> 
> f=me for the moment. Please ask back for a review if you didn't change the
> call I pointed for good reasons or with a new patch.

You can blame me for that one. Without a spec, I'd rather not change behaviour here.

Note that the change to ProcessViewportToken in nsContentUtils.cpp is wrong per <http://dev.w3.org/csswg/css-device-adapt/#parsing-algorithm>. The change in nsScriptLoader.cpp is correct.

In any case, this needs tests.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> You can blame me for that one. Without a spec, I'd rather not change
> behaviour here.

If the spec say spacing characters should be trimmed, it will be HTML5 definition of spacing characters so we will have to change our code. IMO, it would be better to change this code now.

> Note that the change to ProcessViewportToken in nsContentUtils.cpp is wrong
> per <http://dev.w3.org/csswg/css-device-adapt/#parsing-algorithm>. The
> change in nsScriptLoader.cpp is correct.

Indeed :)

> In any case, this needs tests.

I agree.

Comment 8

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Use HTMLWhitespace to trim script event handler attributes

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 792736 [details] [diff] [review]
Use HTMLWhitespace to trim script event handler attributes

Review of attachment 792736 [details] [diff] [review]:
-----------------------------------------------------------------

Missing a commit message. This *should* make us pass some imported tests; don't forget to update the expected failures.

::: content/html/content/test/test_script_event_handler.html
@@ +12,5 @@
> +    var executed = 0;
> +  </script>
> +  <script>
> +    executed++;
> +    ok(true, "should be executed");

The ok(true) ones aren't terribly useful. Fine either way.

@@ +65,5 @@
> +    executed++;
> +    ok(true, "should be executed");
> +  </script>
> +  <script>
> +    is(executed, 9, "should got 9 executions");

"should get", if you think that's useful

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.