Marquee onstart, onfinish and onbounce events not supported

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Martijn Wargers (dead))

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
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

12 years ago
Created attachment 209356 [details]
Testcase1, testing onstart
(Assignee)

Comment 2

12 years ago
Created attachment 209358 [details]
Testcase2, testing onfinish
(Assignee)

Comment 3

12 years ago
Created attachment 209360 [details]
Testcase3, testing onbounce
(Assignee)

Comment 4

12 years ago
Created attachment 209375 [details]
Testcase 4, testing dynamic onbounce
(Assignee)

Comment 5

12 years ago
Created attachment 209392 [details]
Testcase 5, testing dynamic onstart
(Assignee)

Comment 6

12 years ago
Created attachment 209395 [details]
Testcase 6, testing dynamic onfinish
(Assignee)

Comment 7

12 years ago
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.
Attachment #209395 - Attachment is obsolete: true
(Assignee)

Comment 8

12 years ago
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.
(Assignee)

Comment 9

12 years ago
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.
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{
(Assignee)

Comment 11

12 years ago
Created attachment 209490 [details]
Testcase 7, testing invalid input
(Assignee)

Comment 12

12 years ago
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.
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.
(Assignee)

Comment 14

12 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

12 years ago
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.
Attachment #209504 - Attachment is obsolete: true
Attachment #209580 - Flags: review?(doronr)
Attachment #209504 - Flags: review?(doronr)

Updated

12 years ago
Attachment #209580 - Flags: review?(doronr) → review+
(Assignee)

Updated

12 years ago
Attachment #209580 - Flags: superreview?(jst)
Comment on attachment 209580 [details] [diff] [review]
patch4

sr=jst
Attachment #209580 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 17

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

11 years ago
Reassigning to me.
Assignee: nobody → martijn.martijn
Status: REOPENED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.