1425874 - Make <marquee> construct HTMLMarqueeElement instead of overloading HTMLDivElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P2)

Description

[Brian Grinstead [:bgrins]]

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)

Comment 1

[Bobby Holley (:bholley)]

Incrementalism ftw :-)

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1425874 - Don't expect marquee properties to throw with invalid values

Review commit: https://reviewboard.mozilla.org/r/208868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208868/

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1425874 - Make <marquee> an actual tag

See http://w3c-test.org/html/obsolete/requirements-for-implementations/the-marquee-element-0/

Review commit: https://reviewboard.mozilla.org/r/208870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208870/

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/1-2/

Comment 5

[Brian Grinstead [:bgrins]]

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

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/2-3/

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

[Brian Grinstead [:bgrins]]

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

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Brian Grinstead [:bgrins]]

(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

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/3-4/

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8938158 [details]
Bug 1425874 - Don't expect marquee properties to throw with invalid values

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208868/diff/1-2/

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/4-5/

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8938158 [details]
Bug 1425874 - Don't expect marquee properties to throw with invalid values

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208868/diff/2-3/

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/5-6/

Comment 19

[Chris Mills [:cmills]]

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

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8938158 [details]
Bug 1425874 - Don't expect marquee properties to throw with invalid values

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208868/diff/3-4/

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/6-7/

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8938158 [details]
Bug 1425874 - Don't expect marquee properties to throw with invalid values

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208868/diff/4-5/

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/7-8/

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/8-9/

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/9-10/

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208870/diff/10-11/

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: Bug 1425874 - Implement HTMLMarqueeElement

See http://w3c-test.org/html/obsolete/requirements-for-implementations/the-marquee-element-0/

MozReview-Commit-ID: 5K2UWowUr7b

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8938159 [details]
Bug 1425874 - Make <marquee> an actual tag

Rebased and moved to phabricator

Comment 29

[ExE Boss]

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

Comment 30

[ExE Boss]

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)

Comment 31

[Andrew McCreight [:mccr8]]

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.

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: Bug 1425874 - Implement marquee onbounce, onfinish, onstart with WebIDL

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1425874 - Convert xbl-marquee to UA Widget

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 36

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f8410c9696df75d1b8708a88cb88aa7d2f381d

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

Comment 38

[Cristina Coroiu [:ccoroiu]]

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

Comment 39

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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

Comment 40

[ExE Boss]

Attachment: MDN: Browser compatibility table update

Comment 41

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

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

Comment 42

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 43

[Brian Grinstead [:bgrins]]

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

Comment 44

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

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

Comment 45

[Brian Grinstead [:bgrins]]

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

Comment 46

[Boris Zbarsky [:bzbarsky]]

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?

Comment 47

[Anne (:annevk)]

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.

Comment 48

[Jonathan Kingston [:jkt] he/him]

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

Comment 49

[Henri Sivonen (:hsivonen)]

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 50

[Henri Sivonen (:hsivonen)]

Attachment: Parser repo patch

Comment 51

[Brian Grinstead [:bgrins]]

Comment on attachment 9029302 [details] [diff] [review]
Parser repo patch

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

Thanks

Comment 52

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/2762c45ba362689bc90cc3ffeb0e8a37e568e716

Comment 53

[Chris Mills [:cmills]]

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.