Bug 58806

Created attachment 90065 [details] [WORKAROUND] Reorder the divs produces expected behavior

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

Hyatt figured out that it is the trailing whitespace between the <div>s that causes the display:inline-block case to fail.

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.

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.

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.

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

Created attachment 90426 [details] [PATCH] Proposed Fix • Addressed style issue. • Updated the other progressed tests in a non-invasive way.

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.

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.

Created attachment 90427 [details] [PATCH] Proposed Fix Sorry for the spam! This removed a temporary file I accidentally had in the patch.

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.

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.

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.

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

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

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?

@leviw: would you please do a unofficial review?

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

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

> @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!

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 ***

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.