Closed
Bug 324408
Opened 19 years ago
Closed 18 years ago
Marquee onstart, onfinish and onbounce events not supported
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: martijn.martijn)
Details
(Keywords: testcase)
Attachments
(8 files, 4 obsolete files)
http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/marquee.asp onstart event: http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onstart.asp onfinish event: http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onfinish.asp onbounce event: http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbounce.asp These are the marquee specific events that I know of. Neither of them are implemented in Mozilla.
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
Assignee | ||
Comment 3•19 years ago
|
||
Assignee | ||
Comment 4•19 years ago
|
||
Assignee | ||
Comment 5•19 years ago
|
||
Assignee | ||
Comment 6•19 years ago
|
||
Assignee | ||
Comment 7•19 years ago
|
||
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
Assignee | ||
Comment 8•19 years ago
|
||
I've tested this with the testcases attached here. I haven't tested yet on websites or other testcases for eventual regressions.
Assignee | ||
Comment 9•19 years ago
|
||
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 10•19 years ago
|
||
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{
Assignee | ||
Comment 11•19 years ago
|
||
Assignee | ||
Comment 12•19 years ago
|
||
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 13•19 years ago
|
||
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.
Assignee | ||
Comment 14•19 years ago
|
||
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
Assignee | ||
Comment 15•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #209580 -
Flags: review?(doronr) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #209580 -
Flags: superreview?(jst)
Comment 16•19 years ago
|
||
Comment on attachment 209580 [details] [diff] [review] patch4 sr=jst
Attachment #209580 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 17•18 years ago
|
||
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: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•18 years ago
|
||
Reassigning to me.
Assignee: nobody → martijn.martijn
Status: REOPENED → NEW
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•