413027 - Marquee height is sized too small, clipping text vertically

Status: REOPENED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Marquee height is sized too small, clipping text vertically

STEPS TO REPRODUCE
1. load the attached testcase

ACTUAL RESULT
I can see the top 3-4 pixels of the text scrolling from right to left.

EXPECTED RESULT
To see the full text scrolling from right to left.

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox trunk 2008011804 on Linux.
Bug does not occur in Firefox 2.0.0.11 on Linux.
Regression window: 2008-01-09-04 -- 2008-01-10-04  (I'm guessing bug 407016)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase2

Vertical margin doesn't seem to be taken into account when inside a display: -moz-box; overflow: hidden; box.
This is something that was fixed somehow on branch between 2007-07-09 and 2007-07-12:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-09+03&maxdate=2007-07-12+06&cvsroot=%2Fcvsroot
Somehow fixed by bug 384344 on branch?

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase3

Without using overflow: hidden;

Comment 4

[Martijn Wargers (dead)]

Attachment: reference

By using display: table; on the in-between div, I get the desired rendering. I guess I could the same as a hack for marquee, if needed.

Comment 5

[Martijn Wargers (dead)]

Attachment: patch

This fixes the bug, but I haven't tested it a lot, and I'd rather see the underlying issue fixed.

Comment 6

[Martijn Wargers (dead)]

I guess there might be a relation with bug 412646 here.

Comment 7

[Gary Johnson]

Attachment: H2 inside marquee displays incorretly

Another marquee TestCase, an H2 inside marquee displayed incorrectly

3.0b3.  regression,

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The block inside the box should be a margin-root, but it isn't.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. Most things that contain blocks have dedicated anonymous block children that we know to set NS_BLOCK_MARGIN_ROOT on. But XUL and MathML frames with block children don't have such anonymous wrappers so we don't know where to set NS_BLOCK_MARGIN_ROOT.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, nsBlockFrame::BlockIsMarginRoot takes care of that.

Where things actually go wrong is nsFrame::RefreshSizeCache where we compute the pref-size for the DIV-as-box. The pref-height and min-height are both set to just the height of the highest line-box. The ascent is set to the block's first line ascent, which includes the top margin. Then box layout goes off to compute its desired height, but by that point the bottom-margin has been completely forgotten.

This is an insane way to compute the block preferred height. Why don't we just use the height the block asked for? We did just reflow it with its preferred width, after all.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes the bug. It's the scariest patch I've seen in a while. I'll run reftests and mochitests and see how things go.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch passes reftests and mochitests.

I did some CVS archaeology. The original checkin of this approach to finding the min-height/pref-height for a box-wrapped block was in bug 110328. Naturally there's no discussion there of the new approach. The basic idea seems to have been that if 'maxElementWidth' (i.e. min-width) is the width of the widest unbreakable segment of the content, then the corresponding min-height must be the height of the tallest vertically unbreakable segment of the content, i.e. the height of the tallest line. Which makes no sense.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 306198 [details] [diff] [review]
fix

This really does seem like the right thing to do. I guess if we do it, we should do it now for beta4.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I really think we should do this.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 306198 [details] [diff] [review]
fix

OK, r+sr=dbaron.  Sorry for the delay.  I'm not sure whether this is really the right thing to do, but it's not bad, and it's better than the old behavior.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in reftest.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This caused a reftest to fail so I backed it out.

*** 11816 ERROR FAIL | Error thrown during test: document.elementFromPoint(pt.x, pt.y) is null | got 0, expected 1 | /tests/content/xul/document/test/test_bug199692.xul

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I mean a mochitest, on all 3 platforms. Which is strange since I ran mochitests and they were OK.

It may have also been responsible for 3 MathML-related reftest failures on Linux, although I don't see how:

REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/bugs/347348-1.xhtml
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/bugs/347496-1.xhtml
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/bugs/355548-3.xml

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like this wasn't responsible for those reftest failures, they persisted after the backout.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That Mochitest passes for me on Linux.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And Mac.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Windows build still in progress, but I don't expect it to be any different. I relanded this with extra code in test_bug199692.xul to tell me which part of the test actually failed (that extra code may as well stay). I'm also sort of hoping that the failure doesn't occur again.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My local Windows build passed.

Tinderboxes still fail:

*** 11823 ERROR FAIL | Point on XBL content should return element with -moz-binding style (returned null) |  | /tests/content/xul/document/test/test_bug199692.xul
*** 11824 ERROR FAIL | Error thrown during test: e is null | got 0, expected 1 | /tests/content/xul/document/test/test_bug199692.xul

Maybe I need to run the tests all in one long run like Tinderboxes do?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, if I run the tests from its parent directory, it fails! Even though it's the first test in the list!

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's probably because running from the parent directory puts the test in a small IFRAME, and the problematic elementFromPoint is testing a coordinate outside the IFRAME (and hence <window>) bounds.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For some reason, the <window> isn't growing to contain the full height of the <body> *with* my patch, but it does *without* my patch.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase showing XUL layout bug

OK, the problems with my patch are caused by a different XUL layout bug shown in this testcase.

If a XUL box contains a block whose height is equal to its min/pref height, then we don't take the ChildResized path in nsSprocketLayout because the block's rect doesn't change, so we never get around to adjusting the XUL box height for the block min-height. OTOH if the block has two lines so its height is not the height computed by the insane min/pref height code this bug removes, then the ChildResized path kicks in and the XUL box height adjusts properly.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Additional fix in nsSprocketLayout to update the frame bounds early if a child's min-size demands it. The surrounding code is horrible but looking at how originalClientRect is used later, this should be safe. I'll run reftests and mochitests but I'm reasonably confident in the patch; anything it would break must already be extremely fragile since this update would happen anyway if a child changed its size in any way.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> to update the frame bounds early

Sorry, we don't actually update the frame bounds, we just update "originalClientRect" which represents our idea of what the frame bounds should eventually be.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

Reftests found one case where we are depending on the XUL layout bug: scrollbars. nsGfxScrollFrame sets the scrollbar size to zero and expects that to be honoured even though it's below the minimum size. That was a mistake. So now I change nsGfxScrollFrame to not rely on that being honoured; hidden scrollbars are just ignored by BuildDisplayList. (Scrollbars are already ignored for computing the overflow area.)

Comment 32

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 308998 [details] [diff] [review]
fix v3

r+sr=dbaron, but I'd prefer that you leave the childResized variable in, #ifdef DEBUG, and then, in your code that handles the new condition, add an NS_ASSERTION(childResized, "this condition should be more restrictive than the old childResized condition") or something to that effect.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We do want to change the frame bounds in cases where childResized is false (i.e. the child didn't change size after we called GetMinSize on it). The testcase here, for example. So I'm not sure what you mean.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So, this causes new mochitest failures:

*** 47130 ERROR FAIL | listbox: Page down should go to the item one visible page away | got "listbox_item14", expected "listbox_item8" | /tests/toolkit/content/tests/widgets/test_hiddenpaging.xul
*** 47131 ERROR FAIL | listbox: Page down should have scrolled down a visible page | got 2, expected 7 | /tests/toolkit/content/tests/widgets/test_hiddenpaging.xul
*** 47132 ERROR FAIL | listbox: Second page down should go to the item two visible pages away | got "listbox_item14", expected "listbox_item13" | /tests/toolkit/content/tests/widgets/test_hiddenpaging.xul
*** 47133 ERROR FAIL | listbox: Second page down should not scroll beyond the end | got 6, expected 9 | /tests/toolkit/content/tests/widgets/test_hiddenpaging.xul

They're all in one test so maybe it's not severe...

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This problem is in the layout of scrollable grid row groups. We have a single listcol; when we lay it out, nsGridRowLeafLayout::PopulateBoxSizes returns a boxSizes list containing the sizes of all the rows so we end up computing a size for the listcol which is big enough for all the rows. In the listbox, the columns are in an nsStackLayout with the scrollable rows, and so the whole thing grows to the size of the largest child, which is the listcol. Effectively the desired size of the actual rows is ignored.

So this has regressed because before my change to nsSprocketLayout, the size desired by the children simply wasn't taken into account to compute the final size of the listcol. (It would have been if one of the listcol children changed size during the listcol layout, but there aren't any children in this case.)

I'm not really sure how to fix this. I'll have to think about it.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v4

Same as before, plus a fix in nsGridRowLeafLayout::PopulateBoxSizes to ignore the pref and min size contribution of rows inside a scrollable group when sizing the columns. Note that those pref and min sizes will still potentially contribute to the layout of the grid body. Seems pretty safe. Passes reftests and mochitests on Mac.

Comment 37

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Could you explain why you think the nsGridRowLeafLayout change is safe?

(Also, the patches differ since v4 doesn't have the added reftest files, but that seems ok since it looks like they weren't backed out after landing v3.)

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #37)
> (Also, the patches differ since v4 doesn't have the added reftest files, but
> that seems ok since it looks like they weren't backed out after landing v3.)

That's right.

> Could you explain why you think the nsGridRowLeafLayout change is safe?

It prevents the min/pref height of grid items in a scrollable container from contributing to the min/pref height of a grid column frame. (And similar for widths and grid row frames).

Currently on trunk a grid column has completely the wrong height in that situation --- it's the height of the full grid contents, even if the grid is shorter. Since the frame is outside the scroll frame, if it rendered anything it would render incorrectly.

The stack will still stretch the column to the height of the grid, so we're not losing anything other than the ability of the column to stretch the height of the grid, in this specific situation, which doesn't seem like something we want anyway.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Currently on trunk a grid column has completely the wrong height in that
> situation

Actually that's not true. Because of the sprocket layout bug that I'm fixing here, and the fact that the column frame never has any children, currently AFAICT the column frame never actually takes the min/pref height of its children into account.

Comment 40

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 309358 [details] [diff] [review]
fix v4

So I wrote a bunch of grid reftests to try to understand what this change is doing.  I'm not sure that I agree with the behavior they're testing, but patch v4 does significantly more than just reverting what patch v3 does to scrollable grid row groups.  In particular, it seems to make some columns disappear.  Have a look at layout/xul/base/src/grid/reftests/scrollable-columns.xul (which I'll land once the tree turns green again).

I don't think we have a good spec for what these things are supposed to do or who depends on what, but changing grid this significantly at this point scares me.  Maybe there's a simple fix, though.

Comment 41

[Damon Sicore (:damons)]

With roc out, is it reasonable to expect us to get this in beta 5?  

Comment 42

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm not sure.  I started to look at it just now, and I'll try to continue tomorrow.

I had another look at the behavior of the grid testcases that I'd written with the patch to see if it made any sense; I still don't think it does.  In one case it causes a scrollbar thumb to have a size that doesn't match the scrolled/scrollable area ratio (though it matches the ratio that it would have if the rendering were correct).  And it seems to cause some rows/columns to disappear.

Furthermore, the problem isn't just a theoretical problem with my testcases -- it breaks all the font size dropdowns in the advanced part of font preferences.  They end up too narrow (or, in the min font size case, too short -- except that seems to happen even without the grid patch), and the dropdowns are misplaced relative to the control.  (Preferences -> Content -> Fonts & Colors -> Advanced)

I have a rough idea of what I think the grid code ought to be doing -- something like ignoring row-progression-direction contributions of rows inside a scrollable rowgroup to the size of the grid as a whole (or the size of its other-direction rowgroup child) -- but I still haven't figured out the grid code well enough to do that.

Comment 43

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So, thinking about it a little more, maybe what roc did is roughly right, except that:
 * we want to do it for min size only, and not pref size
 * we also want it to apply to row groups in a scrollable row group container

Comment 44

[Damon Sicore (:damons)]

Really appreciate you taking this on, David.  Above and beyond the call. 

Comment 45

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Aha!  The other problem with roc's patch is that nsGrid::GetScrollBox always returns non-null.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It turns out that fixing this bug is going require a bunch of changes to poorly understood XUL code in ways that impact XUL apps. The original regression was really quite minor and does not justify that kind of risk, so David and I agreed to take this off the blocking list.

Comment 47

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: some patches on top of fix v3

For what it's worth, here's what I was working on top of fix v3.  It didn't yet do what I expected it do do, though.

Comment 48

[Gary Johnson]

(In reply to comment #46)
> It turns out that fixing this bug is going require a bunch of changes to poorly
> understood XUL code in ways that impact XUL apps. The original regression was
> really quite minor and does not justify that kind of risk, so David and I
> agreed to take this off the blocking list.
> 
I voted for this bug because I saw that the use of Marquee was quite popular in the Far East?  For a few references see: 

http://blog.mozilla.com/gen/2007/11/08/browser-and-web-content-compatibility-in-asia/
http://simonwillison.net/2002/Aug/30/marqueeInMozilla/

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm aware of that.

Martijn, can we take the patch in comment #5?

Comment 50

[Martijn Wargers (dead)]

Comment on attachment 298105 [details] [diff] [review]
patch

Yeah, I guess that's the only solution at this point, then.
I still need to see if this passes reftests, though (I didn't do that last time, iirc).
Also, need to test a bunch of websites and testcase, I have.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 298105 [details] [diff] [review]
patch

This should work unless the children contain table parts, but hopefully that's not going to happen.

Comment 52

[Martijn Wargers (dead)]

Attachment: workaround patch v2

Previous patch caused a reftest failure (407016-1-a.html and 407016-1-b.html).
This fixes it.
I still need to write marquee reftests for this bug itself.

Comment 53

[Martijn Wargers (dead)]

Attachment: reftest for marquee

I tested a lot of marquee pages with the workaround patch and it seems to work ok.

Comment 54

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 312019 [details] [diff] [review]
reftest for marquee

a=beltzner

Comment 55

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311598 [details] [diff] [review]
workaround patch v2

a=beltzner

Comment 56

[Martijn Wargers (dead)]

https://bugzilla.mozilla.org/attachment.cgi?id=311598 and https://bugzilla.mozilla.org/attachment.cgi?id=312019 can be checked in.
It can be that the reftest fails on Mac, because of some half pixel difference thingie (which was explained to me that's not a bug). In that case, it doesn't have to be backed out per se, but the text 'text' can be replaced with ' ', for example.

Comment 57

[Martijn Wargers (dead)]

Checking in xbl-marquee.xml;
/cvsroot/mozilla/layout/style/xbl-marquee/xbl-marquee.xml,v  <--  xbl-marquee.xml
new revision: 1.32; previous revision: 1.31
done

Checked in the workaround patch.

Comment 58

[Martijn Wargers (dead)]

https://bugzilla.mozilla.org/attachment.cgi?id=312019 (reftest for marquee) also checked in now.

Comment 59

[j.j.]

OS >> All, please

Comment 60

[Martijn Wargers (dead)]

Bug 412646 also suffered from this same issue, I think. But that bug was fixed by a workaround solution.
There is a testcase in that bug that needs to be verified, when this gets fixed:
https://bugzilla.mozilla.org/attachment.cgi?id=297757

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We checked in Martijn's workaround, can we close this bug?

Comment 62

[Martijn Wargers (dead)]

It's a workaround, no? Isn't a real fix needed here?

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess so