407016 - Marquee GetMinWidth() is no longer 0 after bug 402567 landed

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Gabriel Chadwick]

On this website http://sph.hebergemonsite.com/ there is a horizontal scroll bar in the header of the website. There should not be one.

This regressed today (2007/12/05). There was no scroll bar yesterday.

Comment 1

[Gabriel Chadwick]

Attachment: Testcase

Testcase extracted from the sph website.

Comment 2

[Kevin Brosnan]

I see the problem described in comment 0.

Comment 3

[Daniel Holbert [:dholbert]]

I think this was caused by the patch for bug 402567.

Comment 4

[Mike Schroepfer]

Do we need to fix this for beta2?

Comment 5

[Daniel Holbert [:dholbert]]

We probably should fix this... It breaks any marquee whose scrolled content is wider than the marquee itself.

The problem here is that we implement marquees using scrollframes, and nsScrollFrame::GetMinWidth() changed in the patch for bug 402567.

Previously: marquee's GetMinWidth() == 0 always
Now:        marquee's GetMinWidth() == scrolled contents' min-width

I think we probably want to keep marquee min-width at 0, the way it was before...  I'll take a look at how FF / IE / Safari handle some testcases when I'm in the office tomorrow morning, and hopefully I can address this pretty quickly.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> We probably should fix this... 
(for beta2, I mean)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: semi-reference 2

This semi-reference case for testcase 2 shows that we *don't* add horizontal scrollbars to make really-wide "overflow: hidden" items visible.  (and that's good.)

This is relevant because we implement marquees using souped-up "overflow: hidden" divs.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: reference 2 (using explicit XBL binding)

This reference case works, simply by substituting
 <span style="-moz-binding: url(chrome://xbl-marquee/content/xbl-marquee.xml#marquee-horizontal)">
for
 <marquee>

I'm not sure why this works but "testcase 2" doesn't... I'd expected that <marquee> would be equivalent to a span with an explicit marquee-XBL binding.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> This reference case works, simply by substituting

Just to be clear, 'cause it's not necessarily obvious from within my testcases:

works  = no horizontal scrollbar
broken = horizontal scrollbar

Comment 11

[Daniel Holbert [:dholbert]]

Ah, found what I'm missing.

From http://mxr.mozilla.org/seamonkey/source/layout/style/html.css#493 , real marquees also have:

  display: inline-block;

which seems to be the key.  Adding that to the span element in "reference 2" makes the scrollbar appear.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (overflow: hidden + inline-block)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 291945 [details]
testcase 3 (overflow: hidden + inline-block)

From testcase 3:
>  There should be no horizontal scrollbar on this page.

To be more correct:  **In order for our current marquee implementation to work**, there should be no horizontal scrollbar on that page.   However, it looks like our new rendering of that page (*with* the scrollbar) *is actually correct* -- this behavior matches IE7 and WebKit, at least.

So I think our marquee implementation needs to change slightly, because some assumptions that it makes (namely, that "overflow: hidden" frames have GetMinWidth() == 0)  no longer hold.

Comment 14

[Daniel Holbert [:dholbert]]

(updating title to be more descriptive of the real problem)

Comment 15

[Martijn Wargers (dead)]

Yeah, I think you're correct with comment 13.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: possible patch

This patch removes the horizontal scrollbar from Testcase, testcase2, and the original site http://sph.hebergemonsite.com/ 

(and as said in comment 13, testcase3 is actually correct as-is, with horizontal scrollbar)

Patch is based on attachment 183958 [details] [diff] [review] from bug 292690.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 291962 [details] [diff] [review]
possible patch

Nevermind, that patch is broken.

e.g. on the first Testcase for this bug, the marquee text scrolls leftwards (correctly) until its rightmost edge is aligned with the right edge of the window.  Then, the movement stops for a period of time, during which the text should be scrolling offscreen to the left. 

I think this is because the "width: 0" is preventing the horizontal margins from being created within the marquee.

Comment 18

[Martijn Wargers (dead)]

Attachment: patch?

Well, this fixes it, but I don't really like it.

This doesn't work well, when the content of the marquee is wider then the marquee itself, and you resize the window. The marquee won't widen or shrink anymore.

Comment 19

[Martijn Wargers (dead)]

Attachment: patch2?

This works for the example here, but this doesn't work inside tables, for example:
<table style="width: 200px; border: 1px solid black;"><tr><td>
<marquee scrollamount="0" style="background-color: lime;">
  <div style="width: 2000px; height: 50px;"></div>
</marquee>
</td></tr></table>

Comment 20

[Martijn Wargers (dead)]

Attachment: testcase for patch2

But that seems like a bug to me (that started when bug 402567 got fixed).
Compare this against Opera. Opera doesn't understand width: -moz-available, but width: 100% does the same job (btw, what is effectively the difference between the two?), and with width: 100%, it stays inside the table limits.
So I think Mozilla should do that too.
So I think patch2? might be the correct patch, but the brokenness inside tables has to be fixed too then.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: testcase with skinny table wrapped around marquee

(In reply to comment #19)
> This works for the example here, but this doesn't work inside tables, for
> example:

Here's a simple testcase demonstrating this.

FF2, Opera, WebKit, WinIE:
 - Table is 20px wide, showing only part of the marquee.

Trunk (with & without 'patch 2?')
 - Table grows to width of marquee contents.

To fix this behavior, we effectively need the marquee's GetMinWidth() to return 0.  (And, as described in comment 13, nsHTMLScrollFrames no longer have GetMinWidth() == 0 by default.)

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: patch3?

This modification of "patch2?" fixes the table issues.

The only change from that patch is adding "width: 0px; max-width: -moz-available" to xbl-marquee.xml:661, within "marquee-horizontal"

+        ><html:div style="display: -moz-box; margin: 0 100%; width: 0px; max-width: -moz-available"

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 292129 [details] [diff] [review]
patch3?

Nevermind, patch3 is broken on simple marquees like
<div style="width: 200px; background: lightgreen">
  <marquee>a</marquee>
</div>

Comment 24

[Martijn Wargers (dead)]

So https://bugzilla.mozilla.org/attachment.cgi?id=292064 is actually rendering correctly in current trunk build?

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: multitest

Here's a test page with marquees in various circumstances.
(to quickly see what upcoming patches fix / break)

 Current trunk: Broken on test 2,3,4,5,6
 With patch2:   Broken on test 3,5  (due to table issues mentioned in Comment 19)

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: multitest (attempt #2)

erm..... looks like the first multitest upload was broken.  Trying again

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: multitest (attempt #3)

Aha, I was grabbing the file from the wrong folder.  Sorry 'bout that.  This one should work.

Comment 28

[Martijn Wargers (dead)]

Attachment: patch4?

Well, this fixes it, but this makes bug 364434 even worse, it can cause some marquees to not work anymore in that case.
I would have hoped that margin: 0 100% would work in this patch, so I wouldn't need to add a fixed margin. But margin: 0 100% never seems to have worked the way I would have liked it, so I guess I can't blame Mozilla for that.

For some reason the RTL hack has to be removed, for some reason it isn't necessary with this patch.

Vertical marquees also would need a display: -moz-box rule, but I got weird results when I did that, see bug 407967, so I haven't done that in this patch.

All in all, not really an ideal solution, I wonder if there really is one :/

Comment 29

[Daniel Holbert [:dholbert]]

(Reassigning to Martijn -- hope that's ok)

Comment 30

[Martijn Wargers (dead)]

Attachment: Reftests for this bug

I haven't really bothered with contenteditable marquees yet.

Comment 31

[Martijn Wargers (dead)]

Attachment: patch5?

So perhaps try something like this?
It passes reftests in layout/reftests/bugs/ (even the first one from bug 403519, although that one should fail), I haven't tried the other reftests.

This makes bug 364434 better, although I'm not really sure what should be done there (doing what IE does perhaps, although that doesn't look nice). And it won't work for dynamic cases.

I haven't really bothered with contenteditable cases much (except that it passes existing reftest), I guess I could do that in a follow-up bug or something.

If someone has a better idea of how to fix this, be my guest.

Comment 32

[Martijn Wargers (dead)]

   <binding id="marquee-vertical"
-           extends="chrome://xbl-marquee/content/xbl-marquee.xml#marquee"
            inheritstyle="false">

Oops, that part shouldn't be in there.

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 293877 [details] [diff] [review]
Reftests for this bug

>RCS file: layout/reftests/bugs/244686-1-ref.html
>RCS file: layout/reftests/bugs/244686-1.html

(Looks like these files got included in this patch by accident -- they're part of bug 244686's reftest)

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 293877 [details] [diff] [review]
Reftests for this bug

r=dholbert, with the 244686 reftest removed

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 293881 [details] [diff] [review]
patch5?

>+            var width = window.getComputedStyle(this, "").width;
(...)
>+            if (width != window.getComputedStyle(this, "").width) {
>+              var width = window.getComputedStyle(this, "").width;

I don't think this "if" check will ever be true, will it?  Unless I'm missing something, this looks equivalent to:
  var foo = bar;
  (...)
  if (foo != bar) {  // This condition is never satisfied
    foo = bar;
    // more stuff
  }

>-          var lambda = function myScopeFunction() { myThis.init(); }
>+          var lambda = function myScopeFunction() { if (myThis.init) myThis.init(); }

Up through patch4, you had a comment explaining that this has to do with contenteditable marquees, but the comment is gone in patch5.  I'd suggest sticking the comment back in there to explain the change.

>-      <html:div xbl:inherits="" style="overflow: hidden; width: 100%; -moz-margin-end: 100%"
>+      <html:div style="overflow: hidden; width: -moz-available;"

Looks like the xbl:inherits="" had no function -- ok, cool. (we talked about this briefly in IRC)

Also, what's the purpose of all the s/100%/-moz-available/ changes?  I tried reverting that (changing all instances of -moz-available to 100% in xbl-marquee.xml), and after that, we still pass your new reftests, and attachment 292454 [details].

If there's a demonstrable difference between the two (with -moz-available being correct, and 100% being incorrect), could you post one more reftest that shows this difference?  Otherwise, it seems like we should leave those widths how they were, sticking with 100%.

>    <!-- use -moz-margin-end to force large intrinsic width -->

There's one last copy of this comment that needs to be removed, at line 682, just above the marquee-vertical binding definition.

Comment 36

[Martijn Wargers (dead)]

(In reply to comment #35)
> I don't think this "if" check will ever be true, will it?  Unless I'm missing
> something, this looks equivalent to:

Yes, it was necessary to fix the testcase from bug 364434: https://bugzilla.mozilla.org/attachment.cgi?id=249233
See:
+            var width = window.getComputedStyle(this, "").width;
+            this.innerDiv.parentNode.style.margin = '0 ' + width;

The adding of the margin causes the marquee to widen in that case. So to make at least the marquee working there, I have use that code there. I know it's hacky, but I don't know another way.

> Up through patch4, you had a comment explaining that this has to do with
> contenteditable marquees, but the comment is gone in patch5.  I'd suggest
> sticking the comment back in there to explain the change.

Sorry, yes, will do that. I also will remove the  xbl:inherits="" thing.

> Also, what's the purpose of all the s/100%/-moz-available/ changes?  I tried
> reverting that (changing all instances of -moz-available to 100% in
> xbl-marquee.xml), and after that, we still pass your new reftests, and
> attachment 292454 [details].
> 
> If there's a demonstrable difference between the two (with -moz-available being
> correct, and 100% being incorrect), could you post one more reftest that shows
> this difference?  Otherwise, it seems like we should leave those widths how
> they were, sticking with 100%.

See bug 163504, comment 9. It seems that dbaron preffered to used width: -moz-available here. Thanks for the review!

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to comment #36)
> (In reply to comment #35)
> > I don't think this "if" check will ever be true, will it?
> Yes, it was necessary to fix the testcase from bug 364434:
> https://bugzilla.mozilla.org/attachment.cgi?id=249233
> See:
> +            var width = window.getComputedStyle(this, "").width;
> +            this.innerDiv.parentNode.style.margin = '0 ' + width;
> 
> The adding of the margin causes the marquee to widen in that case. So to make
> at least the marquee working there, I have use that code there. I know it's
> hacky, but I don't know another way.

Ah, interesting -- I see, that's what the //xxx comment was about.  Given that it's kind of a workaround, it might be a good idea to include a link to the testcase (or bug) that we're working around, in the XXX comment. That way, down the road with future marquee fixes, we'll remember why that chunk was in there and we'll be able to easily check if it's still needed.

> See bug 163504, comment 9. It seems that dbaron preffered to used width:
> -moz-available here.

Ah -- yes, he mentions "width: shrink-wrap," which sounds like an old name for width: -moz-available.  Cool, ok.

> Thanks for the review!

Sure! :)

Comment 38

[Martijn Wargers (dead)]

Attachment: patchv5b

This fixes the mistake I mentioned in comment 32 and added some comments to address the concerns in comment 35. (sorry for the delay)

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 295240 [details] [diff] [review]
patchv5b

r=dholbert.
Thanks for fixing this!

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 295240 [details] [diff] [review]
patchv5b

> Opera doesn't understand width: -moz-available, but width: 100% does the same
> job (btw, what is effectively the difference between the two?)

Not much if your margin/padding/border are 0.  If they're not, the two give different answers.  -moz-available is basically what an in-flow non-replaced display:block element with auto width does.

> Ah -- yes, he mentions "width: shrink-wrap," which sounds like an old name for
> width: -moz-available.

"shrink-wrap" is the old name for "-moz-fit-content".  See bug 402706 for the renaming.

sr=bzbarsky

Comment 41

[Gabriel Chadwick]

Some one can check this in ?

Comment 42

[Martijn Wargers (dead)]

Yes, sorry, I'll check this in tomorrow, when I have enough time to check the tree.

Comment 43

[Martijn Wargers (dead)]

Checking in html.css;
/cvsroot/mozilla/layout/style/html.css,v  <--  html.css
new revision: 3.215; previous revision: 3.214
done
Checking in xbl-marquee/xbl-marquee.xml;
/cvsroot/mozilla/layout/style/xbl-marquee/xbl-marquee.xml,v  <--  xbl-marquee.xm
l
new revision: 1.31; previous revision: 1.30
done

Patch checked in, reftests will come in a while.

Comment 44

[Martijn Wargers (dead)]

I had to disable the reftest for bug 403519, because it failed on MacOSX Darwin 8.8.4 qm-xserve01 dep unit test.
This was sort of expected, I had some discussion with Daniel in bug 403519 where I'm arguing that the test is incorrect.

Comment 45

[Martijn Wargers (dead)]

Reftests checked in.

Comment 46

[Martijn Wargers (dead)]

(In reply to comment #28)
> Vertical marquees also would need a display: -moz-box rule, but I got weird
> results when I did that, see bug 407967, so I haven't done that in this patch.

I filed bug 411493 for that.

Comment 47

[Gabriel Chadwick]

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre ID:2008011005

Comment 49

[Martijn Wargers (dead)]

(Mainly a note to myself)
>+            if (width != window.getComputedStyle(this, "").width) {
>+              var width = window.getComputedStyle(this, "").width;
It seems to me the "var width = window.getComputedStyle(this, "").width;" line could be removed. It causes surrounding tables to unnecessary widen in some cases.