Select spills out of containing floated block element.

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Layout: Floats
P1
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: nemo, Assigned: dbaron)

Tracking

({regression})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Flags: blocking1.9?
(Reporter)

Comment 1

9 years ago
Created attachment 298941 [details]
Testcase
(Assignee)

Updated

9 years ago
Blocks: 300030
(Assignee)

Comment 2

9 years ago
Regressed between Lniux nightlies 2006-12-07-04-trunk and 2006-12-08-04-trunk, therefore from reflow branch landing (bug 300030).
(Reporter)

Comment 3

9 years ago
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

9 years ago
-->JST
Assignee: nobody → jst

Comment 5

9 years ago
Didn't mean to send this to JST --> DBaron to find right owner
Assignee: jst → dbaron
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
(Assignee)

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Created attachment 302070 [details]
some examples of lists and floats (standards mode)
(Assignee)

Comment 8

9 years ago
Created attachment 302071 [details]
some examples of lists and floats (quirks mode)
(Assignee)

Comment 9

9 years ago
Created attachment 302078 [details]
example varying padding, border, and margin
(Assignee)

Comment 10

9 years ago
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.
(Reporter)

Comment 11

9 years ago
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.
(Assignee)

Comment 12

9 years ago
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.
Attachment #302085 - Attachment is obsolete: true
Attachment #302210 - Flags: superreview?(roc)
Attachment #302210 - Flags: review?(roc)
(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.
(Assignee)

Comment 14

9 years ago
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).
Created attachment 302279 [details]
test case for comment 13

Tested on OS X 10.5
Created attachment 302280 [details]
screenshot of testcase - comment13
(Assignee)

Comment 17

9 years ago
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.
Attachment #302210 - Attachment is obsolete: true
Attachment #302210 - Flags: superreview?(roc)
Attachment #302210 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Attachment #302341 - Flags: superreview?(roc)
Attachment #302341 - Flags: review?(roc)
(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 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.
Attachment #302341 - Flags: superreview?(roc)
Attachment #302341 - Flags: superreview+
Attachment #302341 - Flags: review?(roc)
Attachment #302341 - Flags: review+
(Assignee)

Comment 20

9 years ago
Checked in to trunk, 2008-02-10 13:49/50 -0800.

Filed bug 416732 on the issue in the last paragraph of comment 12.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
There seem to be plenty of tests in the attached and committed patch, not sure why this flag wasn't already flipped.
Flags: in-testsuite+
(Assignee)

Updated

9 years ago
Blocks: 54696
(Assignee)

Updated

9 years ago
Blocks: 283798
(Assignee)

Updated

9 years ago
Blocks: 163110
(Assignee)

Updated

9 years ago
Blocks: 127878
(Assignee)

Updated

9 years ago
Blocks: 143162
(Assignee)

Updated

9 years ago
Target Milestone: --- → mozilla1.9beta4
(Reporter)

Comment 22

9 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 24

9 years ago
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?
Yes, file a new bug and mark this bug as blocking your new one.
(Assignee)

Updated

9 years ago
Depends on: 427370
(Assignee)

Updated

8 years ago
Blocks: 369361
You need to log in before you can comment on or make changes to this bug.