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

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: chadwickgab+mozilla, Assigned: martijn.martijn)

Tracking

(Depends on 1 bug, {regression, testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(13 attachments, 5 obsolete attachments)

742 bytes, text/html; charset=iso-8859-1
Details
203 bytes, text/html
Details
239 bytes, text/html
Details
288 bytes, text/html
Details
320 bytes, text/html
Details
2.05 KB, patch
Details | Diff | Splinter Review
4.89 KB, patch
Details | Diff | Splinter Review
831 bytes, text/html
Details
156 bytes, text/html
Details
1.42 KB, text/html
Details
6.67 KB, patch
Details | Diff | Splinter Review
5.42 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
7.17 KB, patch
dholbert
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
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.
Flags: blocking1.9?
Posted file Testcase
Testcase extracted from the sph website.
I see the problem described in comment 0.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I think this was caused by the patch for bug 402567.
Blocks: 402567
Attachment #291720 - Attachment mime type: text/html → text/html; charset=iso-8859-1
Do we need to fix this for beta2?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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.
Assignee: nobody → dholbert
(In reply to comment #5)
> We probably should fix this... 
(for beta2, I mean)
Status: NEW → ASSIGNED
Keywords: qawanted
Posted file testcase 2
Posted file 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.
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.
(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
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 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.
(updating title to be more descriptive of the real problem)
Summary: Scroll bar for moving text when there should be none → Marquee GetMinWidth() is no longer 0 after bug 402567 landed
Yeah, I think you're correct with comment 13.
Keywords: testcase
Posted patch possible patch (obsolete) — Splinter Review
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 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.
Attachment #291962 - Attachment is obsolete: true
Posted patch patch?Splinter Review
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.
Posted patch patch2?Splinter Review
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>
Posted file 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.
(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.)
Posted patch patch3? (obsolete) — Splinter Review
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 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>
Attachment #292129 - Attachment is obsolete: true
So https://bugzilla.mozilla.org/attachment.cgi?id=292064 is actually rendering correctly in current trunk build?
Posted file multitest (obsolete) —
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)
Posted file multitest (attempt #2) (obsolete) —
erm..... looks like the first multitest upload was broken.  Trying again
Attachment #292450 - Attachment is obsolete: true
Aha, I was grabbing the file from the wrong folder.  Sorry 'bout that.  This one should work.
Attachment #292453 - Attachment is obsolete: true
Posted patch patch4?Splinter Review
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 :/
(Reassigning to Martijn -- hope that's ok)
Assignee: dholbert → martijn.martijn
Status: ASSIGNED → NEW
I haven't really bothered with contenteditable marquees yet.
Attachment #293877 - Flags: review?(dholbert)
Posted patch patch5? (obsolete) — Splinter Review
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.
Attachment #293881 - Flags: review?(dholbert)
   <binding id="marquee-vertical"
-           extends="chrome://xbl-marquee/content/xbl-marquee.xml#marquee"
            inheritstyle="false">

Oops, that part shouldn't be in there.
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 on attachment 293877 [details] [diff] [review]
Reftests for this bug

r=dholbert, with the 244686 reftest removed
Attachment #293877 - Flags: review?(dholbert) → review+
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.
Attachment #293881 - Flags: review?(dholbert) → review-
(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!
(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! :)
Posted patch patchv5bSplinter Review
This fixes the mistake I mentioned in comment 32 and added some comments to address the concerns in comment 35. (sorry for the delay)
Attachment #293881 - Attachment is obsolete: true
Attachment #295240 - Flags: review?(dholbert)
Comment on attachment 295240 [details] [diff] [review]
patchv5b

r=dholbert.
Thanks for fixing this!
Attachment #295240 - Flags: review?(dholbert) → review+
Attachment #295240 - Flags: superreview?(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
Attachment #295240 - Flags: superreview?(bzbarsky) → superreview+
Some one can check this in ?
Yes, sorry, I'll check this in tomorrow, when I have enough time to check the tree.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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.
Reftests checked in.
Flags: in-testsuite? → in-testsuite+
(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.
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre ID:2008011005
Duplicate of this bug: 412612
Depends on: 413027
(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.
You need to log in before you can comment on or make changes to this bug.