Closed
Bug 167224
Opened 22 years ago
Closed 21 years ago
Cleanup marquee
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.7alpha
People
(Reporter: doronr, Assigned: doronr)
References
Details
(Keywords: topembed-, Whiteboard: [dev notes] whitebox)
Attachments
(2 files, 12 obsolete files)
14.48 KB,
patch
|
neil
:
review+
bryner
:
superreview+
tor
:
approval1.7+
|
Details | Diff | Splinter Review |
14.37 KB,
patch
|
Details | Diff | Splinter Review |
Will fix:
- use % for height
- correct behaviour of alternate scrolling
- allow marquee's direction to be changed anytime
- make marquee a block in html.css
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
Attachment #98242 -
Attachment is obsolete: true
Assignee | ||
Comment 3•22 years ago
|
||
much better patch
Attachment #98244 -
Attachment is obsolete: true
Assignee | ||
Comment 4•22 years ago
|
||
Attachment #98373 -
Attachment is obsolete: true
Updated•22 years ago
|
QA Contact: petersen → amar
Assignee | ||
Updated•22 years ago
|
Updated•22 years ago
|
Priority: -- → P2
Comment 6•22 years ago
|
||
Marking as nsbeta1+/topembed+ per EDT triage.
Assignee | ||
Comment 7•22 years ago
|
||
this applies correctly on 1.2 and trunk
Attachment #98523 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•22 years ago
|
||
Attachment #105209 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #106286 -
Flags: review?(ian)
Comment 9•22 years ago
|
||
Comment on attachment 106286 [details] [diff] [review]
fix 2 minor "bugs" *cough*IE compat*cough*
fix the three things I mentioned on IRC and add some breathing space around the
?: operators and come back to me. :-)
Attachment #106286 -
Flags: review?(ian) → review-
Updated•22 years ago
|
Whiteboard: [dev notes]
Updated•22 years ago
|
Whiteboard: [dev notes] → [dev notes] whitebox
Assignee | ||
Updated•22 years ago
|
Target Milestone: mozilla1.4alpha → Future
Comment 11•22 years ago
|
||
Discussed in topembed bug triage. Minusing.
Comment 14•21 years ago
|
||
Doron,
can you repair the things Ian requested and check if the patch still works in
current trunk version?
pi
Assignee | ||
Comment 15•21 years ago
|
||
Since opera now supports marquee, we have to have better support than they do :)
I lost all the stuff at netscape, so will redo the patch (parts of it are in the
tree) and get new reviews.
Target Milestone: mozilla1.6alpha → mozilla1.6beta
Assignee | ||
Comment 17•21 years ago
|
||
Attachment #106286 -
Attachment is obsolete: true
Assignee | ||
Comment 18•21 years ago
|
||
Attachment #134567 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #135185 -
Flags: review?(ian)
Comment 19•21 years ago
|
||
Comment on attachment 135185 [details] [diff] [review]
add some spacing to make it more readable
don't have time to review this in the near future, sorry
Attachment #135185 -
Flags: review?(ian) → review?
Assignee | ||
Updated•21 years ago
|
Attachment #135185 -
Flags: review? → review?(timeless)
Comment 20•21 years ago
|
||
Comment on attachment 135185 [details] [diff] [review]
add some spacing to make it more readable
> <property name="direction">
> <getter>
> if (this.hasAttribute("direction"))
>- return this.getAttribute("direction");
>+ return this.getAttribute("direction").toLowerCase();
no. getters are generally called more often, and as you'll see later, you want
to have the case 'correct' internally.
> return "left"; //default value is "left"
> </getter>
>+ <setter>
>+ // since we changed the direction, set startNewDirection to 1
>+ this.startNewDirection = 1;
>+ this.setAttribute("direction", val);
toLowerCase here.
>+ </setter>
> </property>
>
> <property name="behavior">
> <getter>
> if (this.hasAttribute("behavior"))
>- return this.getAttribute("behavior");
>+ return this.getAttribute("behavior").toLowerCase();
same comment, not here.
> return "scroll"; //default value is "scroll"
> </getter>
>+ <setter>
>+ this.setAttribute("behavior", val);
same comment, convert here.
>+ </setter>
> </property>
>
> <field name="dirsign">1</field>
>@@ -115,18 +128,22 @@
> <![CDATA[
> if (this.hasAttribute("width"))
> return this.getAttribute("width");
>- else if (this.parentNode.offsetWidth > 0)
>- return this.parentNode.offsetWidth;
>- else if (this.parentNode.parentNode.offsetWidth > 0)
>- return this.parentNode.parentNode.offsetWidth;
>- else
else after return
>+ else if (this.offsetParent){
>+ var myElem = this;
>+ while (myElem && (myElem.offsetWidth <= 0))
>+ myElem = this.offsetparent;
>+
>+ if(myElem.offsetWidth > 0)
>+ return myElem.offsetWidth;
>+ else return 1;
else after and even before return
>+ } else
> return 1;
and after again.
if (this.hasAttribute("width"))
return this.getAttribute("width");
if (!this.offsetParent)
return 1;
var myElem = this;
while (myElem && (myElem.offsetWidth <= 0))
myElem = this.offsetparent;
return myElem.offsetWidth || 1;
/* as you can easily see from this loop and return, there's a problem if myElem
becomes false. i couldn't even see that in your original code. what do you want
to do? return myElem && myElem.offsetWidth || 1; is one possibility. changing
the loop control is another, there are others still.... */
>+ if ((this.direction == "up") || (this.direction == "down")){
note that you don't check for "UP" here. fixing the setter avoids this problem.
>+
>+ if (this.hasAttribute('height')){
>+ height = this.getAttribute('height');
>+ if (height == "") //empty attribute, default to 200px
>+ height = "200px";
^ trailing whitespace.
>+ }
>+ else
>+ height = "200px";
What if the height is 0?
If height = 0 is meaningless (and should be treated as empty) then you can use:
height = this.hasAttribute('height') && this.getAttribute('height') || "200px";
as a replacement for the enter height section.
>+
>+ // support % heights
>+ if (height.indexOf("%") > 0){
>+ height = parseInt('0' + height, 10);
what happens if parseInt returns NaN?
>+ height = (height/100) * this.outerDiv.offsetHeight;
then this is NaN
>+ }
>+
>+ this.outerDiv.style.height = height;
and this
>+
>+ this.innerDiv.style.padding = height + " 0";
and this is "NaN 0"
> switch (this.direction)
again, you wanted direction lowercased at the setter
> {
> case "up":
> this.dirsign = 1;
>- this.startAt = 0;
>- this.stopAt = this.innerDiv.offsetHeight -
>- parseInt(this.outerDiv.style.height);
>+ this.startAt = (this.behavior == 'alternate') ? this.originalHeight : 0;
this.startAt = (this.behavior == 'alternate') && this.originalHeight;
>+ this.stopAt = this.innerDiv.offsetHeight - parseInt(this.outerDiv.style.height)
>+ - ((this.behavior == 'alternate') ? this.originalHeight : 0);
- ((this.behavior == 'alternate') && this.originalHeight);
> break;
...
>+ this.startAt = this.innerDiv.offsetHeight - parseInt(this.outerDiv.style.height)
>+ - ((this.behavior == 'alternate') ? this.originalHeight : 0);
>+ this.stopAt = (this.behavior == 'alternate') ? this.originalHeight : 0;
same idea
>+ case "left":
>+ this.dirsign = 1;
>+ this.startAt = (this.behavior != 'alternate') ? (this.innerDiv.offsetLeft - this.outerDiv.offsetWidth) : this.innerDiv.offsetWidth;
>+ this.stopAt = this.outerDiv.offsetWidth
>+ + ((this.behavior != 'alternate') ? (this.innerDiv.offsetWidth + this.startAt) : 0);
and here
>+ this.stopAt = (this.behavior != 'alternate') ? this.innerDiv.offsetLeft - this.outerDiv.offsetWidth:this.innerDiv.offsetWidth;
>+ this.startAt = this.outerDiv.offsetWidth
>+ + ((this.behavior!='alternate') ? (this.innerDiv.offsetWidth+ this.stopAt) : 0);
and here
Attachment #135185 -
Flags: review?(timeless) → review-
Comment 21•21 years ago
|
||
Comment on attachment 135185 [details] [diff] [review]
add some spacing to make it more readable
>>- return this.getAttribute("direction");
>>+ return this.getAttribute("direction").toLowerCase();
>no. getters are generally called more often, and as you'll see later, you want
>to have the case 'correct' internally.
I can't really comment on this without knowing what IE does - if you have
<MARQUEE DIRECTION="RIGHT"> what does IE return as the property? (If it's right,
then doron is right, if it's RIGHT, then IMHO you're both wrong.)
>+ while (myElem && (myElem.offsetWidth <= 0))
>+ myElem = this.offsetparent;
Infinite loop alert.
>+ if(myElem.offsetWidth > 0)
>+ return myElem.offsetWidth;
>+ else return 1;
trailing whitespace and you should use ?: here.
>+ if (this.hasAttribute('height')){
>+ height = this.getAttribute('height');
>+ if (height == "") //empty attribute, default to 200px
>+ height = "200px";
>+ }
>+ else
>+ height = "200px";
height = this.getAttribute("height") || "200px";
>What if the height is 0?
>If height = 0 is meaningless (and should be treated as empty) then you can use:
>height = this.hasAttribute('height') && this.getAttribute('height') || "200px";
>as a replacement for the enter height section.
Actually, "0" will evaluate as true for the purposes of ||.
>>+
>>+ // support % heights
>>+ if (height.indexOf("%") > 0){
>>+ height = parseInt('0' + height, 10);
>what happens if parseInt returns NaN?
Rather neatly, the ,10 fixes that :-)
>>+ this.startAt = (this.behavior == 'alternate') ?
>>this.originalHeight : 0;
>this.startAt = (this.behavior == 'alternate') && this.originalHeight;
This is tricky JS at the best of times, assuming that startAt will only be used
as an integer so that false will convert to zero.
Comment 22•21 years ago
|
||
Comment on attachment 135185 [details] [diff] [review]
add some spacing to make it more readable
neil notes that
>+ while (myElem && (myElem.offsetWidth <= 0))
>+ myElem = this.offsetparent;
this loop is infinite (oops), since nothing advances myElem
>+ height = parseInt('0' + height, 10);
and that in my haste, i missed the ,10 -sorry-. so there isn't a nan concern.
Assignee | ||
Comment 23•21 years ago
|
||
on IE, marquee returns the lowercased direction, even if its set to RIGHT in the
HTML.
Nitpickers! :)
Comment 24•21 years ago
|
||
that doesn't answer the question.
if you set "RIGHT", does it behave like "right" or "random-unrecognized-string"?
Assignee | ||
Comment 25•21 years ago
|
||
all marquee attributes are case insensative, and always return the lowercased
version when queried via DOM.
Assignee | ||
Comment 26•21 years ago
|
||
Attachment #135185 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #135400 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 27•21 years ago
|
||
Comment on attachment 135400 [details] [diff] [review]
Fixes the nitpicks and the performance issue
but not the infinite loop this => myElem ?
Attachment #135400 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee | ||
Comment 28•21 years ago
|
||
Attachment #135400 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #135423 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 29•21 years ago
|
||
Comment on attachment 135423 [details] [diff] [review]
fix the loop issue
Were you aware that this patch regresses the behaviour that setAttribute may be
used to modify the marquee?
Do you have a testcase to demonstrate the effect of your changes?
Here are the usual nits:
>+ // since we changed the scrolldelay, restart the marquee
>+ this.stop();
>+ this.start();
start() already calls stop(), also lost the trailing whitespace.
>+ // since we changed the direction, set startNewDirection to 1
Um... why isn't this a true/false?
>+ // "", 0 and empty height should default to 200px
>+ height = (this.hasAttribute('height') && this.getAttribute('height')) ? this.getAttribute('height') : "200px";
This doesn't work for "0", because it's a string.
height = (this.getAttribute("height") != "0" && this.getAttribute("height")) ||
"200px";
>+ if (height.indexOf("%") > 0){
if (/%/.test(height) {
>+ // swap direction
>+ this.directionField =
>+ (this.directionField == "left") ? "right" : (this.directionField == "down") ? "up" : (this.directionField == "up") ? "down" : "left";
const swap = {left: "right", down: "up", up: "down", right: "left"};
this.directionField = swap[this.directionField];
Assignee | ||
Comment 31•21 years ago
|
||
Patch fixes nitpicks as well as makes the marquee transition nicely when a new
direction is set (no marquee position resetting).
Behavior changing doesn't work well still.
Attachment #135423 -
Attachment is obsolete: true
Comment 32•21 years ago
|
||
Any plan to support marquue in CSS3 Style?
http://www.w3.org/TR/2002/WD-css3-box-20021024/#marquee
The pages can then be written using strict mode with visual marquee effects.
The 2nd working draft of the module is planned in April 2004, though.
Assignee | ||
Updated•21 years ago
|
Attachment #137721 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 33•21 years ago
|
||
Comment on attachment 137721 [details] [diff] [review]
fix nitpicks
>+ this.start();
No trailing spaces, please. (You do this quite a lot :-( )
>+ // pass in aSkipSettingNewPosition as true
>+ this.start(true);
Isn't start() a public method? In which case we shouldn't be passing it our own
parameters.
>+ while (myElem.offsetParent && (myElem.offsetWidth <= 0))
>+ myElem = myElem.offsetParent;
>+
>+ if (myElem.offsetWidth > 0)
myElem might be null at this point. However, if it is not null, then its
offsetWidth will be greater than zero.
>+ // Website can define a background color via the bgcolor attribute
>+ if (this.hasAttribute("bgcolor"))
>+ this.outerDiv.style.backgroundColor = this.getAttribute("bgcolor");
This one isn't dynamic then?
The marquee didn't like me deleting the direction or behavior attributes.
Isn't setting the direction/behaviour supposed to update the attribute?
I would liked to have had more test cases.
Attachment #137721 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee | ||
Comment 34•21 years ago
|
||
testcase tool with new xbl embedded:
http://www.nexgenmedia.net/marquee/marqueetest.html
Made a non-public method _doMove that start() now calls. Fixed the whitespaces
as well.
>+ while (myElem.offsetParent && (myElem.offsetWidth <= 0))
>+ myElem = myElem.offsetParent;
>+
>+ if (myElem.offsetWidth > 0)
myElem might be null at this point. However, if it is not null, then its
offsetWidth will be greater than zero.
In theory myElm will always be non-null, but maide it check just in case.
And bgcolor is a different bug, its not new code but code that moved.
Assignee | ||
Comment 35•21 years ago
|
||
Attachment #137721 -
Attachment is obsolete: true
Assignee | ||
Comment 36•21 years ago
|
||
Setting behavior/direction to "" is now fixed, it should do nothing.
Comment 37•21 years ago
|
||
Comment on attachment 141156 [details] [diff] [review]
Fixes neil's issues, also fixes some table width in marquee problems
>+ rv = this.defaultScrollDelay; //default value is 85 ms
var rv, please.
>+ // in case direction is swamped between horizontal/vertical, use
Typo: swapped
>+ if (!this.startNewDirection){
>
Why did you leave that blank line in?
> <method name="init">
> <body>
> <![CDATA[
>- var height;
>
Take out this blank line too.
>+ if (oldValue != newValue && newValue){
>+ if (attrName == "direction"){
>+ this.startNewDirection = true;
>+ this.directionField = newValue;
>+ this._doMove(true);
>+ } else if (attrName == "behavior"){
Spaces between )s and {s please.
Attachment #141156 -
Flags: review+
Assignee | ||
Comment 38•21 years ago
|
||
who does xbl sr's these days?
Attachment #141156 -
Flags: superreview?(bryner)
Blocks: 237314
Comment 39•21 years ago
|
||
If you have a good r=, the sr need not be an XBl/JS whiz.
Should this patch go into 1.7b?
/be
Assignee | ||
Comment 40•21 years ago
|
||
It fixes serveral IE incompatabilities and cleans up the code, so it would be
great for 1.7b. And shouldn't regress anything.
Comment 42•21 years ago
|
||
Comment on attachment 141156 [details] [diff] [review]
Fixes neil's issues, also fixes some table width in marquee problems
>+ <handler event="DOMAttrModified" phase="target">
Unfortunately there is a performance penalty for using mutation events (we
don't bother firing the events at all if there are no mutation listeners). I
can't really see another way to do what you're doing here though. So see if
you can think of a different way to do this, but if not, don't worry about it.
sr=me.
Attachment #141156 -
Flags: superreview?(bryner) → superreview+
Assignee | ||
Comment 43•21 years ago
|
||
Assignee | ||
Comment 44•21 years ago
|
||
Since 1.7 is a long lived branch now - nominating.
Flags: blocking1.7?
Assignee | ||
Updated•21 years ago
|
Attachment #141156 -
Flags: approval1.7?
Attachment #141156 -
Flags: approval1.7? → approval1.7+
Comment 45•21 years ago
|
||
Checking in layout/html/document/src/xbl-marquee/resources/content/xbl-marquee.xml;
/cvsroot/mozilla/layout/html/document/src/xbl-marquee/resources/content/xbl-marquee.xml,v
<-- xbl-marquee.xml
new revision: 1.8; previous revision: 1.7
done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Attachment #135423 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•21 years ago
|
Flags: blocking1.7?
Comment 46•21 years ago
|
||
*** Bug 237314 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•