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
•