Closed
Bug 1249061
Opened 8 years ago
Closed 8 years ago
Improve property/attribute handling for marquees
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: martijn.martijn, Assigned: martijn.martijn)
References
()
Details
(Whiteboard: dom-triaged btpp-active)
Attachments
(1 file, 6 obsolete files)
33.58 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
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.
Updated•8 years ago
|
Whiteboard: dom-triaged btpp-active
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0591327a3cf
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=144d3857d575
Assignee | ||
Comment 6•8 years ago
|
||
Oops, sorry about that.
Attachment #8722082 -
Attachment is obsolete: true
Attachment #8722174 -
Flags: review?(bugs)
Comment 7•8 years ago
|
||
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•8 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)
Comment 9•8 years ago
|
||
(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)
Comment 10•8 years ago
|
||
(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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=486fdce62287
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33383a219db5
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
Attachment #8724206 -
Attachment is obsolete: true
Attachment #8724206 -
Flags: review?(bugs)
Attachment #8724207 -
Flags: review?(bugs)
Comment 15•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40cbe4aabb05
Assignee | ||
Comment 17•8 years ago
|
||
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•8 years ago
|
||
Olli, you can take another look at this last patch after you're back from holiday?
Flags: needinfo?(bugs)
Comment 19•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dec698857df
Assignee | ||
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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•8 years ago
|
||
Attachment #8735265 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7387f7b46b
Keywords: checkin-needed
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b7387f7b46b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•