59200 - errors wrapping floats with width:auto [FLOAT]

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Josh Harding]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.0; Windows 98; DigExt)
BuildID:    2000101014

When the window is just narrower than the combined widths of boxes one and two, 
box three is displayed in the wrong location.  When box two is wrapped, box 
three is displayed above and to the right of box two.

This is incorrect behavior according to CSS1 section 4.1.4 rule #2:
"The left outer edge of a left floating element must be to the right of the 
right outer edge of every earlier (in the HTML source) left-floating element or 
the top of the former must be lower than the bottom of the latter. Analogously 
for right floating elements."

When viewed in IE5, the page is rendered properly.


Reproducible: Always
Steps to Reproduce:
1. Load page in M18
2. Adjust window width to just narrower than combined width of boxes one and 
two. 
3. Observer placement of box three

Actual Results:  Box three is above box two and not to the right of box two.

Expected Results:  Box three should be either completely below or completely to 
the right of box two.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Interesting.  Removing the <nobr> and </nobr> tags from the source makes it lay
out the boxes correctly on linux trunk build 2000110421.

Though the sizing of the divs is still a bit weird...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testase that uses CSS instead of nobr and still has the problem

Comment 3

[Hixie (not reading bugmail)]

I shall attach a series of test cases showing the various bugs present.

Comment 4

[Hixie (not reading bugmail)]

Attachment: Self explanatory testcase; shows problem just as wrapping begins.

Comment 5

[Hixie (not reading bugmail)]

Attachment: Self explanatory testcase; float size is set from remaing space next to previous float instead of from contents.

Comment 6

[Hixie (not reading bugmail)]

triaging...

Comment 7

[Erich 'Ricky' Iseli]

Attachment: Screenshot showing correct behavior.

Comment 8

[Erich 'Ricky' Iseli]

The correct behavior can be seen on build 2000110804 on Windows 98 when you
first maximize the window, reload the broken page, and finally narrow down your
window.

Comment 9

[Hixie (not reading bugmail)]

It turns out this bug completely kills the use of floats for layout -- see, for
example, the page I will attach shortly: it should have no overlap, and no
overflow in the floats. Oops.

So much for fluid document layouts.

This is _the_ floater bug I would recommend fixing for mozilla0.9.

Comment 10

[Hixie (not reading bugmail)]

Attachment: Extreme example of why this bug is bad

Comment 11

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

See bug 38157 (and bug 46285) for some previous history on this issue.

Comment 12

[Hixie (not reading bugmail)]

Note that I am assuming that the issue that floats which have wrapped end up
having their width decided by the space remaining on the *previous* `line' is
the main bug here. The issue that floats use remaining space rather than being
exclusively based on their contents may or may not be correct depending on the
outcome of a current thread in the CSS working group.

If this would be clearer with a new bug report, I would be happy to open one.
IMHO this is the most serious floater bug in Mozilla at the moment.
See http://bugzilla.mozilla.org/showattachment.cgi?attach_id=21033 for a 
particularly explicit example of why.

Comment 13

[Hixie (not reading bugmail)]

Attachment: Simple testcase. Resize the window to see the effect.

Comment 14

[karnaze (gone)]

moving to m0.9.1

Comment 15

[Chris Waterson]

Taking off buster's list.

Comment 16

[Chris Waterson]

So it looks like when troy first implemented BRS_SHRINKWRAPWIDTH, he made 
PostPlaceLine() forcibly take up the full amount of space. I'm not sure this is 
necessary. Removing this code fixes the simplest test cases in this bug without 
regressing bug 38157 (the 2nd and 3rd attachments, made by Hixie on 11/12/00 
13:55 and 13:59)

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19135
  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19136

Nevertheless, it does *not* appear to fix the originally reported bug; nor does 
it fix the original test case, or the dread ``Da Mow Mow Portal''.

I also just noticed that we appear to layout http://www.w3.org/Jigsaw/ wrong: 
note the position of the topmost graphic and the tag-line, probably worth 
another bug.

cc'ing smart people who probably know more about shrink-wrap than I do.

Comment 17

[Chris Waterson]

Attachment: don't force shrink-wrapped line to consume all the width

Comment 18

[Chris Waterson]

FWIW, the last patch makes it through the layout regression tests cleanly.

Comment 19

[Hixie (not reading bugmail)]

Chris: the damowmow portal triggers approximately a gazillion different floater 
bugs, so while your fix should probably make a difference, don't expect the 
page to magically start working either.

Comment 20

[Chris Waterson]

Attachment: work-in-progress; we need to reflow the floater as we push it

Comment 21

[Chris Waterson]

There are at least two more obvious bugs in the floater code; the above patch 
fixes one of them (for the most part). Specifically, as we advance mY to find 
space for the floater, we need to reflow the floater in the new space.

I've partially fixed this by combining the floater's reflow with the floater's 
placement, renaming PlaceFloater() to FlowAndPlaceFloater(). This method reflows 
the floater, and then attempts to place it, looping until it the floater is 
placed. There is still a bit of work to be done here, as CanPlaceFloater() will 
sometimes advance the reflow state's mY, which might necessitate more reflow.

The other bug that I discovered is that we need to obey the floated frame's 
maxElementSize, and never let the floater's width fall below that value.

Comment 22

[Chris Waterson]

Attachment: handle max-element-size, too

Comment 23

[Chris Waterson]

Okay, so I was wrong about CanPlaceFloater(); it only needs to manipulate mY so
it can ``peer'' down into the bands below. So this changes two major things
about floaters:

1. What was previously a two-call lockstep process (reflow the floater, then try
   to place it) has been combined into one call,
   nsBlockReflowState::FlowAndPlaceFloater(), which will iteratively reflow
   the floater until we find a band where the floater fits.

2. For auto-width floaters, ensures that the floater does not get flowed at a
   width less than its maxElementSize. This was obvious in the above
   test cases: if you shrank the window enough, you'd get something that
   looked like:

   +----------+
   | Supercalifragiliticexpialidocius
   +----------+

   instead of:

   +----------------------------------+
   | Supercalifragiliticexpialidocius |
   +----------------------------------+

Another issue that I'd like to address (although I'm not sure if I should
address it in a different bug), is the violation of 9.5.1 rule [2]. I think that
this may be easy to do if the block's floater cache is sorted in the same order
as the elements. (I'm not sure if that's actually the case; maybe it's already
done.) If it _were_ the case, then we could simply advance mY to the mY of the
previous floater, and start looking for space there.

Comment 24

[Chris Waterson]

FWIW, this passes the block & table layout regression tests, as well as dbaron's
floater tests (at least, it passes them as well as we used to).

Comment 25

[Chris Waterson]

Attachment: also enforce 9.5.1 rule [2]

Comment 26

[Chris Waterson]

The last patch adds a member variable |mLastFloaterY| to the block reflow state,
which maintains the y-coordinate of the last floater that we placed. This allows
us to enforce 9.5.1 rule [2], because we'll start the search for a floater's
home on the same line where the last floater was placed.

Comment 27

[Chris Waterson]

Attachment: test case for 9.5.1 rule [2]

Comment 28

[rbs]

Attachment: da mow mow portal at different sizes - exhibit missing floats

Comment 29

[rbs]

Had another look at the browser window. Actually, the floats are _not_ missing,
they are just sitting below the bigger graphics, i.e., the 'Hosting' and 
'Boilerplate' floats are placed next to the other 'W3C', 'Bookmarklets', etc.
Is this behavior also deemed correct? They appear to be really _floating_ in a 
literal sense. 

Comment 30

[Chris Waterson]

Yes, I realized the same thing. I'll need to take the test case apart to see
what's happening. My first inclination is to say, ``probably not the right thing!''

Comment 31

[rbs]

Your patch makes a significant difference! I tried viewing this dreaded Da Mow
Mow Portal without your patch and it was awful. Others cc:ed on the bug who have
regular builds can point and click to see for themselves. No way you will get
the screenshots that I attached!
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=21033
 
Since it makes the product so much better and you reported to have passed the
regressions tests, my r=rbs applies if you want to checkin.

If it goes in the tree now (before PDT locks you out for the months to come),
much testing can still be made. And as Hixie warned at the beginning, his Da Mow
Mow Portal is an extreme test case. Other simpler cases can already benefit
fully from your improvements. You can refine in subsequent iterations.

Comment 32

[Marc Attinasi]

sr=attinasi - these changes are great!

Comment 33

[Brendan Eich [:brendan]]

>If it goes in the tree now (before PDT locks you out for the months to come),

PDT isn't locking anyone out of the cvs.mozilla.org trunk for months to come. 
Talk to [e-mail] if you want to land something moderately scary
before mozilla1.0 (which we still want to be this year, but which will not be in
only a few more milestones).

/be

Comment 34

[Chris Waterson]

Verified this does not impact page load tests. Before change, I get 856msec
(average) on jrgm's tests; afterwards, I get 857msec.

Comment 35

[Chris Waterson]

Fix checked in.

Comment 36

[Hixie (not reading bugmail)]

VERIFIED that the original test case is now FIXED.

I will open new bugs for the bugs still visible on:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19134
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19135
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19136

Comment 37

[Hixie (not reading bugmail)]

FYI: the bugs are (respectively) bug 82313, bug 82314, and bug 82315.

Comment 38

[rbs]

I have this little theory that converting a resize reflow in a block with 
floaters to a style change reflow (i.e., total reflow :-) will fix these 
remaining bugs (albeit in an expensive manner...). But that is just a conjecture 
with no investigation to back it up.

No bug filed for the dreaded Da Mow Mow testcase? Does this mean, you are happy 
with the currently much improved rendering? 

Comment 39

[Hixie (not reading bugmail)]

For all intents and purposes, http://www.damowmow.com/portal/ now renders 
correctly in Mozilla (for which I am eternally grateful, Chris!). If there are
other bugs on that page, they are covered by those three bugs and the other
bugs marked as blocking bug 78094.

Comment 40

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.