nemo Reporter
	Description • 17 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.

	David Baron :dbaron: Assignee
	Comment 2 • 17 years ago

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

	nemo Reporter
	Comment 3 • 17 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.

	Mike Schroepfer
	Comment 4 • 17 years ago

-->JST

	Mike Schroepfer
	Comment 5 • 17 years ago

Didn't mean to send this to JST --> DBaron to find right owner

	David Baron :dbaron: Assignee
	Comment 6 • 17 years ago

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.

	David Baron :dbaron: Assignee
	Comment 10 • 17 years ago

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.

	nemo Reporter
	Comment 11 • 17 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.

	David Baron :dbaron: Assignee
	Comment 12 • 17 years ago

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.

	philippe (part-time)
	Comment 13 • 17 years ago

(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.

	David Baron :dbaron: Assignee
	Comment 14 • 17 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).

	philippe (part-time)
	Comment 15 • 17 years ago

Tested on OS X 10.5

	David Baron :dbaron: Assignee
	Comment 17 • 17 years ago

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.

	philippe (part-time)
	Comment 18 • 17 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 17 years ago

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.

	David Baron :dbaron: Assignee
	Comment 20 • 17 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.

	Jeff Walden [:Waldo]
	Comment 21 • 17 years ago

There seem to be plenty of tests in the attached and committed patch, not sure why this flag wasn't already flipped.

	nemo Reporter
	Comment 22 • 17 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 17 years ago

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.

	nemo Reporter
	Comment 24 • 17 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?

	Steve England [:stevee]
	Comment 25 • 17 years ago

Yes, file a new bug and mark this bug as blocking your new one.