Last Comment Bug 413840 - Select spills out of containing floated block element.
: Select spills out of containing floated block element.
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9beta4
Assigned To: David Baron :dbaron: ⌚️UTC-10
:
: Jet Villegas (:jet)
Mentors:
http://m8y.org/tmp/testcase2.xhtml
Depends on: 427370
Blocks: 54696 127878 143162 163110 283798 reflow-refactor 369361
  Show dependency treegraph
 
Reported: 2008-01-24 07:19 PST by nemo
Modified: 2009-01-26 12:56 PST (History)
9 users (show)
roc: blocking1.9+
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (721 bytes, application/xhtml+xml)
2008-01-24 07:20 PST, nemo
no flags Details
two testcases (1.04 KB, text/html; charset=UTF-8)
2008-02-07 19:07 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details
some examples of lists and floats (standards mode) (959 bytes, text/html; charset=UTF-8)
2008-02-07 19:30 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details
some examples of lists and floats (quirks mode) (865 bytes, text/html; charset=UTF-8)
2008-02-07 19:31 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details
example varying padding, border, and margin (2.77 KB, text/html; charset=UTF-8)
2008-02-07 21:11 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details
patch to make lists not use -moz-float-edge (14.66 KB, patch)
2008-02-07 22:53 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch to make lists not use -moz-float-edge (20.04 KB, patch)
2008-02-08 15:28 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
test case for comment 13 (1.86 KB, text/html)
2008-02-09 03:48 PST, philippe (part-time)
no flags Details
screenshot of testcase - comment13 (10.06 KB, image/png)
2008-02-09 03:50 PST, philippe (part-time)
no flags Details
patch to make lists not use -moz-float-edge (21.52 KB, patch)
2008-02-09 13:34 PST, David Baron :dbaron: ⌚️UTC-10
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description nemo 2008-01-24 07:19:56 PST
In testcase, the containing block fails to enclose the select if it is contained in a list.
Remove the unordered list wrapping the select, unfloat the containing block, or remove the child float block on the left, and everything wraps correctly.
Comment 1 nemo 2008-01-24 07:20:34 PST
Created attachment 298941 [details]
Testcase
Comment 2 David Baron :dbaron: ⌚️UTC-10 2008-01-24 21:43:20 PST
Regressed between Lniux nightlies 2006-12-07-04-trunk and 2006-12-08-04-trunk, therefore from reflow branch landing (bug 300030).
Comment 3 nemo 2008-01-25 07:47:54 PST
That was fast.
Somehow I get the feeling you have copies of both those builds wrapped in a shell script for quick checking bugzilla attachments.

I'll review the (long) list of still-open reflow bugs, and see if I can dupe this.
Comment 4 Mike Schroepfer 2008-01-27 15:48:57 PST
-->JST
Comment 5 Mike Schroepfer 2008-01-27 16:08:37 PST
Didn't mean to send this to JST --> DBaron to find right owner
Comment 6 David Baron :dbaron: ⌚️UTC-10 2008-02-07 19:07:52 PST
Created attachment 302064 [details]
two testcases

Here's the same with another case added for comparison, to separate intrinsic width calculation from behavior at a given width a little more.

I think we ought to be doing what Safari does, at least, which is to push the select down.  But I think -moz-float-edge gets in the way.

If this is really a blocker, it may mean getting rid of -moz-float-edge, which is not a small change.
Comment 7 David Baron :dbaron: ⌚️UTC-10 2008-02-07 19:30:59 PST
Created attachment 302070 [details]
some examples of lists and floats (standards mode)
Comment 8 David Baron :dbaron: ⌚️UTC-10 2008-02-07 19:31:34 PST
Created attachment 302071 [details]
some examples of lists and floats (quirks mode)
Comment 9 David Baron :dbaron: ⌚️UTC-10 2008-02-07 21:11:10 PST
Created attachment 302078 [details]
example varying padding, border, and margin
Comment 10 David Baron :dbaron: ⌚️UTC-10 2008-02-07 22:53:55 PST
Created attachment 302085 [details] [diff] [review]
patch to make lists not use -moz-float-edge

This makes lists not use -moz-float-edge, which fixes this (the select gets bumped down) and makes us much more compatible with both Safari and IE.  (There is a slight exception:  we're less compatible with IE in the "hasLayout" case -- i.e., for IE's second type display:block rendering object.)

Safari passes the reftests included here; IE/Windows fails the background one because I trigger hasLayout there, and fails the other two because it handles the test fine but botches the reference.

I still need to look into two more issues here:
 (1) [which affects the initial testcase here] I should consider the line box's position instead of the top content edge, in case the first line box got pushed down due to wrapping around floats.
 (2) I need to write some tests for cases where the child is a block -- some issues there could be related.
Comment 11 nemo 2008-02-08 14:27:14 PST
Hm. Unfortunate.  Not clear why the select should get pushed down, but if that is the correct behaviour I'll just have to deal with it.
I really wanted a nice border wrapped around the content like IE and FF2 used to do :-/

Fortunately there are other ways to do it.
Comment 12 David Baron :dbaron: ⌚️UTC-10 2008-02-08 15:28:17 PST
Created attachment 302210 [details] [diff] [review]
patch to make lists not use -moz-float-edge

See comment 10 for a description of the main idea here.

This patch also
  * passes a correct mY in the case that the bullet is associated with a line box, and
  * cleans up the conditions for the ReflowBullet calls such that:
      + when there are no lines, we do only the call in ReflowDirtyLines and not also the one in Reflow (fixing duplicate calls)
      + when there is an empty inline line followed by another inline line, we do the ReflowBullet call in PlaceLine rather than the one in Reflow (which was for child blocks, but was getting used for inline lines as well) (changing one call to another; important because we pass the correct line position through in one but not the other)
  * adds a testcase that tests the combination of the above two changes (which also both affect the testcase in this bug)

It's still not quite optimal -- we use the floats present at the top of the block when the bullet is placed for the baseline of a child block.  (I could also use the baseline, but that would break some (more?) cases to fix others.)  We really need bug 25888 here, plus a function to get the full height of the line the bullet is associated with.  I should file a followup bug on this.  But I don't want to try to do too much for 1.9.
Comment 13 philippe (part-time) 2008-02-08 20:35:21 PST
(with the latest patch)
Is this a known issue ? On a list-item with (text-)content that wraps over multiple line, the list-marker drops to/is aligned with the second line.
Comment 14 David Baron :dbaron: ⌚️UTC-10 2008-02-09 01:49:58 PST
Could you attach a testcase showing that problem?  I don't see it with a simple one that I wrote (although I certainly believe it's possible).
Comment 15 philippe (part-time) 2008-02-09 03:48:27 PST
Created attachment 302279 [details]
test case for comment 13

Tested on OS X 10.5
Comment 16 philippe (part-time) 2008-02-09 03:50:28 PST
Created attachment 302280 [details]
screenshot of testcase - comment13
Comment 17 David Baron :dbaron: ⌚️UTC-10 2008-02-09 13:34:43 PST
Created attachment 302341 [details] [diff] [review]
patch to make lists not use -moz-float-edge

Oops, I think when I tested yesterday I was testing with an objdir that I hadn't built since applying this patch.

In any case, this fixes the regression in comment 13 by adding one condition that I'd meant to check, but forgot to.  I also added a reftest for the regression.
Comment 18 philippe (part-time) 2008-02-09 17:36:37 PST
(In reply to comment #17)
> Created an attachment (id=302341) [details]
> patch to make lists not use -moz-float-edge
 
> In any case, this fixes the regression in comment 13 by adding one condition
> that I'd meant to check, but forgot to.  I also added a reftest for the
> regression.
> 

Yup, that works fine with both the testcase and a couple of live sites where I saw the issue.
So far, I haven't found any other problem.

Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-02-10 13:26:04 PST
Comment on attachment 302341 [details] [diff] [review]
patch to make lists not use -moz-float-edge

Looks OK, but I don't really know the bullet code, so this is a bit of a rubber-stamp.
Comment 20 David Baron :dbaron: ⌚️UTC-10 2008-02-10 14:35:01 PST
Checked in to trunk, 2008-02-10 13:49/50 -0800.

Filed bug 416732 on the issue in the last paragraph of comment 12.
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2008-02-10 15:44:54 PST
There seem to be plenty of tests in the attached and committed patch, not sure why this flag wasn't already flipped.
Comment 22 nemo 2008-03-25 18:38:14 PDT
http://m8y.org/tmp/add_poc.jsp.xhtml
Close again if you feel appropriate, but I'd like to note that this fix, while it corrected the original issue identified in the testcase, caused my actual real page to once again deviate.
The div and the select list now overlap, which really shouldn't be possible since neither is positioned.
The real page is linked above - well, a slightly modified version of it anyway.
Comment 23 Ryan VanderMeulen [:RyanVM] 2008-03-25 18:41:25 PDT
If your original problem is fixed, this bug should remain closed. File a new bug for other problems you're seeing. Otherwise, bugs get incredibly complicated and difficult to follow.
Comment 24 nemo 2008-03-25 20:17:59 PDT
I was a little concerned that if the fix introduced new problems, filing a new bug would break the chain.  I suppose I can reference this one and mark it as depending?
Comment 25 Steve England [:stevee] 2008-03-26 04:43:43 PDT
Yes, file a new bug and mark this bug as blocking your new one.

Note You need to log in before you can comment on or make changes to this bug.