Closed Bug 324408 Opened 19 years ago Closed 19 years ago

Marquee onstart, onfinish and onbounce events not supported

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

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

Details

(Keywords: testcase)

Attachments

(8 files, 4 obsolete files)

Attached file Testcase 6, testing dynamic onfinish (obsolete) —
Sorry, last one was incorrect. Note that the dynamic testcase work differently/not at all for certain buttons, because of differences in setAttribute and .onevent.
Attachment #209395 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
I've tested this with the testcases attached here. I haven't tested yet on websites or other testcases for eventual regressions.
Attached patch patch2 (obsolete) — Splinter Review
Found one small thing, this needed changing: - if (!this.runId) + if (this.runId == 0) For the rest, everything looked fine.
Attachment #209401 - Attachment is obsolete: true
Attachment #209406 - Flags: review?(doronr)
Comment on attachment 209406 [details] [diff] [review] patch2 _set_onStart is a weird name, maybe _setOnStart() Also, _set_onStart and _set_onFinish are nearly identical, please create a helper method. >+ default: >+ break; >+ } >+ return true; >+ ]]> >+ </body> >+ </method> What happens in IE if the value isn't a string or a function ref? Right now you do nothing. >+ var e = document.createEvent("Events"); >+ e.initEvent('bounce', false, true); >+ this.dispatchEvent(e); Also perhaps a helper method to create events, since you use it a few times. - >- if (!this.startNewDirection) { >+ else >+ { > if ((this._direction == "up") || (this._direction == "down")) > this.outerDiv.scrollTop = this.newPosition; > else > this.outerDiv.scrollLeft = this.newPosition; > } file defaults to else {, not else\n{
Attached patch patch3 (obsolete) — Splinter Review
Ok, I followed your comments. I've replaced _set_onStart, _set_onFinish and _set_onBounce with _setEvent, which handles all of the three events. I've also made a _fireEvent, that's the helper method to create events. > What happens in IE if the value isn't a string or a function ref? Right now > you do nothing. I've now done this: + case "boolean": + case "number": + case "undefined": + throw new Error("Invalid argument for Marquee::on" + aName); This is also when IE6 is throwing errors. And default is just setting the property without adding an eventlistener.
Attachment #209406 - Attachment is obsolete: true
Attachment #209504 - Flags: review?(doronr)
Attachment #209406 - Flags: review?(doronr)
Comment on attachment 209504 [details] [diff] [review] patch3 >+ >+ case "boolean": >+ case "number": >+ case "undefined": >+ throw new Error("Invalid argument for Marquee::on" + aName); >+ break; >+ >+ default: >+ this["_on" + aName] = aValue; Why not make default be the new Error() part? Shouldn't anything that isn't a string/function ref cause the error? Other than that, looks good.
See testcase 7, button 5, onbounce = {}, that doesn't throw an error in IE6. But I guess I could change this, and make objects not throw an error and by default throw an error.
Keywords: testcase
Attached patch patch4Splinter Review
Oops! I forgot to add this._ignoreNextCall = false; before throwing an error. This is needed otherwise you won't get the error next time. And with this patch, I've made the default throw an error.
Attachment #209504 - Attachment is obsolete: true
Attachment #209580 - Flags: review?(doronr)
Attachment #209504 - Flags: review?(doronr)
Attachment #209580 - Flags: review?(doronr) → review+
Attachment #209580 - Flags: superreview?(jst)
Comment on attachment 209580 [details] [diff] [review] patch4 sr=jst
Attachment #209580 - Flags: superreview?(jst) → superreview+
Checking in layout/style/xbl-marquee/xbl-marquee.xml; /cvsroot/mozilla/layout/style/xbl-marquee/xbl-marquee.xml,v <-- xbl-marquee.xm l new revision: 1.23; previous revision: 1.22 done My first check-in!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reassigning to me.
Assignee: nobody → martijn.martijn
Status: REOPENED → NEW
Status: NEW → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: