Improve property/attribute handling for marquees

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Martijn Wargers (dead))

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

(Whiteboard: dom-triaged btpp-active, URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

2 years ago
In bug 1160342, I added a mochitest - test_bug1160342_marquee.html - that tests a lot of property/attribute handling for marquee handling.
A lot of the subtests in there are in todo_is, because they are failing currently in Mozilla and those subtests are passing in Edge/IE.

I chatted with Olli on irc and he told me we should mimick Edge/IE behavior, but he'd rather not see us mimick Edge/IE behavior when they throw on setting certain values on properties.
To be precise, I hope we don't start to throw in cases we don't throw currently and where Chrome doesn't throw either. Just because throwing an exception is such an easy way to break pages.
Whiteboard: dom-triaged btpp-active
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0591327a3cf
(Assignee)

Comment 3

2 years ago
Created attachment 8722082 [details] [diff] [review]
1160342_marquee.diff

Like this? This would make us behave identical as IE/Edge for the cases in the mochitest, except for the throwing js errors part.
I also added an attributefilter for the mutationobserver.
Attachment #8722082 - Flags: review?(bugs)
Comment on attachment 8722082 [details] [diff] [review]
1160342_marquee.diff

You landed wrong patch? This one is converting us to use MutationObserver.
Attachment #8722082 - Flags: review?(bugs)
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=144d3857d575
(Assignee)

Comment 6

2 years ago
Created attachment 8722174 [details] [diff] [review]
1249061_marquee.diff

Oops, sorry about that.
Attachment #8722082 - Attachment is obsolete: true
Attachment #8722174 - Flags: review?(bugs)
Comment on attachment 8722174 [details] [diff] [review]
1249061_marquee.diff

>       <property name="scrollAmount" exposeToUntrustedContent="true">
>         <getter>
>           <![CDATA[
>           this._mutationActor(this._mutationObserver.takeRecords());
>-          var val = parseInt(this.getAttribute("scrollamount"));
>-
>-          if (val <= 0 || isNaN(val))
>-            return this._scrollAmount;
>-
>-          return val;
>+          return this._scrollAmount;
>           ]]>
>         </getter>
>         <setter>
>+          <![CDATA[
>+          var val = parseInt(val);
>+          if (val < 0)
>+            return;
>+          if (isNaN(val))
>+            val = 0;
>           this.setAttribute("scrollamount", val);
>+          ]]>
>         </setter>
>       </property>
> 
>       <property name="scrollDelay" exposeToUntrustedContent="true">
>         <getter>
>           <![CDATA[
>           this._mutationActor(this._mutationObserver.takeRecords());
>           var val = parseInt(this.getAttribute("scrolldelay"));
> 
>           if (val <= 0 || isNaN(val))
>             return this._scrollDelay;
> 
>           return val;
>           ]]>
>         </getter>
>         <setter>
>-          this.setAttribute("scrolldelay", val);
>+          var val = parseInt(val);
>+          if (val > 0 )
>+            this.setAttribute("scrolldelay", val);
>         </setter>
>       </property>

Ok, so the setters never set _scrollAmount or _scrollDelay because MutationObserver will set those _ variables, right?


>       <property name="behavior" exposeToUntrustedContent="true">
>         <getter>
>-          this._mutationActor(this._mutationObserver.takeRecords());
>-          return this._behavior;
>+          var val = this.getAttribute("behavior");
>+          if (val) {
>+            val = val.toLowerCase();
>+          }
>+          if (val == "alternate" || val == "slide") {
>+            return val;
>+          }
>+          return "scroll";
Hmm, so we never use this._behavior here.
I must be missing something here. Why do we need to change the method implementation here?


>         </getter>
>         <setter>
>-          this.setAttribute("behavior", val);
>+          if (val) {
>+            val = val.toLowerCase();
>+          }
>+          if (val == "alternate" || val == "slide" || val == 'scroll') {
>+            this.setAttribute("behavior", val);
>+          }
Hmm, I don't see any tests whether marquee.behavior = "foobar"; is supposed to change the
value of marquee.getAttribute("behavior");
Aren't we missing such tests also for other properties?


So, some clarifications when _ variables are supposed to be used, and also tests for getAttribute please.
Attachment #8722174 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

2 years ago
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #7)
> Ok, so the setters never set _scrollAmount or _scrollDelay because
> MutationObserver will set those _ variables, right?

Yes.

> Hmm, so we never use this._behavior here.
> I must be missing something here. Why do we need to change the method
> implementation here?

Because otherwise the "x.setAttribute('behavior', 'invalid');" case would be failing.
 
> 
> >         </getter>
> >         <setter>
> >-          this.setAttribute("behavior", val);
> >+          if (val) {
> >+            val = val.toLowerCase();
> >+          }
> >+          if (val == "alternate" || val == "slide" || val == 'scroll') {
> >+            this.setAttribute("behavior", val);
> >+          }
> Hmm, I don't see any tests whether marquee.behavior = "foobar"; is supposed
> to change the
> value of marquee.getAttribute("behavior");
> Aren't we missing such tests also for other properties?

There is the  try{x.behavior = 'invalid';} case, isn't that what you mean?

> So, some clarifications when _ variables are supposed to be used, and also
> tests for getAttribute please.

The _ variables are needed to store what is used, because the attributes can store invalid values or values that are not being used (in the case of scrollDelay for example when truespeed is false) in certain cases.
What do you mean with tests for getAttribute? You want tests when I'm setting properties to make sure that the getAttribute value is correct?
Flags: needinfo?(bugs)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #8)
> > Hmm, I don't see any tests whether marquee.behavior = "foobar"; is supposed
> > to change the
> > value of marquee.getAttribute("behavior");
> > Aren't we missing such tests also for other properties?
> 
> There is the  try{x.behavior = 'invalid';} case, isn't that what you mean?
I don't see any getAttribute calls in the test.


> What do you mean with tests for getAttribute? You want tests when I'm
> setting properties to make sure that the getAttribute value is correct?
Yes. It is unclear to me in which cases setting some property is supposed to update the attribute value.
Flags: needinfo?(bugs)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #8)
> 
> Because otherwise the "x.setAttribute('behavior', 'invalid');" case would be
> failing.
Oh, we don't update _behavior if invalid attribute is set, yet
x.behavior starts to return "scroll". That feels odd.
Don't we want to update _behavior to be "scroll"?
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=486fdce62287
(Assignee)

Comment 12

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33383a219db5
(Assignee)

Comment 13

2 years ago
Created attachment 8724206 [details] [diff] [review]
1160342_marquee.diff

Ok, I think I addressed comment 9 and 10 in this patch.
Attachment #8722174 - Attachment is obsolete: true
Attachment #8724206 - Flags: review?(bugs)
(Assignee)

Comment 14

2 years ago
Created attachment 8724207 [details] [diff] [review]
1249061_marquee.diff
Attachment #8724206 - Attachment is obsolete: true
Attachment #8724206 - Flags: review?(bugs)
Attachment #8724207 - Flags: review?(bugs)
Comment on attachment 8724207 [details] [diff] [review]
1249061_marquee.diff

>+    x.setAttribute('behavior', 'slide');
>     x.removeAttribute('behavior');
>     is(x.behavior, "scroll", "Wrong behavior value");
>+    is(x.getAttribute('behavior'), null, "Wrong behavior attribute");
I assume this is the behavior Edge has.
Same with other getAttribute calls


>+          if (val == 'left' || val == 'right' || val == 'up' || val == 'down')
>+            return val;
>+          else
>+            return "left";

I would prefer using {} here
if () {

} else {

}

but not sure what the coding style is in xbl widgets usually
>         <setter>
>-          this.setAttribute("loop", val);
>+          <![CDATA[
>+          var val = parseInt(val);
>+          if (val >= -1) {
>+            this.setAttribute("loop", val);
>+          }
>+          ]]>
>         </setter>
So Edge actually limits the val to [-1,<max int>]?
Based on the tests looks like so.
Could you test also what happens if something like -1.123 is passed as value?

>       <method name="_set_behavior">
>         <parameter name="aValue"/>
>         <body>
>         <![CDATA[
>           if (typeof aValue == 'string')
>             aValue = aValue.toLowerCase();
>-          if (aValue != 'alternate' && aValue != 'slide' && aValue != 'scroll')
>-            return false;
>-
>-          this._behavior = aValue;
>-          return true;
>+          if (aValue != 'alternate' && aValue != 'slide' && aValue != 'scroll') {
>+            this._behavior = 'scroll';
>+          } else {
>+            this._behavior = aValue;
>+          }
>         ]]>
Tiny bit odd to have all this behavior handling here and also in .behavior getter, but I guess fine.
Attachment #8724207 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40cbe4aabb05
(Assignee)

Comment 17

2 years ago
Created attachment 8724570 [details] [diff] [review]
1249061_marquee.diff

I addressed all your issues in comment 15. 
Indeed, _set_behavior could be simplified as well as _set_loop and _set_direction, so I did that here.
I've now converted almost all if () calls to use {}.
I've now also added 0, -0.123 and -1.123 as values to test in the mochitest.

The mochitest can also be seen here: http://people.mozilla.org/~mwargers/mochitestjs2/test_bug1160342_marquee.html
And all the tests are passing in IE/Edge, you can check using http://netrenderer.com/index.php (use the extra rendering time checkbox)
With this patch, Mozilla is passing for all the tests, except for the cases where IE/Edge is throwing errors.
Attachment #8724207 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Olli, you can take another look at this last patch after you're back from holiday?
Flags: needinfo?(bugs)
Comment on attachment 8724570 [details] [diff] [review]
1249061_marquee.diff

Could you test how marquee.direction = { toString: function _toString() { return "up"} }
behaves in Edge?
Same with .behavior.
We actually end up throwing in .behavior case, and I think that is a bug, at least it regresses behavior. Please add a test for that case.

Do we have tests for cases like 
marquee.behavior = null; or marquee.behavior = undefined; ? Could you test.

r- because of that throwing in .behavior setter case.
Flags: needinfo?(bugs)
Attachment #8724570 - Flags: review-
(Assignee)

Comment 20

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dec698857df
(Assignee)

Comment 21

2 years ago
Created attachment 8735265 [details] [diff] [review]
1249061_marquee.diff

I did the things you asked for in comment 19, I also added some tests for direction up/down.
The problem with marquee.direction = { toString: function _toString() { return "up"} } is that I can't access user defined functions from inside the chrome marquee binding, unless I use .wrappedJSObject, but I don't think I'm allowed to do that, because it would have security problems, right?
So I changed the marquee binding so that it doesn't cause an error, at least. With this patch, it just doesn't do anything.
Attachment #8724570 - Attachment is obsolete: true
Attachment #8735265 - Flags: review?(bugs)
Comment on attachment 8735265 [details] [diff] [review]
1249061_marquee.diff

I see. Too bad that toString doesn't work.
Attachment #8735265 - Flags: review?(bugs) → review+
(Assignee)

Comment 23

2 years ago
Created attachment 8737164 [details] [diff] [review]
1249061_marquee.diff (for check-in)
Attachment #8735265 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 24

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7387f7b46b
Keywords: checkin-needed

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b7387f7b46b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.