Closed
Bug 324408
Opened 19 years ago
Closed 19 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•19 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: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•19 years ago
|
||
Reassigning to me.
Assignee: nobody → martijn.martijn
Status: REOPENED → NEW
Assignee | ||
Updated•19 years ago
|
Status: NEW → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•