324408 - Marquee onstart, onfinish and onbounce events not supported

Status: RESOLVED FIXED

Description

[Martijn Wargers (zombie)]

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.

Comment 1

[Martijn Wargers (zombie)]

Created attachment 209356 [details]
Testcase1, testing onstart

Comment 2

[Martijn Wargers (zombie)]

Created attachment 209358 [details]
Testcase2, testing onfinish

Comment 3

[Martijn Wargers (zombie)]

Created attachment 209360 [details]
Testcase3, testing onbounce

Comment 4

[Martijn Wargers (zombie)]

Created attachment 209375 [details]
Testcase 4, testing dynamic onbounce

Comment 5

[Martijn Wargers (zombie)]

Created attachment 209392 [details]
Testcase 5, testing dynamic onstart

Comment 6

[Martijn Wargers (zombie)]

Created attachment 209395 [details]
Testcase 6, testing dynamic onfinish

Comment 7

[Martijn Wargers (zombie)]

Created attachment 209400 [details]
Testcase 6, testing dynamic onfinish

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.

Comment 8

[Martijn Wargers (zombie)]

Created attachment 209401 [details] [diff] [review]
patch

I've tested this with the testcases attached here.
I haven't tested yet on websites or other testcases for eventual regressions.

Comment 9

[Martijn Wargers (zombie)]

Created attachment 209406 [details] [diff] [review]
patch2

Found one small thing, this needed changing:
-                    if (!this.runId)
+                    if (this.runId == 0)
For the rest, everything looked fine.

Comment 10

[Doron Rosenberg (IBM)]

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{

Comment 11

[Martijn Wargers (zombie)]

Created attachment 209490 [details]
Testcase 7, testing invalid input

Comment 12

[Martijn Wargers (zombie)]

Created attachment 209504 [details] [diff] [review]
patch3

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.

Comment 13

[Doron Rosenberg (IBM)]

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.

Comment 14

[Martijn Wargers (zombie)]

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.

Comment 15

[Martijn Wargers (zombie)]

Created attachment 209580 [details] [diff] [review]
patch4

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.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 209580 [details] [diff] [review]
patch4

sr=jst

Comment 17

[Martijn Wargers (zombie)]

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!

Comment 18

[Martijn Wargers (zombie)]

Reassigning to me.