To get one step closer to removing XBL in <marquee>, we could create an HTMLMarqueeElement definition and update the parser to construct this new element instead of an HTMLDivElement. This would have the benefits of: 1) Simplify HTMLDivElement implementation 2) Remove the instances of `<property exposeToUntrustedContent="true">` in marquee, which appear to be the only non-test instances of that attribute on a XBL property tag in tree: (https://searchfox.org/mozilla-central/search?q=exposetountrustedcontent) 3) Prepare marquee to be fully implemented in C++ instead of XBL (using CSS transitions + some kind of User Agent Shadow DOM not visible to the page) 4) Bring <marquee> more up to spec (https://wpt.fyi/html/obsolete/requirements-for-implementations/the-marquee-element-0)

Incrementalism ftw :-)

bz: this changes the behavior of some of the properties on <marquee>, and I'd like some guidance on if I should try to make things work how they used to in Firefox or just go with what seems to be the default behavior for webidl. Page 1: data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.scrollAmount = "foo"; console.log(m.scrollAmount, m.getAttribute("scrollamount"))</script> |----------------------------------------------------------------------------| | | m.scrollAmount | m.getAttribute("scrollAmount") | |----------------------------------------------------------------------------| |Fx (without patch) | foo | null | |Fx (with patch) | 0 | "0" | |Chrome | 0 | "0" | |----------------------------------------------------------------------------| Page 2: data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.setAttribute("scrollAmount", "foo"); console.log(m.scrollAmount, m.getAttribute("scrollamount"))</script> |----------------------------------------------------------------------------| | | m.scrollAmount | m.getAttribute("scrollAmount") | |----------------------------------------------------------------------------| |Fx (without patch) | undefined | "foo" | |Fx (with patch) | 6 | "foo" | |Chrome | 6 | "foo" | |----------------------------------------------------------------------------| Page 3: data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.setAttribute("scrollAmount", "-1"); console.log(m.scrollAmount, m.getAttribute("scrollamount"))</script> |----------------------------------------------------------------------------| | | m.scrollAmount | m.getAttribute("scrollAmount") | |----------------------------------------------------------------------------| |Fx (without patch) | undefined | "-1" | |Fx (with patch) | 6 | "-1" | |Chrome | 6 | "-1" | |----------------------------------------------------------------------------| In these cases, we are aligning with Chrome and at least more closely with the spec as most of https://w3c-test.org/html/obsolete/requirements-for-implementations/the-marquee-element-0 is passing. There's a similar situation with the 'scrollDelay' and 'loop' properties which are each numbers with non-0 default values as well. Also 'behavior' and 'direction' properties are changed to use ParseEnumValue instead of the custom XBL impl (I haven't gone through to test all of the variations of that yet). I'd specifically like to know: 1) Should we change the behavior to more closely align with what Firefox currently does instead of potentially breaking existing consumers that rely on the outdated behavior 2) Does the new behavior / code seem to match your reading of the spec at https://www.w3.org/TR/html5/obsolete.html#the-marquee-element? Those platform tests are pretty sparse..

> Page 1: data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.scrollAmount = "foo"; console.log(m.scrollAmount, m.getAttribute("scrollamount"))</script> > > |Fx (without patch) | foo | null | While true, this modification: Page 1a: data:text/html,<marquee>hi</marquee><script>onload = function() { var m = document.querySelector("marquee"); m.scrollAmount = "foo"; console.log(m.scrollAmount, m.getAttribute("scrollamount")) }</script> gives 0 and "0". Presumably in "Page 1" the code is running before the binding has been attached (e.g. before we've done frame construction for the <marquee>, which is in fact quite likely)... So the risk of a change here is lower than "Page 1" suggests. > Page 2: data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.setAttribute("scrollAmount", "foo"); console.log(m.scrollAmount, m.getAttribute("scrollamount"))</script> > > |Fx (without patch) | undefined | "foo" | Again, page 2a: data:text/html,<marquee>hi</marquee><script>onload = function() { var m = document.querySelector("marquee"); m.setAttribute("scrollAmount", "foo"); console.log(m.scrollAmount, m.getAttribute("scrollamount")) }</script> which gives 6 and "foo". Similarly, page 3a: data:text/html,<marquee>hi</marquee><script>onload = function() { var m = document.querySelector("marquee"); m.setAttribute("scrollAmount", "-1"); console.log(m.scrollAmount, m.getAttribute("scrollamount")) }</script> gives 6 and "-1" in Firefox without the patches in this bug. > 1) Should we change the behavior to more closely align with what Firefox currently does instead of potentially breaking existing consumers that rely on the outdated behavior I think what Firefox currently does aims to match the spec, and just fails in some cases because binding attachment is not sync with DOM mutation. Aligning with the spec in all cases seems fine. > 2) Does the new behavior / code seem to match your reading of the spec at https://www.w3.org/TR/html5/obsolete.html#the-marquee-element? First, the relevant spec is <https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element>. This isn't just idle nitpicking; the w3c fork is not maintained very well and is buggier. As far as implementation, I skimmed it sort of quickly, but it looked reasonable to me for the most part. One issue I see is that the setters that end up calling Set*Attr should probably be marked [Throws] in the webidl and pass through the ErrorResult they get. To answer the XXX question about SetLoop, you can't tell apart 0 and NaN. They both become 0 in the binding layer. Per spec, any values that are not > 0 or -1 should be ignored (and we should have tests for this if we don't have them already; the attached patch seems to get it wrong). There are other complications around SetLoop as well (e.g. it not doing the attr set if the _parsed_ value of the attribute matches the passed-in integer). It does _not_ just reflect the content attribute in the spec, unlike the other DOM attributes. > Those platform tests are pretty sparse.. We have a setup for writing pretty exhaustive reflection tests in mochitest (which we should consider upstreaming into wpt). See https://searchfox.org/mozilla-central/source/dom/html/test/forms/test_input_attributes_reflection.html (the part where it makes a bunch if reflect* calls, passing in an element and an attribute name). Might be worth adding a test for marquee using that infrastructure.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7) > As far as implementation, I skimmed it sort of quickly, but it looked > reasonable to me for the most part. One issue I see is that the setters > that end up calling Set*Attr should probably be marked [Throws] in the > webidl and pass through the ErrorResult they get. The webidl at https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element doesn't have [Throws] on any of the attributes - is that something we can/should add ourselves?

FWIW chrome does throw on `loop` but nothing else: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMarqueeElement.idl?l=28. I.e. this throws: `data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.loop = "baz";</script>` but this doesn't: `data:text/html,<marquee>hi</marquee><script>var m = document.querySelector("marquee"); m.scrollAmount = "foo";</script>`

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7) > While true, this modification: > > Page 1a: data:text/html,<marquee>hi</marquee><script>onload = function() { > var m = document.querySelector("marquee"); m.scrollAmount = "foo"; > console.log(m.scrollAmount, m.getAttribute("scrollamount")) }</script> > > gives 0 and "0". Presumably in "Page 1" the code is running before the > binding has been attached (e.g. before we've done frame construction for the > <marquee>, which is in fact quite likely)... So the risk of a change here > is lower than "Page 1" suggests. Ah good catch, I didn't realize this. There are definitely cases where we still differ even after being bound though. For instance, we ignore invalid values if there was a previous good one set (instead of resetting to the default): data:text/html,<marquee>hi</marquee><script>onload = function() { var m = document.querySelector("marquee"); m.scrollAmount = 1; m.scrollAmount = -1; console.log(m.scrollAmount, m.getAttribute("scrollamount")) }</script> |----------------------------------------------------------------------------| | | m.scrollAmount | m.getAttribute("scrollAmount") | |----------------------------------------------------------------------------| |Fx (without patch) | 1 | "1" | |Fx (with patch) | 6 | "6" | |Chrome | 6 | "6" | |----------------------------------------------------------------------------| Other similar examples are shown in the changes to dom/tests/mochitest/bugs/test_bug1160342_marquee.html. Luckily we have this test here which is pretty extensive and can let us review these changes one by one and see if any are unacceptable.

Some text I see in the whatwg spec that looks like we may need extra handling is: > The loop IDL attribute, on getting, must return the element's marquee loop count; and on setting, if the new value is different than the element's marquee loop count and either greater than zero or equal to −1, must set the element's loop content attribute (adding it if necessary) to the valid integer that represents the new value. (Other values are ignored.) The part about "(Other values are ignored.)" makes me think if you do `m.loop = 5; m.loop = -2` then `m.loop` should still = 5. With the current patch it is reset to -1 and in chrome it throws: With `data:text/html,<marquee>hi</marquee><script>onload = function() { var m = document.querySelector("marquee"); m.loop = 5; m.loop = -2; console.log(m.loop, m.getAttribute("loop")) }</script>` |-------------------------------------------------------------| | | m.loop | m.getAttribute("loop") | |-------------------------------------------------------------| |Fx (without patch) | 5 | "5" | |Fx (with patch) | -1 | "-2" | |Chrome | throws | throws | |-------------------------------------------------------------| I *think* the right thing to do would be: |-------------------------------------------------------------| | | m.loop | m.getAttribute("loop") | |-------------------------------------------------------------| | | 5 | "-2" | |-------------------------------------------------------------|

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7) > > Those platform tests are pretty sparse.. > > We have a setup for writing pretty exhaustive reflection tests in mochitest > (which we should consider upstreaming into wpt). See > https://searchfox.org/mozilla-central/source/dom/html/test/forms/ > test_input_attributes_reflection.html (the part where it makes a bunch if > reflect* calls, passing in an element and an attribute name). Might be > worth adding a test for marquee using that infrastructure. Ah, it looks like we already have reflection tests in wpt (which are currently failing with the patch applied [0]). I believe the tests will need updates, i.e. behavior and direction are not strings but enums: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/dom/elements-obsolete.js#15. [0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14ce63011773b2d519e6f10099eacca9c3d425c&selectedJob=152729245

> The webidl at https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element doesn't have [Throws] [Throws] is a Gecko-specific annotation, to avoid generating throwing machinery for methods/getters/setters that can't throw. In the spec, everything is assumed to be able to throw and the prose describes whether it does. Anyway, the short answer is we should be adding [Throws] on things that can throw, compared to the spec IDL. > chrome does throw on `loop` but nothing else That's not spec-compliant. Per spec, the "loop" setter doesn't throw for any particular value. The only reason it would throw in Gecko is out-of-memory. > There are definitely cases where we still differ even after being bound though. I definitely believe it. ;) > The part about "(Other values are ignored.)" makes me think if you do `m.loop = 5; m.loop = -2` > then `m.loop` should still = 5. That is what the spec says right now, yes. It's worth maybe checking what UAs do; if none of them follow the spec here, then maybe the spec can/should change to something simpler... > I *think* the right thing to do would be: The right thing per the current spec text is: |-------------------------------------------------------------| | | m.loop | m.getAttribute("loop") | |-------------------------------------------------------------| | | 5 | "5" | |-------------------------------------------------------------| because the "loop" setter is defined to no-op when passed a number that is <= 0 and not -1. In particular it does not change the attribute value. I just checked what other UAs do with this -2 test, and Chrome throws, Safari throws, Edge throws. We could probably just adopt the throwing behavior (which is not too hard to implement) and file an issue to get the spec changed...

For completeness, we probably ought to document this at some point. Every tech writer wants to write <marquee> examples ;-)

See http://w3c-test.org/html/obsolete/requirements-for-implementations/the-marquee-element-0/ MozReview-Commit-ID: 5K2UWowUr7b

Comment on attachment 8938159 [details] Bug 1425874 - Make <marquee> an actual tag Rebased and moved to phabricator

(In reply to Brian Grinstead [:bgrins] from comment #0) > 3) Prepare marquee to be fully implemented in C++ instead of XBL (using CSS > transitions + some kind of User Agent Shadow DOM not visible to the page) Now that bug 1431255 is fixed, does that mean that this can finally be worked on?

I have now added some documentation to MDN about this: https://developer.mozilla.org/docs/Web/API/HTMLMarqueeElement (will need to be updated once this bug is resolved)

I think this will fix a few of the WPT tests that only fail in Firefox. Specifically, wpt/html/dom/reflection-obsolete.html has some tests relating to marquee properties such as bgColor, direction and height which only fail in Firefox. There are also a number of other tests relating to marquee which also fail in Edge.

With the second patch, work done in bug 1277475 and bug 871887 can be removed.

I will move the 3rd patch to another bug and take the bug, so that HTMLMarqueeElement can be done independently here.

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cc5d07b8f492 Implement HTMLMarqueeElement r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/7d3cd7ee5d67 Implement marquee onbounce, onfinish, onstart with WebIDL r=bzbarsky

https://hg.mozilla.org/mozilla-central/rev/cc5d07b8f492 https://hg.mozilla.org/mozilla-central/rev/7d3cd7ee5d67

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14053 for changes under testing/web-platform/tests

The wpt changes in https://github.com/web-platform-tests/wpt/commit/3029f42ddc9060f1631fb5ed8c87be9f514c210a seem incorrect. reflection.js says about the "enum" type: * This is only used for enums that are limited to known values, not other * enums (those are treated as generic strings by the spec). The data * object passed to reflects() can contain these keys: I don't see anything in the spec to suggest that these attributes are "limited to known values".

Upstream PR merged

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #41) > The wpt changes in > https://github.com/web-platform-tests/wpt/commit/ > 3029f42ddc9060f1631fb5ed8c87be9f514c210a seem incorrect. > > reflection.js says about the "enum" type: > > * This is only used for enums that are limited to known values, not > other > * enums (those are treated as generic strings by the spec). The data > * object passed to reflects() can contain these keys: > > I don't see anything in the spec to suggest that these attributes are > "limited to known values". From https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element: The behavior content attribute on marquee elements is an enumerated attribute with the following keywords (all non-conforming): Keyword State scroll scroll slide slide alternate alternate The missing value default and invalid value default are the scroll state. and The direction content attribute on marquee elements is an enumerated attribute with the following keywords (all non-conforming): Keyword State left left right right up up down down The missing value default and invalid value default are the left state. Is the "enumerated attribute" and "invalid value default" language not enough to consider it an enum for reflection.js?

(In reply to Brian Grinstead [:bgrins] from comment #43) > (In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #41) > > The wpt changes in > > https://github.com/web-platform-tests/wpt/commit/ > > 3029f42ddc9060f1631fb5ed8c87be9f514c210a seem incorrect. > > > > reflection.js says about the "enum" type: > > > > * This is only used for enums that are limited to known values, not > > other > > * enums (those are treated as generic strings by the spec). The data > > * object passed to reflects() can contain these keys: > > > > I don't see anything in the spec to suggest that these attributes are > > "limited to known values". > > From > https://html.spec.whatwg.org/multipage/obsolete.html#the-marquee-element: > > The behavior content attribute on marquee elements is an enumerated > attribute with the following keywords (all non-conforming): > Keyword State > scroll scroll > slide slide > alternate alternate > > The missing value default and invalid value default are the scroll state. > > and > > The direction content attribute on marquee elements is an enumerated > attribute with the following keywords (all non-conforming): > Keyword State > left left > right right > up up > down down > > The missing value default and invalid value default are the left state. > > Is the "enumerated attribute" and "invalid value default" language not > enough to consider it an enum for reflection.js? No; reflection.js says about the "enum" type: * This is only used for enums that are limited to known values, not other * enums (those are treated as generic strings by the spec).

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #44) > > Is the "enumerated attribute" and "invalid value default" language not > > enough to consider it an enum for reflection.js? > > No; reflection.js says about the "enum" type: > > * This is only used for enums that are limited to known values, not > other > * enums (those are treated as generic strings by the spec). But it is limited to known values, right? For `behavior` the missing value default and invalid value default are the scroll state. So if you do `marquee.behavior = "foo"` then `marquee.behavior == "scroll"`. Maybe I'm missing something - forwarding the question to Boris.

So per spec as currently written, "behavior" and "direction" are not limited to known values, because it doesn't say they are in https://html.spec.whatwg.org/multipage/obsolete.html#dom-marquee-behavior In https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes there are different rules for reflecting "a DOMString attribute whose content attribute is an enumerated attribute, and the IDL attribute is limited to only known values" and "a nullable DOMString attribute whose content attribute is an enumerated attribute" and "all other DOMString cases". The "limited to only known values" case has this behavior: marquee.behavior = "foo"; assert(marquee.behavior == "scroll"); The nullable case does not apply here since the attributes are not nullable. The "all other" case would have: marquee.behavior = "foo"; assert(marquee.behavior == "foo"); OK, so what behavior do browsers have? I just tested this: data:text/html,<marquee%20id="x"%20behavior="foo"><script>alert(x.behavior)</script> and I get "foo" alerted in Safari and Chrome. Similar for direction. This matches the current spec, but means you can't get the "real" behavior/direction value from script of course. That said, I just tested Edge and it matches our behavior, which corresponds to these enumerated attribute being limited to known values. See testcase at http://jsfiddle.net/0j16srnw/ At this point we have two courses of action: 1) Keep our implementation, file a spec bug to get the spec changed to align with Firefox and Edge instead of Chrome/Safari, get them to change. 2) Align our implementation with the spec/Chrome/Safari (in a followup). So change our reflection, change the test to just test reflecting as a string, and probably add some IsChromeOrXBL getters that have the behavior of our current direction/behavior getters so the script can keep using those. Anne, any opinions on the spec end?

Not really, seems okay to change if that's preferable. I wouldn't expect us to ever add new features to <marquee> so it isn't exactly needed, but it's a little cleaner and the code path for enumerated attributes already exists. Main work would be convincing Chrome and Safari, which I suspect will happily implement if somebody does the tests.

I think the parser code also needs to also have the changes, running the html parser overwrites this change.

Yes, https://hg.mozilla.org/mozilla-central/diff/cc5d07b8f492/parser/html/nsHtml5ElementName.cpp needs a corresponding change in https://hg.mozilla.org/projects/htmlparser/file/c50d5979f5bb/src/nu/validator/htmlparser/impl/ElementName.java

Comment on attachment 9029302 [details] [diff] [review] Parser repo patch Review of attachment 9029302 [details] [diff] [review]: ----------------------------------------------------------------- Thanks

Note to MDN writers: I'm inclined not to add this to the release notes, but I'll leave the dev-doc-needed open in case anyone feels like documenting HTMLMarqueeElement/write some examples.