167224 - Cleanup marquee

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Doron Rosenberg (IBM)]

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

Comment 1

[Doron Rosenberg (IBM)]

Attachment: fixes most of the issues atm

Comment 2

[Doron Rosenberg (IBM)]

Attachment: really working partial patch

Comment 3

[Doron Rosenberg (IBM)]

Attachment: new patch, fixes lots of little issues

much better patch

Comment 4

[Doron Rosenberg (IBM)]

Attachment: no evil tabs!

Comment 5

[Doron Rosenberg (IBM)]

Attachment: fix regression in left scrolling.

better patch

Comment 6

[Jaime Rodriguez, Jr.]

Marking as nsbeta1+/topembed+ per EDT triage.

Comment 7

[Doron Rosenberg (IBM)]

Attachment: patch for trunk/1.2 branch

this applies correctly on 1.2 and trunk

Comment 8

[Doron Rosenberg (IBM)]

Attachment: fix 2 minor "bugs" *cough*IE compat*cough*

Comment 9

[Hixie (not reading bugmail)]

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. :-)

Comment 10

[Doron Rosenberg (IBM)]

1.4alpha I guess

Comment 11

[Michael Buckland]

Discussed in topembed bug triage.  Minusing.

Comment 12

[Doron Rosenberg (IBM)]

targetting 1.5alpha

Comment 13

[Doron Rosenberg (IBM)]

1.6a

Comment 14

[Boris 'pi' Piwinger]

Doron,

can you repair the things Ian requested and check if the patch still works in
current trunk version?

pi

Comment 15

[Doron Rosenberg (IBM)]

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.

Comment 16

[Doron Rosenberg (IBM)]

oh, and safari now too!

Comment 17

[Doron Rosenberg (IBM)]

Attachment: updated patch

Comment 18

[Doron Rosenberg (IBM)]

Attachment: add some spacing to make it more readable

Comment 19

[Hixie (not reading bugmail)]

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

Comment 20

[timeless]

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

Comment 21

[[e-mail]]

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

[timeless]

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.

Comment 23

[Doron Rosenberg (IBM)]

on IE, marquee returns the lowercased direction, even if its set to RIGHT in the
HTML.

Nitpickers! :)

Comment 24

[timeless]

that doesn't answer the question.

if you set "RIGHT", does it behave like "right" or "random-unrecognized-string"?

Comment 25

[Doron Rosenberg (IBM)]

all marquee attributes are case insensative, and always return the lowercased
version when queried via DOM.

Comment 26

[Doron Rosenberg (IBM)]

Attachment: Fixes the nitpicks and the performance issue

Comment 27

[timeless]

Comment on attachment 135400 [details] [diff] [review]
Fixes the nitpicks and the performance issue

but not the infinite loop this => myElem ?

Comment 28

[Doron Rosenberg (IBM)]

Attachment: fix the loop issue

Comment 29

[[e-mail]]

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];

Comment 30

[Doron Rosenberg (IBM)]

-> 1.7a

Comment 31

[Doron Rosenberg (IBM)]

Attachment: fix nitpicks

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.

Comment 32

[Sekundes]

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.

Comment 33

[[e-mail]]

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.

Comment 34

[Doron Rosenberg (IBM)]

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.

Comment 35

[Doron Rosenberg (IBM)]

Attachment: Fixes neil's issues, also fixes some table width in marquee problems

Comment 36

[Doron Rosenberg (IBM)]

Setting behavior/direction to "" is now fixed, it should do nothing.

Comment 37

[[e-mail]]

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.

Comment 38

[Doron Rosenberg (IBM)]

who does xbl sr's these days?

Comment 39

[Brendan Eich [:brendan]]

If you have a good r=, the sr need not be an XBl/JS whiz.

Should this patch go into 1.7b?

/be

Comment 40

[Doron Rosenberg (IBM)]

It fixes serveral IE incompatabilities and cleans up the code, so it would be
great for 1.7b.  And shouldn't regress anything.

Comment 41

[Brendan Eich [:brendan]]

Get an sr, go for patch approval, fast.

/be

Comment 42

[Brian Ryner (not reading)]

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.

Comment 43

[Doron Rosenberg (IBM)]

Attachment: The small nits Neil had are fixed here - this is to be checked in, r/sr transfer over.

Comment 44

[Doron Rosenberg (IBM)]

Since 1.7 is a long lived branch now - nominating.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 46

[Jason Barnabe (np)]

*** Bug 237314 has been marked as a duplicate of this bug. ***