235564 - Hidden marquee doesn't start once revealed

Status: NEW

(Core :: Layout, defect)

Description

[Emmanuel Bourg]

If the display property of a <marquee> block is set to "none" when the page
loads, and then set to "block" dynamically, the block is displayed but the text
doesn't scroll.

Comment 1

[Emmanuel Bourg]

Attachment: Hidden marquee test case

Comment 2

[Emmanuel Bourg]

"visibility: hidden/visible" works fine on the other hand.

Comment 3

[Boris Zbarsky [:bzbarsky]]

the problem is that the marquee sizes onload.  It probably needs to be resized
when its display (as well as width, height, etc) are changed.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

This fixes the bug for me. But mostly I'm removing part of the code that was
added in bug 167224. I have not a real idea what that code is doing. could
someone explain, maybe?
The code that is added in the patch is a fix for the fact that when resizing a
document that has marquee in it gives problems because the start and end
position of the marquee loop is not updated (with the patch it is).

Comment 5

[Doron Rosenberg (IBM)]

(In reply to comment #3)
> the problem is that the marquee sizes onload.  It probably needs to be resized
> when its display (as well as width, height, etc) are changed.

So what is the best way to listen for such changes?

As for the patch, that only fixes simple cases by removing code that is used to
fix complex cases :)  What Boris suggested is the real way to go.

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> As for the patch, that only fixes simple cases by removing code that is used to
> fix complex cases :)  What Boris suggested is the real way to go.
What are those complex cases? Is there a marquee test matrix somewhere?
The only time I can think of when offsetWidth=0 is with display:none (or
width:0, but that would be a valid reason), and in that case the code isn't working.
Basically, I'm doing in this patch what Boris suggests, I test for width changes
during the timer.

Comment 7

[Doron Rosenberg (IBM)]

Attachment: cleaner patch

This doesn't change the fine-tuned width getter (which takes tables as a parent
for example into consideration).

The testsuite was on my netscape dev box, which is long dead.

Comment 8

[Martijn Wargers (dead)]

Your patch works fine, Doron, except on resizing the window, the marquee starts
again at the start position.
This why I had this in my patch:
-          this._doMove(false);
+          this._doMove(true);

Because otherwise this code would be followed when the width of the marquee was
changed:
            // if the direction attribute is changed, don't reset the starting
position
            if (!aSkipSettingNewPosition)
              this.newPosition = this.startAt;
          } //end if
So I think your patch needs it also.

I still can't imagine a situation where this.offsetWidth is equal to 0, except
in this situation with display:none (and off course the situaton where the width
is set to 0).

Comment 9

[Doron Rosenberg (IBM)]

(In reply to comment #8)
> Your patch works fine, Doron, except on resizing the window, the marquee starts
> again at the start position.
> This why I had this in my patch:
> -          this._doMove(false);
> +          this._doMove(true);
> 
> Because otherwise this code would be followed when the width of the marquee was
> changed:
>             // if the direction attribute is changed, don't reset the starting
> position
>             if (!aSkipSettingNewPosition)
>               this.newPosition = this.startAt;
>           } //end if
> So I think your patch needs it also.
> 
> I still can't imagine a situation where this.offsetWidth is equal to 0, except
> in this situation with display:none (and off course the situaton where the width
> is set to 0).

If you look at the log of the marquee file, you'll find the bug for why the
offsetWidth logic was added.  That code was written to fix a certain issue.

Comment 10

[Martijn Wargers (dead)]

So, do you want me to update the cleaner patch to include with, what I think is
right, or do you think the cleaner patch is ready for review?

Comment 11

[Doron Rosenberg (IBM)]

sure, post a new patch

Comment 12

[Martijn Wargers (dead)]

Attachment: patchv3

Ok, same as cleaner patch, but with this._doMove(true), see comment 8 for that.

Comment 13

[Martijn Wargers (dead)]

Attachment: patch4

This works, but is a blow for performance.

Comment 14

[Martijn Wargers (dead)]

Attachment: patch5

I guess something like this would not cause as much a performance blow, but I need to test.
It would be handy to have the onfinish event for marquees for that, I filed bug 324408 for that.

Comment 15

[Jesse Ruderman]

See also bug 166591, <marquee> created dynamically (e.g. innerHTML) is not rendered.

Comment 16

[j.j.]

Hope there is a chance for Fx3 here (and bug 166591).

Comment 17

[Martijn Wargers (dead)]

Attachment: patch

Ok, here is a new patch.
It specifically fixes this case, where the display: none rule is set on the style attribute on the marquee. It doesn't fix the case where the display: none rule is set from outside style rules.
But I think this case is probably the most used one, anyway.
And the patches that I posted previously were more hacks, than anything else.

I need to write some reftests for this patch.

Comment 18

[Martijn Wargers (dead)]

Btw, there is one known (small) issue with the patch. While toggling from display: none to display: block, the marquee seems to start over. That is not what IE is doing. But that's something for a new bug (if it's considered bad enough).

Comment 19

[Martijn Wargers (dead)]

Attachment: patch with reftests

Ok, this patch fixes the previous issue I was mentioning.
With behavior="alternate", IE is acting differently, but I think what Mozilla is doing with this patch is more correct.
The one test that tests for inequality should ideally also be identical, but I don't know how to fix that currently and IE is also failing for this case, anyway (doing something similar), so I'm leaving that now as it is, if you don't mind.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 365688 [details] [diff] [review]
patch with reftests

Initial comments, from just looking at reftests so far:

>\ No newline at end of file

It looks like all the tests files need a newline at the end.  (There shouldn't be any instances of "No newline at end of file" in the patch)

> == height-dynamic-1b.html height-dynamic-1-ref.html

It looks like these two files are actually identical in content, so you're not really testing anything there. :)

Also, I think generally it's preferred to leave JS out of reference cases -- so if possible, height-dynamic-1-ref.html shouldn't have JS in it.

Also, all three height-dynamic-1* files use JS but don't use "reftest-wait" -- if you keep the JS, make sure to add "reftest-wait".

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #20)
> Also, all three height-dynamic-1* files use JS but don't use "reftest-wait" --
> if you keep the JS, make sure to add "reftest-wait".

The same for width-dynamic-1a.html and width-dynamic-1b.html.

And width-dynamic-1-ref.html has: 
> <body onload="init()">
but no "init" function is defined.  I think you want to just get rid of the onload="init()" there.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 365688 [details] [diff] [review]
patch with reftests

A few cosmetic whitespace notes in xbl-marquee.xml:
>           //startNewDirection is true at first load and whenever the direction is changed
>           if (this.startNewDirection) {
[snip]
>+            this.startNewDirection = false;
>+            this._recalcStartAndEnd();
>             }

Fix indentation of that "}" -- should be moved left two spaces.

RE recalcStartAndEnd -- cool, it looks like you've just refactored that block of code into a method, with the code being the same except you've replaced the final " if (aResetPosition)" clause with the "//this is needed for the case when a marquee goes from display: none to visible" clause.

A few notes on that method:
>-              case "left":
[snip]
>-                this.stopAt  = this.outerDiv.offsetWidth + 
>-                               ((this._behavior == 'alternate' || this._behavior == 'slide') ? 
>-                               corrvalue : (this.innerDiv.offsetWidth + this.startAt));
[snip]
>+            case "left":
[snip]
>+              this.stopAt  = this.outerDiv.offsetWidth + 
>+                             ((this._behavior == 'alternate' || this._behavior == 'slide') ? 
>+                               corrvalue : (this.innerDiv.offsetWidth + this.startAt));

It looks like you've tweaked the indendation of that last line with respect to the original code, but both the old & new positions are actually incorrect -- it should be one space to the left of your updated position. (directly below the second paren in the line above it)

>+          if (this._behavior == 'alternate') {
>+            if ((this._direction == "right") || (this._direction == "left") && 
>+                ((this.dirsign == 1 && this.newPosition < this.startAt) ||
>+                (this.dirsign == -1 && this.newPosition > this.startAt)))

The last line above should be shifted 1 space to the right there.

>+            if ((this._direction == "up") || (this._direction == "down") && 
>+                ((this.dirsign == 1 && this.newPosition < this.startAt) ||
>+                (this.dirsign == -1 && this.newPosition > this.startAt)))

Same here.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 365688 [details] [diff] [review]
patch with reftests

A few notes about this chunk:
>+          if (this._behavior == 'alternate') {
>+            if ((this._direction == "right") || (this._direction == "left") && 
>+                ((this.dirsign == 1 && this.newPosition < this.startAt) ||
>+                (this.dirsign == -1 && this.newPosition > this.startAt)))
>+              this.newPosition = this.startAt;
>+              this.outerDiv.scrollLeft = this.newPosition;
>+
>+            if ((this._direction == "up") || (this._direction == "down") && 
>+                ((this.dirsign == 1 && this.newPosition < this.startAt) ||
>+                (this.dirsign == -1 && this.newPosition > this.startAt)))
>+              this.newPosition = this.startAt;
>+              this.outerDiv.scrollTop = this.newPosition;
>+          }

Firstly, you definitely need to add curly braces for the inner "if" clauses there, or else we'll execute the second statement in each clause regardless of the "if" condition, right?

Secondly, aren't the 'dirsign' checks redundant here? AFAICT, it it looks like the value of "_direction" already completely determines the dirsign.  So these checks could be simplified to something like:

      if ((this._direction == "left"  && this.newPosition < this.startAt) ||
          (this._direction == "right" && this.newPosition > this.startAt))
and
      if ((this._direction == "up"    && this.newPosition < this.startAt) ||
          (this._direction == "down"  && this.newPosition > this.startAt))

Unless you think it's significantly clearer the other way.  If that's the case, maybe you could add a comment noting that there's really only one possibility for each direction's dirsign, and you're just explicitly checking both to be extra clear about the sign issues.


>       <constructor>
[snip]
>+          this.runId = 1;

What's the purpose of setting runId to 1 here?  AFAICT, this has two bad effects:
 #1, when we call "start", we'll skip over the "this.runId == 0" chunk, where previously we would execute it. (Maybe that's what you want? I don't really understand why you'd want that change though.)
 #2, more serious -- it looks like this could cause problems if we call "stop()" at this point, because "1" is actually a valid timeout ID. (i.e. if we call "stop" on the marquee at this point, we'll end up calling clearTimeout(1), which could clear an unrelated & not-supposed-to-be-canceled timeout.)

Comment 24

[Daniel Holbert [:dholbert]]

Sorry, a few more questions about the same chunk referenced above.

>+          // this is needed for the case when a marquee goes from display: none to visible
>+          if (this._behavior == 'alternate') {
>+            if ((this._direction == "right") || (this._direction == "left") && 

Without stepping through execution, I'm not entirely clear on why this chunk is needed & how it fixes things. (Sorry, it's been a little while since I worked with marquees).   In particular:
  - It looks like the check for this.newPosition >/< this.startAt is your way of detecting the display:none-to-visible change -- why does newPosition have that sort of value (beyond startAt) in that case?
  - This chunk is all specifically for behavior="alternate" -- do we already handle other behaviors correctly?  Or are those fixed in different ways in other chunks of the patch?

Comment 25

[Martijn Wargers (dead)]

(In reply to comment #23)
> (From update of attachment 365688 [details] [diff] [review])
> A few notes about this chunk:

Daniel, thanks for the review comments.
I'll try the suggestions you made. Indeed that if block seems rather redundant in some way, but iirc, when I tried to make it simpler I got a reftest failure.

I could also completely remove that if block for now. I don't think it is necessary to fix this bug and it complicates things (it fixes some special cases). I would probably need to come up with some different reftests, though.

> 
> >       <constructor>
> [snip]
> >+          this.runId = 1;
> 
> What's the purpose of setting runId to 1 here?  AFAICT, this has two bad
> effects:
>  #1, when we call "start", we'll skip over the "this.runId == 0" chunk, where
> previously we would execute it. (Maybe that's what you want? I don't really
> understand why you'd want that change though.)
>  #2, more serious -- it looks like this could cause problems if we call
> "stop()" at this point, because "1" is actually a valid timeout ID. (i.e. if we
> call "stop" on the marquee at this point, we'll end up calling clearTimeout(1),
> which could clear an unrelated & not-supposed-to-be-canceled timeout.)

No, that's not happening, see in the init method. I'm storing whether runID is 0 or not in the keepRunning variable. After that I'll call the stop method, which makes runID=0. At the end of the init method I'm calling the start method again, when keepRunning==true. This fixes the case where the stop() method on the marquee is called, before the page has been loaded. This is one of the things I tested in a reftest.

Comment 26

[Martijn Wargers (dead)]

(In reply to comment #22)
> (From update of attachment 365688 [details] [diff] [review])
> RE recalcStartAndEnd -- cool, it looks like you've just refactored that block
> of code into a method, with the code being the same except you've replaced the
> final " if (aResetPosition)" clause with the "//this is needed for the case
> when a marquee goes from display: none to visible" clause.

Yes, I needed the recalcStartAndEnd part for style/width/height changes without doing what the _doMove part was doing. It turned out that it wasn't really handy to have that aResetPosition argument (I needed to transfer that argument 2 times in methods to have access to it, iirc). Instead I now call the "this.newPosition = this.startAt; this._fireEvent("start", false, false); " code at the two spots, directly.

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to comment #25)
> I'm storing whether runID is
> 0 or not in the keepRunning variable. After that I'll call the stop method,
> which makes runID=0.

Ok, but that's still bad -- before making runID=0, the stop method does:

>         if (this.runId != 0) {
>            this._deltaStartStop = Date.now()- this._lastMoveDate;
>            clearTimeout(this.runId);
>          }

which in this case means we call "clearTimeout(1)", which is bad because you don't own that timeout ID.  You could be clearing someone else's timeout, right?

Perhaps the simplest (albeit hackish) way to fix this would be to replace:
  this.runId = 1;
with:
  this.runId = setTimeout("some no-op call", "any amount of time");

That way, runId will contain a timeout ID that you *do* own, and which you know you can safely cancel.

Comment 28

[Daniel Holbert [:dholbert]]

> This fixes the case where the stop() method on
> the marquee is called, before the page has been loaded.

Ah, I think that makes sense (aside from my previous comment, RE the potential problem with hard-coding "runId=1").

Is that how IE handles this, too?  (Do they honor "stop()" calls made before the page has loaded?)

Comment 29

[Daniel Holbert [:dholbert]]

One other cosmetic fix -- it seems like as long as you're simplifying those lambda functions, we might as well get rid of the long & unneeded "myXXXFunction" names on them, right?

i.e., I'd suggest making this change (in 2 places):
  -            var lambda = function myTimeOutFunction(){myThis._doMove();}
  +            var lambda = function() { myThis._doMove(); }

and this change:
 -          var lambda = function myScopeFunction() { 
 +          var lambda = function() { 

This simplified style is already used in at least one other line in the file, here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/xbl-marquee/xbl-marquee.xml#619

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 365688 [details] [diff] [review]
patch with reftests

>       <handler event="DOMAttrModified" phase="target">
>         <![CDATA[
>+          // don't do anything for events firing for (anonymous) content inside the marquee
>+          if (event.originalTarget != this)
>+            return;

This change seems unrelated to this bug.  If you want to make this fix here, can you add a mochitest to make sure it is fixed (& stays fixed)?  Perhaps (in the test) you could tweak the style for a piece of content inside the marquee, and assert that this doesn't cause us to run the code inside your "if (attrName == 'style') {" clause, or  something like that?

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to comment #25)
> Indeed that if block seems rather redundant
> in some way, but iirc, when I tried to make it simpler I got a reftest failure.

If that change (removing the explicit "direction" checks) causes a reftest failure, that sounds like a bug somewhere...

> I could also completely remove that if block for now. I don't think it is
> necessary to fix this bug and it complicates things (it fixes some special
> cases). I would probably need to come up with some different reftests, though.

Well, if that block is necessary to fix this bug's existing reftests (which it sounds like it is?), I'd lean towards keeping it in -- it just needs a better explanation for why it's there in the comment above it (specifically, addressing the questions from comment 24).

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 365688 [details] [diff] [review]
patch with reftests

Flagging as r-, pending responses to comment 27 through comment 31.

Comment 33

[Karl Dubost💡 :karlcow]

Attachment: rendering in browsers

From Top to bottom

Firefox Nightly (Gecko) 87
Edge Chromium (Blink) 90.0.796.0
Safari (WebKit) 120 (Safari 14.2, WebKit 16612.1.2.6)

Firefox shows nothing
Edge Chromium shows a fixed text aligned on the left
Safari shows a scrolling text