Bug 58806

Summary: float:right content positioned too low after display:inline content due to whitespace
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: eric, joepeck, kevin.cs.oh, leviw, mitz, rniwa, webkit_bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
[REDUCTION] Test case
none
[WORKAROUND] Reorder the divs produces expected behavior
none
[REDUCTION] Test case 2 (display:inline-block)
none
[PATCH] Proposed Fix
joepeck: commit-queue-
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix none

Description Joseph Pecoraro 2011-04-18 12:11:00 PDT
Created attachment 90064 [details]
[REDUCTION] Test case

Attached a reduction, and a workaround. It seems a position:right object
that comes after a <div> is incorrectly positioned too low. But, reordering
the float before the <div> (so that it is positioned before the <div>), causes
the float to be positioned correctly.
Comment 1 Joseph Pecoraro 2011-04-18 12:11:28 PDT
Created attachment 90065 [details]
[WORKAROUND] Reorder the divs produces expected behavior
Comment 2 Joseph Pecoraro 2011-04-18 12:25:44 PDT
Created attachment 90068 [details]
[REDUCTION] Test case 2 (display:inline-block)

Hmm, Firefox does the same thing for the original test case. But,
if I switch the <div> content to display:inline-block. Firefox
displays it correctly and WebKit displays it even worse pushing
the (<div> even further down).
Comment 3 Joseph Pecoraro 2011-04-18 12:41:07 PDT
Hyatt figured out that it is the trailing whitespace between the <div>s that causes
the display:inline-block case to fail.
Comment 4 Joseph Pecoraro 2011-04-18 15:21:24 PDT
Comment on attachment 90064 [details]
[REDUCTION] Test case

This case works as expected. The float comes after block content so the highest it can be is after that block content.

However the display:inline case is an issue.
Comment 5 Joseph Pecoraro 2011-04-20 15:11:14 PDT
Created attachment 90424 [details]
[PATCH] Proposed Fix

This worked well for my tests cases. It seems to have caused
an issue with three other tests:

    fast/multicol/float-truncation.html
    fast/multicol/vertical-lr/float-truncation.html
    fast/multicol/vertical-rl/float-truncation.html

Those might be progressions and might just need a minor
update. So while I look at those, I'll put this patch up to get
feedback.
Comment 6 WebKit Review Bot 2011-04-20 15:14:25 PDT
Attachment 90424 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlock.h:501:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Joseph Pecoraro 2011-04-20 15:22:30 PDT
> It seems to have caused an issue with three other tests:
> 
>     fast/multicol/float-truncation.html
>     fast/multicol/vertical-lr/float-truncation.html
>     fast/multicol/vertical-rl/float-truncation.html
> 
> Those might be progressions and might just need a minor
> update. So while I look at those, I'll put this patch up to get
> feedback.

Yes, these look like a progression to me. A float:left can now fit on
the same line as the word where previously it could not due to
trailing space preventing it. I can keep the test the same by just
making the line before the float 1 character longer, causing the
wrap at the same spot it used to. I'll have a new patch up soon
with those minor changes.
Comment 8 Joseph Pecoraro 2011-04-20 15:34:13 PDT
Created attachment 90426 [details]
[PATCH] Proposed Fix

• Addressed style issue.
• Updated the other progressed tests in a non-invasive way.
Comment 9 Joseph Pecoraro 2011-04-20 15:35:43 PDT
Comment on attachment 90426 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=90426&action=review

> LayoutTests/fast/multicol/vertical-lr/float-truncation.html:93
> +            alert(tests[0] + ": " + result.width + " " + result.height)

Oops. I'll remove this. This would have only showed up if the test failed.
Comment 10 WebKit Review Bot 2011-04-20 15:36:36 PDT
Attachment 90426 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlock.h:501:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Joseph Pecoraro 2011-04-20 15:40:47 PDT
Created attachment 90427 [details]
[PATCH] Proposed Fix

Sorry for the spam! This removed a temporary file I accidentally had in the patch.
Comment 12 WebKit Review Bot 2011-04-20 15:43:58 PDT
Attachment 90427 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlock.h:501:  The parameter name "width" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Joseph Pecoraro 2011-04-20 21:48:49 PDT
I could write reftests for these, but I'm not quite for the mac ports
actually use new-run-webkit-tests. A ref test would be pretty
straightforward, as you can see from the expected pngs.
Comment 14 Eric Seidel (no email) 2011-04-22 09:12:06 PDT
Has this always been an issue, or is this a recent regression?  I'm surprised by how much code you had to add to make this work.
Comment 15 ChangSeok Oh 2011-04-22 09:34:31 PDT
(In reply to comment #14)
> Has this always been an issue, or is this a recent regression?  I'm surprised by how much code you had to add to make this work.

This patch solved bug45274 also.
Comment 16 Joseph Pecoraro 2011-04-22 10:10:20 PDT
> This patch solved bug45274 also.

Arg, that bug has a very, very similar patch. If it works in all of
my test cases that one might be the way to go. It doesn't check
if trailingWhiteSpace is true or not when it tries to position
the float ignoring the "additional space". I wonder if that would
cause an issue, or if things just work out. I'll comment there.


> Has this always been an issue, or is this a recent regression?
> I'm surprised by how much code you had to add to make this work.

You can look at ChangSeok's patch on bug45274 for a slightly
smaller patch that looks like it does the same. I actually don't think
this patch is very big, it just has some tests + pixel results.
Comment 17 Martijn T. 2011-04-22 10:32:14 PDT
I believe this is a duplicate of the bug I submitted (see the testcase by Stijn in the last comment):
https://bugs.webkit.org/show_bug.cgi?id=57441

Could someone check and mark that one as a dupe?
Comment 18 Eric Seidel (no email) 2011-04-26 16:22:54 PDT
@leviw: would you please do a unofficial review?
Comment 19 Joseph Pecoraro 2011-04-26 16:24:16 PDT
(In reply to comment #18)
> @leviw: would you please do a unofficial review?

Also, compare this to the patch on bug 45274. That patch may be considered cleaner.
Comment 20 Levi Weintraub 2011-04-27 15:45:35 PDT
Comment on attachment 90427 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=90427&action=review

This seems like a valid fix, but like you said, the patch on bug 45274 appears simpler so I'd lean towards that approach. That said, I'd want your layout tests rolled into the other patch, as you appear to cover more cases (and fix the three tests that need an extra character to yield the same results :)

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1733
> +    Vector<RenderObject*> trailingFloatingBoxes;
> +    Vector<FloatingObject*> trailingFloatingObjects;

I suspect we'd want to reserve space here to avoid mallocing.
Comment 21 ChangSeok Oh 2011-04-28 09:05:45 PDT
(In reply to comment #20)
> (From update of attachment 90427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90427&action=review
> 
> This seems like a valid fix, but like you said, the patch on bug 45274 appears simpler so I'd lean towards that approach. That said, I'd want your layout tests rolled into the other patch, as you appear to cover more cases (and fix the three tests that need an extra character to yield the same results :)

@Joseph:
I'm willing to merge your test cases to the patch on bug45274, if you don't mind. :)
Comment 22 Joseph Pecoraro 2011-04-28 13:12:26 PDT
> @Joseph:
> I'm willing to merge your test cases to the patch on bug45274, if you don't mind. :)

Absolutely. They need to be rebased anyways (no need for the .checksum files anymore,
as they were removed after I posted my patch). Thanks!
Comment 23 Joseph Pecoraro 2011-04-29 09:45:32 PDT
Clearing my flags. I'll dup this to ChangSeok's bug. He has
posted an update with the best of both patches.

*** This bug has been marked as a duplicate of bug 45274 ***
Comment 24 Darin Adler 2014-04-24 16:45:19 PDT
Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.