Bug 166528

Summary:

A floating element within <li> overlaps with the marker

Product:

WebKit

Reporter:

Carlos Alberto Lopez Perez <clopez>

Component:

Layout and Rendering

Assignee:

Carlos Alberto Lopez Perez <clopez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, darin, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan, rego, rniwa, simon.fraser, zalan

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

URL:

https://people.igalia.com/clopez/wkbug/166528/

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews117 for mac-elcapitan	none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews103 for mac-elcapitan	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none

Carlos Alberto Lopez Perez

Reported 2016-12-28 04:58:19 PST

Example URL: data:text/html,<!doctype html><ol><li><input style="float: left;" type=checkbox> Steps to reproduce the problem: 1. Go to the mentioned URL. What is the expected behavior? A marker and then a checkbox to its right. What went wrong? The marker and the checkbox overlap. This bug has been initially reported to Chromium at http://crbug.com/548616 and fixed in https://chromium.googlesource.com/chromium/src/+/c896e79e5ba348d7ed87438cd3a19d0176f3036d It also affects WebKit.

Attachments
Patch (47.55 KB, patch) 2016-12-28 06:24 PST, Carlos Alberto Lopez Perez	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.79 MB, application/zip) 2016-12-28 07:14 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.09 MB, application/zip) 2016-12-28 07:24 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews103 for mac-elcapitan (829.69 KB, application/zip) 2016-12-28 07:25 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (9.32 MB, application/zip) 2016-12-28 07:39 PST, Build Bot	no flags	Details
Patch (206.51 KB, patch) 2016-12-28 08:16 PST, Carlos Alberto Lopez Perez	no flags	Details Formatted Diff Diff
Patch (207.01 KB, patch) 2016-12-28 09:16 PST, Carlos Alberto Lopez Perez	no flags	Details Formatted Diff Diff
Patch (12.39 KB, patch) 2017-01-03 06:02 PST, Carlos Alberto Lopez Perez	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Carlos Alberto Lopez Perez

Comment 1 2016-12-28 05:02:31 PST

(In reply to comment #0) > Example URL: > data:text/html,<!doctype html><ol><li><input style="float: left;" > type=checkbox> > I uploaded the initial test case also here: https://people.igalia.com/clopez/wkbug/166528/

Carlos Alberto Lopez Perez

Comment 2 2016-12-28 06:24:07 PST

Created attachment 297803 [details] Patch Initial patch ported to WebKit with WebKitGTK+ results. Not asking for review still. Waiting for the layout tests results from the EWS

Build Bot

Comment 3 2016-12-28 07:14:20 PST

Comment on attachment 297803 [details] Patch Attachment 297803 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2798467 New failing tests: fast/lists/list-marker-before-float-nested.html fast/lists/list-marker-before-float-rtl.html fast/lists/list-marker-before-float.html fast/lists/list-marker-before-float-nested-rtl.html

Build Bot

Comment 4 2016-12-28 07:14:23 PST

Created attachment 297804 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 5 2016-12-28 07:24:43 PST

Comment on attachment 297803 [details] Patch Attachment 297803 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2798506 New failing tests: fast/lists/list-marker-before-float-nested.html fast/lists/list-marker-before-float-rtl.html fast/lists/list-marker-before-float.html fast/lists/list-marker-before-float-nested-rtl.html

Build Bot

Comment 6 2016-12-28 07:24:47 PST

Created attachment 297805 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 7 2016-12-28 07:24:59 PST

Comment on attachment 297803 [details] Patch Attachment 297803 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2798514 New failing tests: fast/lists/list-marker-before-float-nested.html fast/lists/list-marker-before-float-rtl.html fast/lists/list-marker-before-float.html fast/lists/list-marker-before-float-nested-rtl.html

Build Bot

Comment 8 2016-12-28 07:25:03 PST

Created attachment 297806 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 9 2016-12-28 07:39:30 PST

Comment on attachment 297803 [details] Patch Attachment 297803 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2798524 New failing tests: fast/lists/list-marker-before-float-nested.html fast/lists/list-marker-before-float-rtl.html fast/lists/list-marker-before-float.html fast/lists/list-marker-before-float-nested-rtl.html

Build Bot

Comment 10 2016-12-28 07:39:34 PST

Created attachment 297807 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Carlos Alberto Lopez Perez

Comment 11 2016-12-28 08:16:21 PST

Created attachment 297809 [details] Patch Second try, adding results from EWS. Still not asking for review

Carlos Alberto Lopez Perez

Comment 12 2016-12-28 09:16:20 PST

Created attachment 297813 [details] Patch All EWS green, asking for review now.

alan

Comment 13 2016-12-28 09:50:35 PST

Comment on attachment 297813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297813&action=review > Source/WebCore/ChangeLog:17 > + Computing the offset for a list marker after the rest of the objects on the line > + it is on have been laid out, means it will avoid floats it ought not to. > + > + Instead, compute the offset when laying out the marker and cache it for use later. This patch seems to be a workaround to compensate for the fact that we position the list markers way too late -during the overflow computation. (hence compute early and cache for later). > Source/WebCore/rendering/RenderListMarker.cpp:1123 > + , m_lineOffset() Not needed. > Source/WebCore/rendering/RenderListMarker.cpp:1378 > + for (RenderBox* o = parentBox(); o && o != &m_listItem; o = o->parentBox()) for (auto* box = parentBox(); box && box !=...) > Source/WebCore/rendering/RenderListMarker.h:45 > + LayoutUnit lineOffset() const { return m_lineOffset; } lineOffset is way too vague (left, right, top, bottom offset from parent, list item,containing block?). This name only works in the context of this particular patch. > LayoutTests/ChangeLog:35 > + * fast/lists/list-marker-before-float-nested-rtl.html: Added. > + * fast/lists/list-marker-before-float-nested.html: Added. > + * fast/lists/list-marker-before-float-rtl.html: Added. > + * fast/lists/list-marker-before-float.html: Added. > + * platform/gtk/fast/lists/list-marker-before-float-expected.png: Added. > + * platform/gtk/fast/lists/list-marker-before-float-expected.txt: Added. > + * platform/gtk/fast/lists/list-marker-before-float-nested-expected.png: Added. > + * platform/gtk/fast/lists/list-marker-before-float-nested-expected.txt: Added. > + * platform/gtk/fast/lists/list-marker-before-float-nested-rtl-expected.png: Added. > + * platform/gtk/fast/lists/list-marker-before-float-nested-rtl-expected.txt: Added. > + * platform/gtk/fast/lists/list-marker-before-float-rtl-expected.png: Added. > + * platform/gtk/fast/lists/list-marker-before-float-rtl-expected.txt: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-expected.png: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-expected.txt: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-nested-expected.png: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-nested-expected.txt: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-nested-rtl-expected.png: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-nested-rtl-expected.txt: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-rtl-expected.png: Added. > + * platform/ios-simulator/fast/lists/list-marker-before-float-rtl-expected.txt: Added. > + * platform/mac/fast/lists/list-marker-before-float-expected.png: Added. > + * platform/mac/fast/lists/list-marker-before-float-expected.txt: Added. > + * platform/mac/fast/lists/list-marker-before-float-nested-expected.png: Added. > + * platform/mac/fast/lists/list-marker-before-float-nested-expected.txt: Added. > + * platform/mac/fast/lists/list-marker-before-float-nested-rtl-expected.png: Added. > + * platform/mac/fast/lists/list-marker-before-float-nested-rtl-expected.txt: Added. > + * platform/mac/fast/lists/list-marker-before-float-rtl-expected.png: Added. > + * platform/mac/fast/lists/list-marker-before-float-rtl-expected.txt: Added. Ref tests should be doable.

Carlos Alberto Lopez Perez

Comment 14 2016-12-29 07:02:35 PST

(In reply to comment #13) > Comment on attachment 297813 [details] [...] > > Source/WebCore/rendering/RenderListMarker.h:45 > > + LayoutUnit lineOffset() const { return m_lineOffset; } > > lineOffset is way too vague (left, right, top, bottom offset from parent, > list item,containing block?). This name only works in the context of this > particular patch. > I don't think is vague on the context of being inside a class named "RenderListMarker". Would any of lineOffsetBeforeFloats, logicalOffsetForLine, lineOffsetForListItem sounds good to you? Otherwise I don't think is right saying that is a left/right/top/bottom offset, because it can be any of them depending for example if the page is LTR/RTL.

Carlos Alberto Lopez Perez

Comment 15 2017-01-03 06:02:14 PST

Created attachment 297932 [details] Patch Updated patch trying reftests

Carlos Alberto Lopez Perez

Comment 16 2017-01-03 07:03:29 PST

Committed r210239: <http://trac.webkit.org/changeset/210239>

Darin Adler

Comment 17 2017-01-03 11:37:47 PST

Hooray for the use of reference tests in this patch!

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