1160342 - Implement <marquee> using mutation observers

Status: RESOLVED FIXED

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

Description

[Christopher Van Wiemeersch [:cvan]]

Attachment: test case

1. Load a page with a <marquee> tag.
2. Notice a JS warning appears in the Console:

> Use of Mutation Events is deprecated. Use MutationObserver instead.

–

The <marquee> tag should likely be reimplemented using mutation observers.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Martijn, want to fix this?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(though, it is not clear to me MutationObservers are synchronous enough for this.)

Comment 3

[Martijn Wargers (dead)]

I'll take a look at it later, if someone doesn't beat me to it.

Comment 4

[Martijn Wargers (dead)]

Attachment: 1160342_marquee.html

It looks like MutationObservers are slow, something like this returns 'scrollalternate' with DOMAttrModified (which is correct) and 'scrollscroll' with MutationObservers for marquees, which is incorrect.

I'm not really sure if this would be an issue with real pages on real sites.
There is no way to speed up MutationObservers, is there?

Or we could overload setAttribute/removeAttribute et all, but that was shot down in bug 239919.

Comment 5

[Martijn Wargers (dead)]

Attachment: all.diff

Fwiw, I have something like this for now.

Comment 6

[Martijn Wargers (dead)]

Ok, it might be that .behavior should just reflect the behavior attribute or I need to fix the behavior property.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> It looks like MutationObservers are slow, something like this returns
> 'scrollalternate' with DOMAttrModified (which is correct) and 'scrollscroll'
> with MutationObservers for marquees, which is incorrect.
> I'm not really sure if this would be an issue with real pages on real sites.
> There is no way to speed up MutationObservers, is there?
MutationObservers are, last time I checked, 7x faster than MutationEvents ;)
But they aren't synchronous. The callback is called at the end of a microtask.
See comment 2.

Comment 8

[Martijn Wargers (dead)]

Attachment: 1160342.diff

Something like this, I guess. The mochitest handles various cases, the todo_is is basically what IE is doing, but what Mozilla currently isn't doing with the patch. Although, I'm wondering if it's really worth the effort to mimick IE in all cases here.

Does this need to go through mozreview or something?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(nothing _needs_ to go through mozreview. Some people prefer it, so people prefer the old style.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8719322 [details] [diff] [review]
1160342.diff

>         <setter>
>-          this.setAttribute("scrollamount", val);
>+          if (isNaN(val) || val == null)
>+            val = 0;
>+          if (!this._set_scrollAmount(val)) {
>+            throw new Error("Invalid argument for Marquee::scrollAmount");
>+          } else {
>+            this.setAttribute("scrollamount", val);
>+          }
>         </setter>
Making web exposed setters to throw is backwards incompatible change and should be done in a separate bug.
(and please test what other browsers do)


So waiting to see a patch which has just the minimal MutationObserver related changes.

Comment 11

[Martijn Wargers (dead)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #10)
> Making web exposed setters to throw is backwards incompatible change and
> should be done in a separate bug.
> (and please test what other browsers do)

I think this got broken with the fix for bug 848939, but you're right, this is currently not working, so I guess it's bettor to remove it for now.
I did test what happens in Internet Explorer. Internet Explorer is passing all the tests in the mochitest.
Google Chrome is failing in 21 of the tests.

> So waiting to see a patch which has just the minimal MutationObserver
> related changes.

Ok, will do, I'll just make the tests todo_is() for the cases where it's not doing the same as Internet Explorer.
Ok, will do.

Comment 12

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #11)

> I think this got broken with the fix for bug 848939, but you're right, this
> is currently not working, so I guess it's bettor to remove it for now.
> I did test what happens in Internet Explorer. Internet Explorer is passing
> all the tests in the mochitest.
> Google Chrome is failing in 21 of the tests.

Here is an online version of the mochitest, btw: http://people.mozilla.org/~mwargers/mochitestjs2/test_bug1160342_marquee.html

Comment 13

[Martijn Wargers (dead)]

Attachment: 1160342_marquee.diff

Updated patch with the throwing error part removed. Also added a fix and some tests for the direction property.

What is still needed is more testing of direction=up/down, onbounce, onstart, onfinish. Also, if the actual properties work as advertised.
Also, the mutationObserver could also listen for the relevant attributes in question (although I don't know how that works regarding case sensitivity).

Or perhaps you only want to have this handle the minimal MutationObserver changes?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

If we can keep the existing behavior with the uploaded patch, great.
Changes to behavior could be implemented in followup bugs.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

So looks like the patch does add plenty of non-mutationobserver related fixes too.
I really would like to have first just the change to start using mutationobserver.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8720041 [details] [diff] [review]
1160342_marquee.diff

So could you split the non-mutationobserver related stuff to a separate patch/bug.
I'm talking about the changes to various getters and setters and so.
And change to _set_scrollDelay.

So I would expect to see a patch which has just those couple of 
this._mutationActor(this._mutationObserver.takeRecords()); calls and
<method name="_mutationActor">...</method> and the change to constructor.
(or is there a reason to have all the other changes in this bug?)

Sorry being a bit difficult here, but
this all mainly to ease tracking possible regressions.

Comment 17

[Martijn Wargers (dead)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a33e0f1020e

Comment 18

[Martijn Wargers (dead)]

Attachment: 1160342_marquee.diff

Ok, this keeps existing attribute/property handling of marquees.
Wit this patch, the results are the same in the mochitest as we have currently (which is not correct in many ways).
Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a33e0f1020e

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8720242 [details] [diff] [review]
1160342_marquee.diff

Still don't know why we need those two parseInt calls, which seem like unrelated changes.
But fine.

Comment 20

[Martijn Wargers (dead)]

Because I would otherwise get differences in results in the mochitest.

Comment 21

[Martijn Wargers (dead)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d2b84dcf81

Comment 22

[Martijn Wargers (dead)]

Attachment: final patch (for checkin)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3efe856a27a9

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3efe856a27a9