Closed Bug 1249061 Opened 4 years ago Closed 4 years ago

Improve property/attribute handling for marquees

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

()

Details

(Whiteboard: dom-triaged btpp-active)

Attachments

(1 file, 6 obsolete files)

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
Attached patch 1160342_marquee.diff (obsolete) — Splinter Review
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)
Attached patch 1249061_marquee.diff (obsolete) — Splinter Review
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-
(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"?
Attached patch 1160342_marquee.diff (obsolete) — Splinter Review
Ok, I think I addressed comment 9 and 10 in this patch.
Attachment #8722174 - Attachment is obsolete: true
Attachment #8724206 - Flags: review?(bugs)
Attached patch 1249061_marquee.diff (obsolete) — Splinter Review
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+
Attached patch 1249061_marquee.diff (obsolete) — Splinter Review
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
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-
Attached patch 1249061_marquee.diff (obsolete) — Splinter Review
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+
Attachment #8735265 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b7387f7b46b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.