Bug 192980 - [css-lists] Fix list marker issues related to line breaks and markers disappearance
Summary: [css-lists] Fix list marker issues related to line breaks and markers disappe...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords:
: 13332 (view as bug list)
Depends on: 193273
Blocks: 196489
  Show dependency treegraph
 
Reported: 2018-12-21 04:05 PST by cathiechen
Modified: 2019-06-16 20:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.59 KB, patch)
2018-12-21 04:09 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (28.96 KB, patch)
2018-12-21 05:15 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (29.04 KB, patch)
2018-12-21 05:28 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (29.05 KB, patch)
2018-12-21 06:12 PST, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.90 MB, application/zip)
2018-12-21 07:15 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.54 MB, application/zip)
2018-12-21 07:26 PST, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (13.08 MB, application/zip)
2018-12-21 07:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (2.45 MB, application/zip)
2018-12-21 08:05 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.86 MB, application/zip)
2018-12-21 08:07 PST, Build Bot
no flags Details
Patch (31.23 KB, patch)
2018-12-24 03:41 PST, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (3.64 MB, application/zip)
2018-12-24 04:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (4.14 MB, application/zip)
2018-12-24 04:54 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.18 MB, application/zip)
2018-12-24 05:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (3.63 MB, application/zip)
2018-12-24 05:39 PST, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (13.83 MB, application/zip)
2018-12-24 08:19 PST, Build Bot
no flags Details
Patch (31.53 KB, patch)
2018-12-25 07:42 PST, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.76 MB, application/zip)
2018-12-25 08:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.41 MB, application/zip)
2018-12-25 08:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (2.32 MB, application/zip)
2018-12-25 09:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (13.06 MB, application/zip)
2018-12-25 09:36 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.78 MB, application/zip)
2018-12-25 09:37 PST, Build Bot
no flags Details
Patch (470.95 KB, patch)
2018-12-27 02:04 PST, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.89 MB, application/zip)
2018-12-27 03:40 PST, Build Bot
no flags Details
Patch (467.18 KB, patch)
2018-12-27 04:55 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (466.67 KB, patch)
2018-12-27 19:05 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (470.79 KB, patch)
2019-01-01 06:43 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (75.88 KB, patch)
2019-01-08 22:35 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (75.88 KB, patch)
2019-01-08 22:39 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (470.64 KB, patch)
2019-01-10 06:53 PST, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.47 MB, application/zip)
2019-01-10 07:58 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.10 MB, application/zip)
2019-01-10 08:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (2.03 MB, application/zip)
2019-01-10 08:49 PST, Build Bot
no flags Details
Patch (472.38 KB, patch)
2019-01-10 23:27 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (472.23 KB, patch)
2019-01-13 19:19 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (472.83 KB, patch)
2019-01-19 06:40 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (473.36 KB, patch)
2019-04-02 04:47 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2018-12-21 04:05:58 PST
Eliminate line-breaks and disappear of list marker
Comment 1 cathiechen 2018-12-21 04:09:34 PST
Created attachment 357937 [details]
Patch
Comment 2 Rob Buis 2018-12-21 04:50:41 PST
Comment on attachment 357937 [details]
Patch

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

> Source/WebCore/rendering/RenderListItem.h:65
> +    bool needBlockDirectionAlign() { return m_needBlockDirectionAlign; }

This should be const.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:79
> +        return;

You probably want to check this before cloning.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:170
> +                else

The indenting is weird here, does check-webkit-style say anything?
Comment 3 cathiechen 2018-12-21 04:59:46 PST
Comment on attachment 357937 [details]
Patch

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

>> Source/WebCore/rendering/RenderListItem.h:65
>> +    bool needBlockDirectionAlign() { return m_needBlockDirectionAlign; }
> 
> This should be const.

done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:79
>> +        return;
> 
> You probably want to check this before cloning.

done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:170
>> +                else
> 
> The indenting is weird here, does check-webkit-style say anything?

Nope, it made me delete the one-line braces at first.
Comment 4 cathiechen 2018-12-21 05:15:46 PST
Created attachment 357938 [details]
Patch
Comment 5 Build Bot 2018-12-21 05:18:29 PST
Attachment 357938 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: LayoutTests/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 12 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 cathiechen 2018-12-21 05:28:31 PST
Created attachment 357939 [details]
Patch
Comment 7 cathiechen 2018-12-21 06:12:25 PST
Created attachment 357940 [details]
Patch
Comment 8 Build Bot 2018-12-21 07:15:37 PST
Comment on attachment 357940 [details]
Patch

Attachment 357940 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10506751

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/block/float/float-not-removed-crash.html
imported/blink/fast/lists/list-inside-columns-crash.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 9 Build Bot 2018-12-21 07:15:39 PST
Created attachment 357942 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 Build Bot 2018-12-21 07:26:52 PST
Comment on attachment 357940 [details]
Patch

Attachment 357940 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10506865

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/block/float/float-not-removed-crash.html
imported/blink/fast/lists/list-inside-columns-crash.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 11 Build Bot 2018-12-21 07:26:54 PST
Created attachment 357943 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 12 Build Bot 2018-12-21 07:44:25 PST
Comment on attachment 357940 [details]
Patch

Attachment 357940 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10506940

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/block/float/float-not-removed-crash.html
imported/blink/fast/lists/list-inside-columns-crash.html
fast/multicol/multicol-li-crash.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 13 Build Bot 2018-12-21 07:44:36 PST
Created attachment 357944 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 14 Build Bot 2018-12-21 08:05:21 PST
Comment on attachment 357940 [details]
Patch

Attachment 357940 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10506911

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/block/float/float-not-removed-crash.html
imported/blink/fast/lists/list-inside-columns-crash.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 15 Build Bot 2018-12-21 08:05:23 PST
Created attachment 357945 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 Build Bot 2018-12-21 08:07:52 PST
Comment on attachment 357940 [details]
Patch

Attachment 357940 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10506912

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/forms/form-hides-table.html
fast/block/float/float-not-removed-crash.html
imported/blink/fast/lists/list-inside-columns-crash.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 17 Build Bot 2018-12-21 08:07:53 PST
Created attachment 357946 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 18 Manuel Rego Casasnovas 2018-12-23 04:32:48 PST
Comment on attachment 357940 [details]
Patch

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

Thanks for the patch, I did an initial review with lots of comments.
I didn't have time to look into the tests yet, but it'd be nice to use WPT instead to share them with Chromium and Firefox.

This was fixed in Chromium first, could you link the Chromium bug in "See also" field? Thanks.

> Source/WebCore/ChangeLog:9
> +        is marker has been added as the child of this overflow block. And the

Nit: "is that marker has been added"

> Source/WebCore/ChangeLog:11
> +        sibilng of a block child. To fix these two kinds of issue:

Is there a way to divide these two fixes in two separated patches, or the code is the same for both things?
I have the feeling that we're fixing two things in this patch one releated to line breaks and another related to overflow: hidden.

> Source/WebCore/ChangeLog:12
> +        1. Create a zero-height anonymouse parent for marker(to eleminate the

Typo: s/anonymouse/anonymous/
Nit: Missing space before "("

> Source/WebCore/ChangeLog:15
> +        has been layout (marker has to adjust itselt to the content of li)

Typo: s/has/have/

> Source/WebCore/ChangeLog:30
> +        (WebCore::RenderListItem::alignMarkerInBlockDirection):

As this method is quite complex, it'd be nice to have a summary of what it does here.

> Source/WebCore/ChangeLog:36
> +        (WebCore::RenderTreeBuilder::List::updateItemMarker):

And the same in this case.

> Source/WebCore/rendering/RenderListItem.cpp:445
> +// Align marker_inline_box in block direction according to line_box_root's baseline.

Nit: s/marker_inline_box/markerInlineBox/
Also I guess s/line_box_root/RootInlineBox/ ?

> Source/WebCore/rendering/RenderListItem.cpp:460
> +    }

Nit: This variable could be initialized like:
  backToOriginalBaseline = !baselineProvider || baselineProvider->isWritingModeRoot();

If this is true couldn't we just return and avoid doing anything in this method?
Aren't we keeping the old behavior? Or are we doing something else?

> Source/WebCore/rendering/RenderListItem.cpp:463
> +    RootInlineBox& markerRoot = markerInlineBox->root();

Is it guaranteed that markerInlineBox is never null? Should we add an ASSERT at least?

> Source/WebCore/rendering/RenderListItem.cpp:465
> +        // If marker_ and baselineProvider share a same RootInlineBox, no need to align marker.

Nit: "marker_" I guess that name might come from Blink and it should be "m_marker" here.

> Source/WebCore/rendering/RenderListItem.cpp:466
> +        if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)

We don't need two ifs, we can combine both in just one if.

> Source/WebCore/rendering/RenderListItem.cpp:473
> +        offset = box->firstLineBaseline().value();

Wouldn't we need "valueOr()" to be safe here?

Nit: Couldn't we write this like:
  offest = downcast<RenderBox>(baslineProvider)->firstLineBaseline().value();

> Source/WebCore/rendering/RenderListItem.cpp:477
> +        baselineProvider = m_marker->containingBlock();

Again I guess we're sure containingBlock() is never null here.

> Source/WebCore/rendering/RenderListItem.cpp:482
> +    if (offset != -1) {

I'd change this for something like:
  if (offset == -1)
      return;

> Source/WebCore/rendering/RenderListItem.cpp:489
> +        // BaselinePosition is workable when marker is an image.

What do you mean here by "is workable"?

> Source/WebCore/rendering/RenderListItem.cpp:491
> +        // So use markerFontMetrics.Ascent when marker is text.

Nit: s/Ascent/ascent/

> Source/WebCore/rendering/RenderListItem.cpp:492
> +        if (m_marker->isImage())

I see that RenderListMarker has a method baselinePosition() why we cannot use that?
I guess it provides wrong information, then why we don't modify it?

> Source/WebCore/rendering/RenderListItem.cpp:496
> +            const FontMetrics& markerFontMetrics = m_marker->style().fontMetrics();
> +            offset -= markerFontMetrics.ascent(markerRoot.baselineType());

Nit: I'd write this as:
  offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType());

> Source/WebCore/rendering/RenderListItem.h:64
> +    void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign = need; }

Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/

> Source/WebCore/rendering/RenderListItem.h:65
> +    bool needBlockDirectionAlign() const { return m_needBlockDirectionAlign; }

Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52
> -        if (!is<RenderBlock>(child) || is<RenderTable>(child) || is<RenderRubyAsBlock>(child))
> -            break;
> +        if (current.hasOverflowClip())
> +            return &current;
> +
> +        if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())))
> +            return &downcast<RenderBlock>(child);

I don't understand this change very well.
For the case of isWritingModeRoot() it was breaking before (so it returned nullptr) and now it's returning the child...
Is this the expected bahvavior? Doesn't it affect other cases?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:114
> +    // 1. Place marker as a child of <li>. Make sure don't share parent with empty inline elements which don't generate InlineBox.

Typo: "Make sure it doesn't share parent"

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120
> +    // Prepare for block direction align

Nit: Comments should finish in a "dot".

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:126
> +            // So add IsInside() here.

Nit: s/IsInside/isInisde/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130
> +                    forceLogicalHeight(*currentParent, Length(style.logicalHeight()));

Isn't enough just calling setLogicalHeigth()?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:133
> +                // We need to remove marker here.E.g: <li><span><div>text<div><span></li>

Mmmm, what's the expected behavior for this example?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139
> +                forceLogicalHeight(*currentParent, Length(0, Fixed));

Same question than before regarding setLogicalHeight().

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:152
> +                    forceLogicalHeight(*markerContainer, Length(0, Fixed));

Ditto.

> LayoutTests/ChangeLog:10
> +        marker in li + block overflow:hiddlen child, and testing if the acting

s/acting/behavior/

> LayoutTests/fast/lists/add-inline-child-after-marker-expected.html:1
> +<head></head><body><ul>

Tests need to start with <!DOCTYPE html> unless you're explicitly testing quirks mode (which I don't think is the case).
Comment 19 cathiechen 2018-12-24 03:41:03 PST
Created attachment 358035 [details]
Patch
Comment 20 cathiechen 2018-12-24 03:41:51 PST
Comment on attachment 357940 [details]
Patch

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

Hi Rego, Thanks very much for the thorough review:)
And sorry for the typos.

>> Source/WebCore/ChangeLog:9
>> +        is marker has been added as the child of this overflow block. And the
> 
> Nit: "is that marker has been added"

Done

>> Source/WebCore/ChangeLog:11
>> +        sibilng of a block child. To fix these two kinds of issue:
> 
> Is there a way to divide these two fixes in two separated patches, or the code is the same for both things?
> I have the feeling that we're fixing two things in this patch one releated to line breaks and another related to overflow: hidden.

Nope, it couldn't be separated.
Logically, this design would fit for all conditions. In this patch, we let li + overflow, flex, etc. use it.

>> Source/WebCore/ChangeLog:12
>> +        1. Create a zero-height anonymouse parent for marker(to eleminate the
> 
> Typo: s/anonymouse/anonymous/
> Nit: Missing space before "("

Done

>> Source/WebCore/ChangeLog:15
>> +        has been layout (marker has to adjust itselt to the content of li)
> 
> Typo: s/has/have/

Done

>> Source/WebCore/ChangeLog:30
>> +        (WebCore::RenderListItem::alignMarkerInBlockDirection):
> 
> As this method is quite complex, it'd be nice to have a summary of what it does here.

Done

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::RenderTreeBuilder::List::updateItemMarker):
> 
> And the same in this case.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:445
>> +// Align marker_inline_box in block direction according to line_box_root's baseline.
> 
> Nit: s/marker_inline_box/markerInlineBox/
> Also I guess s/line_box_root/RootInlineBox/ ?

Done

>> Source/WebCore/rendering/RenderListItem.cpp:460
>> +    }
> 
> Nit: This variable could be initialized like:
>   backToOriginalBaseline = !baselineProvider || baselineProvider->isWritingModeRoot();
> 
> If this is true couldn't we just return and avoid doing anything in this method?
> Aren't we keeping the old behavior? Or are we doing something else?

Done.

In this case, because marker might be moved to other position at last layout pass.
So we have to make sure it back to its original baseline, couldn't just return.

>> Source/WebCore/rendering/RenderListItem.cpp:463
>> +    RootInlineBox& markerRoot = markerInlineBox->root();
> 
> Is it guaranteed that markerInlineBox is never null? Should we add an ASSERT at least?

Done

>> Source/WebCore/rendering/RenderListItem.cpp:465
>> +        // If marker_ and baselineProvider share a same RootInlineBox, no need to align marker.
> 
> Nit: "marker_" I guess that name might come from Blink and it should be "m_marker" here.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:466
>> +        if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)
> 
> We don't need two ifs, we can combine both in just one if.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:473
>> +        offset = box->firstLineBaseline().value();
> 
> Wouldn't we need "valueOr()" to be safe here?
> 
> Nit: Couldn't we write this like:
>   offest = downcast<RenderBox>(baslineProvider)->firstLineBaseline().value();

Yes, using value() directly would cause a crash issue. I use Optional in the new patch.

RenderBox::firstLineBaseline() is public.
RenderBlock::firstLineBaseline() is protected.
In order to call it, I have to cast baslineProvider from RenderBlock to RenderBox.
RenderBox is the parent class, so it isn't downcast...

>> Source/WebCore/rendering/RenderListItem.cpp:477
>> +        baselineProvider = m_marker->containingBlock();
> 
> Again I guess we're sure containingBlock() is never null here.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:482
>> +    if (offset != -1) {
> 
> I'd change this for something like:
>   if (offset == -1)
>       return;

Use this format in the new patch.

    if (offset)
        LayoutUnit offsetValue = offset.value();

>> Source/WebCore/rendering/RenderListItem.cpp:489
>> +        // BaselinePosition is workable when marker is an image.
> 
> What do you mean here by "is workable"?

I mean RenderListMarker::baselinePosition() return the right value when marker is image.

>> Source/WebCore/rendering/RenderListItem.cpp:491
>> +        // So use markerFontMetrics.Ascent when marker is text.
> 
> Nit: s/Ascent/ascent/

Done

>> Source/WebCore/rendering/RenderListItem.cpp:492
>> +        if (m_marker->isImage())
> 
> I see that RenderListMarker has a method baselinePosition() why we cannot use that?
> I guess it provides wrong information, then why we don't modify it?

Yes, it return baselinePosition of list item when marker isn't image. This isn't right.

Corrected baselinePosition() and lineheight() in the new patch

>> Source/WebCore/rendering/RenderListItem.cpp:496
>> +            offset -= markerFontMetrics.ascent(markerRoot.baselineType());
> 
> Nit: I'd write this as:
>   offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType());

Done

>> Source/WebCore/rendering/RenderListItem.h:64
>> +    void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign = need; }
> 
> Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/

Done

>> Source/WebCore/rendering/RenderListItem.h:65
>> +    bool needBlockDirectionAlign() const { return m_needBlockDirectionAlign; }
> 
> Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52
>> +            return &downcast<RenderBlock>(child);
> 
> I don't understand this change very well.
> For the case of isWritingModeRoot() it was breaking before (so it returned nullptr) and now it's returning the child...
> Is this the expected bahvavior? Doesn't it affect other cases?

We use getParentOfFirstLineBox to determine if this list marker should use the new design.
And use getParentOfFirstLineBox as the baselineprovider in alignMarkerInBlockDirection().

And yes, for list-style-position:inside cases, this might cause crash issues.
The new patch would deal with it.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:114
>> +    // 1. Place marker as a child of <li>. Make sure don't share parent with empty inline elements which don't generate InlineBox.
> 
> Typo: "Make sure it doesn't share parent"

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120
>> +    // Prepare for block direction align
> 
> Nit: Comments should finish in a "dot".

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:126
>> +            // So add IsInside() here.
> 
> Nit: s/IsInside/isInisde/

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130
>> +                    forceLogicalHeight(*currentParent, Length(style.logicalHeight()));
> 
> Isn't enough just calling setLogicalHeigth()?

Not sure if I understand the question correctly.
We need use setStyle to make sure it would trigger needsLayout().
And it calls three times, so create a function for them.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:133
>> +                // We need to remove marker here.E.g: <li><span><div>text<div><span></li>
> 
> Mmmm, what's the expected behavior for this example?

In this case, marker might occupy a anonymous block.
<li>
  <anonymous><span>m_marker</anonymous>
  <div>text<div>
  <anonymous><span></anonymous>
</li>

We need detach m_marker from render tree, and insert it to newParent. So the render tree would like this:
<li>
  <anonymous><span></anonymous>
  <div>m_marker text<div>
  <anonymous><span></anonymous>
</li>

Explained more about it in the new patch.

>> LayoutTests/ChangeLog:10
>> +        marker in li + block overflow:hiddlen child, and testing if the acting
> 
> s/acting/behavior/

Done

>> LayoutTests/fast/lists/add-inline-child-after-marker-expected.html:1
>> +<head></head><body><ul>
> 
> Tests need to start with <!DOCTYPE html> unless you're explicitly testing quirks mode (which I don't think is the case).

Done
Comment 21 Build Bot 2018-12-24 04:43:36 PST
Comment on attachment 358035 [details]
Patch

Attachment 358035 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10534026

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/lists/list-and-margin-collapse.html
fast/css-generated-content/table-with-before.html
fast/css/only-child-pseudo-class.html
fast/css/empty-pseudo-class.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/css/first-child-pseudo-class.html
fast/forms/form-hides-table.html
css2.1/20110323/height-applies-to-010a.htm
fast/lists/list-marker-before-overflow-hidden.html
fast/css/last-of-type-pseudo-class.html
fast/css/last-child-pseudo-class.html
fast/css/only-of-type-pseudo-class.html
fast/css/first-of-type-pseudo-class.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 22 Build Bot 2018-12-24 04:43:37 PST
Created attachment 358036 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 23 Build Bot 2018-12-24 04:54:29 PST
Comment on attachment 358035 [details]
Patch

Attachment 358035 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10534039

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css/first-of-type-pseudo-class.html
fast/css-generated-content/table-with-before.html
fast/css/only-child-pseudo-class.html
fast/css/empty-pseudo-class.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/css/first-child-pseudo-class.html
fast/css/last-child-pseudo-class.html
fast/forms/form-hides-table.html
css2.1/20110323/height-applies-to-010a.htm
fast/css/last-of-type-pseudo-class.html
fast/lists/list-marker-before-overflow-hidden.html
fast/css/only-of-type-pseudo-class.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 24 Build Bot 2018-12-24 04:54:31 PST
Created attachment 358037 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 25 Build Bot 2018-12-24 05:33:42 PST
Comment on attachment 358035 [details]
Patch

Attachment 358035 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10534085

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/lists/list-and-margin-collapse.html
fast/css-generated-content/table-with-before.html
fast/css/only-child-pseudo-class.html
fast/css/empty-pseudo-class.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/css/first-child-pseudo-class.html
fast/forms/form-hides-table.html
css2.1/20110323/height-applies-to-010a.htm
fast/lists/list-marker-before-overflow-hidden.html
fast/css/last-of-type-pseudo-class.html
fast/css/last-child-pseudo-class.html
fast/css/only-of-type-pseudo-class.html
fast/css/first-of-type-pseudo-class.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 26 Build Bot 2018-12-24 05:33:44 PST
Created attachment 358038 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 27 Build Bot 2018-12-24 05:39:05 PST
Comment on attachment 358035 [details]
Patch

Attachment 358035 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10534102

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css/first-of-type-pseudo-class.html
fast/css-generated-content/table-with-before.html
fast/css/only-child-pseudo-class.html
fast/css/empty-pseudo-class.html
fast/css-generated-content/table-row-with-before.html
fast/css/first-child-pseudo-class.html
editing/pasteboard/innerText-inline-table.html
fast/css/last-child-pseudo-class.html
fast/forms/form-hides-table.html
css2.1/20110323/height-applies-to-010a.htm
fast/css/last-of-type-pseudo-class.html
fast/lists/list-marker-before-overflow-hidden.html
fast/css/only-of-type-pseudo-class.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 28 Build Bot 2018-12-24 05:39:07 PST
Created attachment 358039 [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.13.6
Comment 29 Build Bot 2018-12-24 08:19:24 PST
Comment on attachment 358035 [details]
Patch

Attachment 358035 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10535130

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css/first-of-type-pseudo-class.html
fast/css-generated-content/table-with-before.html
fast/css/only-child-pseudo-class.html
fast/css/empty-pseudo-class.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/css/first-child-pseudo-class.html
fast/css/last-child-pseudo-class.html
fast/forms/form-hides-table.html
css2.1/20110323/height-applies-to-010a.htm
fast/css/last-of-type-pseudo-class.html
fast/lists/list-marker-before-overflow-hidden.html
fast/css/only-of-type-pseudo-class.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 30 Build Bot 2018-12-24 08:19:37 PST
Created attachment 358041 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 31 cathiechen 2018-12-25 07:42:48 PST
Created attachment 358063 [details]
Patch
Comment 32 Build Bot 2018-12-25 08:46:03 PST
Comment on attachment 358063 [details]
Patch

Attachment 358063 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10543823

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 33 Build Bot 2018-12-25 08:46:05 PST
Created attachment 358065 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 34 Build Bot 2018-12-25 08:55:04 PST
Comment on attachment 358063 [details]
Patch

Attachment 358063 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10543839

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 35 Build Bot 2018-12-25 08:55:06 PST
Created attachment 358066 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 36 Build Bot 2018-12-25 09:34:36 PST
Comment on attachment 358063 [details]
Patch

Attachment 358063 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10543972

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 37 Build Bot 2018-12-25 09:34:38 PST
Created attachment 358067 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 38 Build Bot 2018-12-25 09:36:19 PST
Comment on attachment 358063 [details]
Patch

Attachment 358063 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10544054

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/lists/list-marker-before-content-table.html
fast/forms/form-hides-table.html
webanimations/leak-document-with-web-animation.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 39 Build Bot 2018-12-25 09:36:31 PST
Created attachment 358068 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 40 Build Bot 2018-12-25 09:36:59 PST
Comment on attachment 358063 [details]
Patch

Attachment 358063 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10543976

New failing tests:
fast/css-generated-content/table-row-group-with-before.html
fast/css-generated-content/table-with-before.html
fast/css-generated-content/table-row-with-before.html
fast/forms/form-hides-table.html
fast/lists/list-marker-before-overflow-hidden.html
fast/lists/list-and-margin-collapse.html
fast/lists/ordered-list-with-no-ol-tag.html
Comment 41 Build Bot 2018-12-25 09:37:01 PST
Created attachment 358069 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 42 cathiechen 2018-12-27 02:04:41 PST
Created attachment 358105 [details]
Patch
Comment 43 Build Bot 2018-12-27 03:40:32 PST
Comment on attachment 358105 [details]
Patch

Attachment 358105 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10558823

New failing tests:
fast/forms/form-hides-table.html
Comment 44 Build Bot 2018-12-27 03:40:44 PST
Created attachment 358106 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 45 cathiechen 2018-12-27 04:55:55 PST
Created attachment 358109 [details]
Patch
Comment 46 cathiechen 2018-12-27 19:05:14 PST
Created attachment 358114 [details]
Patch
Comment 47 Manuel Rego Casasnovas 2018-12-28 01:47:12 PST
Comment on attachment 358114 [details]
Patch

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

I have added a few extra comments on the patch, anyway I'd wait for WPT tests before merging this.

The results in the rebaselined tests look much better.

> Source/WebCore/ChangeLog:37
> +

Nit: You need to regenrate ChangeLog as some changes from the patch are missing (for example the change in RenderBlock::isSelfCollapsingBlock()).

> Source/WebCore/rendering/RenderListItem.cpp:472
> +        assert(baselineProvider);

Why assert and not ASSERT?

> Source/WebCore/rendering/RenderListItem.cpp:474
> +        RenderBox* box = baselineProvider;
> +        offset = box->firstLineBaseline();

So this is done because RenderBlock::firstLineBaseline() is protected,
wouldn't make sense to make it public?
Specially if RenderBox::firstLineBaseline() is already public.

Is this ending up calling RenderBlock::firstLineBaseline() or it only calls RenderBox::firstLineBaseline(),
if it only goes to the RenderBox method we don't need to actually call it as it only returns "Optional<int>()".

> Source/WebCore/rendering/RenderListItem.cpp:496
> +        for (RenderBox* o = m_marker->parentBox(); o != this; o = o->parentBox())
> +            offsetValue -= o->logicalTop();

This is confusing, on a first sight I thought it was doing the same than the first loop.
But one is using baselineProvider and another the m_marker->parentBox(), which I guess can be different.
Could we put these two loops closer in the code and explain what they're doing
we could even avoid that if baselineProvider == m_marker->parentBox(), but I don't know if that ever happens.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:37
> +    bool inside = is<RenderListMarker>(marker) ? downcast<RenderListMarker>(marker).isInside() : false;

Why we need a variable for this if it's only used in one place.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:53
> +        if (!inside) {
> +            if (current.hasOverflowClip())
> +                return &current;
> +            if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())))
> +                return &downcast<RenderBlock>(child);

Could you add a comment explaining this change?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:84
> +    element.setStyle(WTFMove(style));

If this is only needed to trigger needs layout, why not doing:
  element.setLogicalHeight(WTFMove(height));
  element.setNeedsLayout();

If this work I'd rename the method to something like:
  setLogicalHeightAndForceLayout()

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:137
> +                // E.g: <li><span><div>text<div><span></li>

This particular case seems to be already working "fine" in WebKit and different from Firefox and Chromium.
Check the following example:
  <ul>
    <li><span></span><div>text</div></li>
    <li><span>foo</span><div>text</div></li>
  </ul>
Comment 48 cathiechen 2019-01-01 06:08:30 PST
Comment on attachment 358114 [details]
Patch

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

>> Source/WebCore/ChangeLog:37
>> +
> 
> Nit: You need to regenrate ChangeLog as some changes from the patch are missing (for example the change in RenderBlock::isSelfCollapsingBlock()).

Done

>> Source/WebCore/rendering/RenderListItem.cpp:472
>> +        assert(baselineProvider);
> 
> Why assert and not ASSERT?

Done

>> Source/WebCore/rendering/RenderListItem.cpp:474
>> +        offset = box->firstLineBaseline();
> 
> So this is done because RenderBlock::firstLineBaseline() is protected,
> wouldn't make sense to make it public?
> Specially if RenderBox::firstLineBaseline() is already public.
> 
> Is this ending up calling RenderBlock::firstLineBaseline() or it only calls RenderBox::firstLineBaseline(),
> if it only goes to the RenderBox method we don't need to actually call it as it only returns "Optional<int>()".

Yes, this would call RenderBlock::firstLineBaseline().

Checked the patch that changed it to protect.
https://chromium.googlesource.com/chromium/src/+/a5fd6dfebaee8cd6ed7f0c08e82306b4b44323e1

Done.

>> Source/WebCore/rendering/RenderListItem.cpp:496
>> +            offsetValue -= o->logicalTop();
> 
> This is confusing, on a first sight I thought it was doing the same than the first loop.
> But one is using baselineProvider and another the m_marker->parentBox(), which I guess can be different.
> Could we put these two loops closer in the code and explain what they're doing
> we could even avoid that if baselineProvider == m_marker->parentBox(), but I don't know if that ever happens.

Yes, they are different. Normally, baselineProvider is the child of list item which contains content. m_marker->parentBox() is the zero-height marker container.
My intent is:
offsetValue = (baseline of baselineProvider + offset from baselineProvider to listItem) - (baseline of marker container - offset from marker container to listItem).
offsetValue is the distance marker need to move.

Added comment to explain them.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:37
>> +    bool inside = is<RenderListMarker>(marker) ? downcast<RenderListMarker>(marker).isInside() : false;
> 
> Why we need a variable for this if it's only used in one place.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:53
>> +                return &downcast<RenderBlock>(child);
> 
> Could you add a comment explaining this change?

Done.

=====================
// For outside marker, return if we meet !overflow:visible, root writing-mode,
// or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.).
// The result would trigger setNeedsBlockDirectionAlign(true). And it will be the baselineProvider.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:84
>> +    element.setStyle(WTFMove(style));
> 
> If this is only needed to trigger needs layout, why not doing:
>   element.setLogicalHeight(WTFMove(height));
>   element.setNeedsLayout();
> 
> If this work I'd rename the method to something like:
>   setLogicalHeightAndForceLayout()

Looks like element.setLogicalHeight has been removed. And RenderObject just provide const RenderStyle.

I think maybe this's reasonable in order to limit the change of style in setStyle.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:137
>> +                // E.g: <li><span><div>text<div><span></li>
> 
> This particular case seems to be already working "fine" in WebKit and different from Firefox and Chromium.
> Check the following example:
>   <ul>
>     <li><span></span><div>text</div></li>
>     <li><span>foo</span><div>text</div></li>
>   </ul>

Sorry this example is causing misunderstands. I've added more explanation to it.
===============


// If currentParent isn't the ancestor of newParent, marker might generate a new empty line.
// We need to remove marker here. So it could be inserted to the proper position, and no extra line generated.
// E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become:
// RenderListItem
//    RenderBlock (anonymous, marker container)
//      RenderListMarker
//      RenderInline (span)
//    RenderBlock (anonymous)
//      RenderBlock (div)
//        RenderText
//    RenderBlock (anonymous)
//      RenderInline (span)
// In this case, It would case an empty line because of marker. So we have to reattach marker
Comment 49 cathiechen 2019-01-01 06:15:13 PST
Hi Rego,

Thanks again for the review. :)

I've add some comments, hope it would help to make thing clear.

And the patch of moving test cases to wpt in chromium has been uploaded for code review.
Comment 50 cathiechen 2019-01-01 06:43:26 PST
Created attachment 358171 [details]
Patch
Comment 51 Frédéric Wang (:fredw) 2019-01-08 01:44:33 PST
*** Bug 13332 has been marked as a duplicate of this bug. ***
Comment 52 cathiechen 2019-01-08 22:35:42 PST
Created attachment 358673 [details]
Patch
Comment 53 cathiechen 2019-01-08 22:39:26 PST
Created attachment 358674 [details]
Patch
Comment 54 Simon Fraser (smfr) 2019-01-09 10:40:11 PST
This bug need a better title; I can't parse the current one.
Comment 55 cathiechen 2019-01-10 06:53:33 PST
Created attachment 358789 [details]
Patch
Comment 56 Build Bot 2019-01-10 07:58:48 PST
Comment on attachment 358789 [details]
Patch

Attachment 358789 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10697319

New failing tests:
imported/w3c/web-platform-tests/css/css-lists/list-and-writing-mode-001.html
Comment 57 Build Bot 2019-01-10 07:58:51 PST
Created attachment 358793 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 58 Build Bot 2019-01-10 08:09:52 PST
Comment on attachment 358789 [details]
Patch

Attachment 358789 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10697341

New failing tests:
imported/w3c/web-platform-tests/css/css-lists/list-and-writing-mode-001.html
Comment 59 Build Bot 2019-01-10 08:09:54 PST
Created attachment 358794 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 60 Build Bot 2019-01-10 08:49:32 PST
Comment on attachment 358789 [details]
Patch

Attachment 358789 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10697464

New failing tests:
imported/w3c/web-platform-tests/css/css-lists/list-and-writing-mode-001.html
Comment 61 Build Bot 2019-01-10 08:49:35 PST
Created attachment 358797 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 62 cathiechen 2019-01-10 23:27:42 PST
Created attachment 358881 [details]
Patch
Comment 63 cathiechen 2019-01-11 02:02:42 PST
(In reply to Simon Fraser (smfr) from comment #54)
> This bug need a better title; I can't parse the current one.

Done. Sorry for the typo.
Comment 64 Javier Fernandez 2019-01-11 05:19:59 PST
Comment on attachment 358881 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:519
> +        if (logicalHeightLength.isFixed() && isAnonymous() && parent() && is<RenderListItem>(parent())) {

is<> already checks for nullptr, so there is no need for the parent() clause.

> Source/WebCore/rendering/RenderBlock.cpp:521
> +            if (child && is<RenderListMarker>(child) && !child->nextSibling())

ditto

> Source/WebCore/rendering/RenderBlock.h:-354
> -    Optional<int> firstLineBaseline() const override;

Why we need to change visibility of this function ? As far as I understood, m_baselineProvides is a RenderBlock, so protected visibility should be enough.

> Source/WebCore/rendering/RenderListItem.cpp:461
> +    if (baselineProvider && is<RenderBlockFlow>(baselineProvider) && downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)

No need to check for nullptr here if we already use the is<> template.

> Source/WebCore/rendering/RenderListItem.cpp:465
> +    if (!backToOriginalBaseline)

Why not doing it this way instead ? 

 Optional<int> offset = backToOriginalBaseline ? nullopt : baselineProvider->firstLineBaseline();

Then in the if below we can just have one clause:

   if (!offset) {

> Source/WebCore/rendering/RenderListItem.cpp:474
> +    if (offset) {

Maybe we can early return here in case of !offset instead.
Comment 65 cathiechen 2019-01-13 19:15:16 PST
Comment on attachment 358881 [details]
Patch

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

Hi Javier,

Thanks a lot for the review:)

>> Source/WebCore/rendering/RenderBlock.cpp:519
>> +        if (logicalHeightLength.isFixed() && isAnonymous() && parent() && is<RenderListItem>(parent())) {
> 
> is<> already checks for nullptr, so there is no need for the parent() clause.

Done

>> Source/WebCore/rendering/RenderBlock.cpp:521
>> +            if (child && is<RenderListMarker>(child) && !child->nextSibling())
> 
> ditto

Done

>> Source/WebCore/rendering/RenderBlock.h:-354
>> -    Optional<int> firstLineBaseline() const override;
> 
> Why we need to change visibility of this function ? As far as I understood, m_baselineProvides is a RenderBlock, so protected visibility should be enough.

The compiler complains. I think the reason is that for m_baselineProvides RenderListItem isn't the inside of own class.

>> Source/WebCore/rendering/RenderListItem.cpp:461
>> +    if (baselineProvider && is<RenderBlockFlow>(baselineProvider) && downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)
> 
> No need to check for nullptr here if we already use the is<> template.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:465
>> +    if (!backToOriginalBaseline)
> 
> Why not doing it this way instead ? 
> 
>  Optional<int> offset = backToOriginalBaseline ? nullopt : baselineProvider->firstLineBaseline();
> 
> Then in the if below we can just have one clause:
> 
>    if (!offset) {

Done. Thanks!

>> Source/WebCore/rendering/RenderListItem.cpp:474
>> +    if (offset) {
> 
> Maybe we can early return here in case of !offset instead.

Done
Comment 66 cathiechen 2019-01-13 19:19:16 PST
Created attachment 359016 [details]
Patch
Comment 67 Manuel Rego Casasnovas 2019-01-15 03:18:20 PST
Comment on attachment 359016 [details]
Patch

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

r=me. Thanks your all your work on this, I've several inline comments and nits please fix them before landing.

> Source/WebCore/ChangeLog:11
> +        sibilng of a block child. To fix these two kinds of issue:

Typo: s/two kinds of issue/two kind of issues/

> Source/WebCore/ChangeLog:15
> +        has been layout (marker have to adjust itself to the content of li)

Typo: s/has been layout/have been layout/
Typo: s/marker have/marker has/
Nit: Missing dot at the end of sentence.

> Source/WebCore/ChangeLog:17
> +        Rebaseline test:

I'd also mention the tests that are now passing from WPT (not as rebaseline tests but somewhere in the changelog.

> Source/WebCore/ChangeLog:27
> +        marker_container only have marker as its child, make it a self collapsing

Typo: s/only have/only has/

> Source/WebCore/ChangeLog:40
> +        whose m_needBlockDirectionAlign is true, insert marker to a zero height

Typo: s/insert marker to/insert marker into/

> Source/WebCore/rendering/RenderListItem.cpp:452
> +    // So if there's no line box in baselineProvider make sure it back to its original position.

Typo: s/make sure it back/make sure it is back/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:49
> +        // For outside marker, return if we meet !overflow:visible, root writing-mode,
> +        // or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.).

This kind of comments are not very useful as they're like repeating the if condition in a comment.
We'd need to either describe this with words or remove it.
Also it doesn't say what we return... and it can be current or child.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:118
> +    // Create a zero height parent for marker, so that marker could be displayed or no line break between marker and content.

I'd reword this a little bit doing something like:
* "marker could be displayed" => "marker would be visible with overflow: hidden"
* "and there won't be a line break between marker and block content"
Feel free to change it and write it properly.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:121
> +    // If marker is the only child of markerContainer, set LogicalHeight of markerContainer to 0px; else restore it to LogicalHeight of <li>.

I'd move this line 121 to around line 130, as there is where this is done.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:122
> +    if (!markerRenderer->isInside() && newParent && (newParent->hasOverflowClip() || !is<RenderBlockFlow>(newParent) || newParent->isWritingModeRoot()))

Why we check "newParent->isWritingModeRoot()" here?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:131
> +            // So add isInside() here.

Remove this line, the previous line is explaining what we're doing so we don't need this one (this is already in the if condition).

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139
> +                // E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become:

Please close the SPAN and DIV tags to make it clearer the example.
And I'd remove the second SPAN as it's not needed.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:148
> +                //    RenderBlock (anonymous)
> +                //      RenderInline (span)

So we could remove these two lines if we remove the second SPAN.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:149
> +                // In this case, It would case an empty line because of marker. So we have to reattach marker

Typo: s/It would case/it would cause/
Maybe we need to reword this a little bit. It'd cause an empty line or a line break? I understand that the reason is because marker is in a different RenderBlock.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:169
> +                if (originalMarker.get())

Isn't enough with using "if (originalMarker)" ?

> LayoutTests/TestExpectations:2932
> +# LI need to get notified when its subtree is changed, then it could adjust the position of list marker.

Instead of adding a comment, I'd report this a separated bug and link it from here like:
webkit.org/b/XXXXXXX imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ]
Comment 68 Simon Fraser (smfr) 2019-01-15 10:38:18 PST
Comment on attachment 359016 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        1. Create a zero-height anonymous parent for marker (to eleminate the

eleminate -> eliminate

> Source/WebCore/ChangeLog:14
> +        2. Reposition the position of marker after all list item's children

"Reposition the position of marker after" -> "Reposition the marker to be after"

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:80
> +static void forceLogicalHeight(RenderElement& element, Length&& height)

Don't call the argument 'element'. That makes it sound like an HTMLElement.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:125
> +    // Prepare for block direction align.

This comment doesn't add anything.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:127
> +        // Deal with the situation of render tree changed.

This comment doesn't add anything.

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:187
> +                    m_builder.attach(*markerContainer, WTFMove(originalMarker), firstNonMarkerChild(*markerContainer));
> +                else
> +                    m_builder.attach(*markerContainer, WTFMove(newMarkerRenderer), firstNonMarkerChild(*markerContainer));
> +                m_builder.attach(listItemRenderer, WTFMove(markerContainer), beforeChild);
> +            } else {
> +                if (markerRenderer->isInside()) {
> +                    listItemRenderer.setNeedsBlockDirectionAlign(false);
> +                    if (!newParent)
> +                        newParent = &listItemRenderer;
> +                    if (originalMarker.get())
> +                        m_builder.attach(*newParent, WTFMove(originalMarker), firstNonMarkerChild(*newParent));
> +                    else
> +                        m_builder.attach(*newParent, WTFMove(newMarkerRenderer), firstNonMarkerChild(*newParent));
> +
> +                } else if (originalMarker.get())
> +                    m_builder.attach(listItemRenderer, WTFMove(originalMarker), beforeChild);
> +                else
> +                    m_builder.attach(listItemRenderer, WTFMove(newMarkerRenderer), beforeChild);

All these repeated "m_builder.attach(listItemRenderer, <something>, beforeChild);" makes me think this code could be refactored to be much cleaner. Generally, if you have code that has multiple calls to a function with different parameters, you should factor it to have a single call.

You could have a function or lambda that returns the appropriate marker (original or new), and just have one call to m_builder.attach().
Comment 69 cathiechen 2019-01-19 06:25:55 PST
Comment on attachment 359016 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        sibilng of a block child. To fix these two kinds of issue:
> 
> Typo: s/two kinds of issue/two kind of issues/

Done

>> Source/WebCore/ChangeLog:15
>> +        has been layout (marker have to adjust itself to the content of li)
> 
> Typo: s/has been layout/have been layout/
> Typo: s/marker have/marker has/
> Nit: Missing dot at the end of sentence.

Done

>> Source/WebCore/ChangeLog:17
>> +        Rebaseline test:
> 
> I'd also mention the tests that are now passing from WPT (not as rebaseline tests but somewhere in the changelog.

Done

>> Source/WebCore/ChangeLog:27
>> +        marker_container only have marker as its child, make it a self collapsing
> 
> Typo: s/only have/only has/

Done

>> Source/WebCore/ChangeLog:40
>> +        whose m_needBlockDirectionAlign is true, insert marker to a zero height
> 
> Typo: s/insert marker to/insert marker into/

Done

>> Source/WebCore/rendering/RenderListItem.cpp:452
>> +    // So if there's no line box in baselineProvider make sure it back to its original position.
> 
> Typo: s/make sure it back/make sure it is back/

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:49
>> +        // or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.).
> 
> This kind of comments are not very useful as they're like repeating the if condition in a comment.
> We'd need to either describe this with words or remove it.
> Also it doesn't say what we return... and it can be current or child.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:118
>> +    // Create a zero height parent for marker, so that marker could be displayed or no line break between marker and content.
> 
> I'd reword this a little bit doing something like:
> * "marker could be displayed" => "marker would be visible with overflow: hidden"
> * "and there won't be a line break between marker and block content"
> Feel free to change it and write it properly.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:121
>> +    // If marker is the only child of markerContainer, set LogicalHeight of markerContainer to 0px; else restore it to LogicalHeight of <li>.
> 
> I'd move this line 121 to around line 130, as there is where this is done.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:122
>> +    if (!markerRenderer->isInside() && newParent && (newParent->hasOverflowClip() || !is<RenderBlockFlow>(newParent) || newParent->isWritingModeRoot()))
> 
> Why we check "newParent->isWritingModeRoot()" here?

We'd make li + WritingModeRoot using the "zero-height approach", otherwise there is a line-break.
This's according to getParentOfFirstLineBox().
Sorry, this's hard to read. Changed it a bit. It might be clear now.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:131
>> +            // So add isInside() here.
> 
> Remove this line, the previous line is explaining what we're doing so we don't need this one (this is already in the if condition).

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139
>> +                // E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become:
> 
> Please close the SPAN and DIV tags to make it clearer the example.
> And I'd remove the second SPAN as it's not needed.

Done and reword the example.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:148
>> +                //      RenderInline (span)
> 
> So we could remove these two lines if we remove the second SPAN.

Sorry... See the new explain of this example.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:149
>> +                // In this case, It would case an empty line because of marker. So we have to reattach marker
> 
> Typo: s/It would case/it would cause/
> Maybe we need to reword this a little bit. It'd cause an empty line or a line break? I understand that the reason is because marker is in a different RenderBlock.

Done

Yeah, it's line break.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:169
>> +                if (originalMarker.get())
> 
> Isn't enough with using "if (originalMarker)" ?

Done

>> LayoutTests/TestExpectations:2932
>> +# LI need to get notified when its subtree is changed, then it could adjust the position of list marker.
> 
> Instead of adding a comment, I'd report this a separated bug and link it from here like:
> webkit.org/b/XXXXXXX imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ]

Done
Comment 70 zalan 2019-01-19 06:28:24 PST
(In reply to cathiechen from comment #69)
> 
> >> LayoutTests/TestExpectations:2932
> >> +# LI need to get notified when its subtree is changed, then it could adjust the position of list marker.
> > 
> > Instead of adding a comment, I'd report this a separated bug and link it from here like:
> > webkit.org/b/XXXXXXX imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ]
> 
> Done
Thanks for fixing them all. Do you mind if I take a final look before you cq+ your updated patch? Thanks.
Comment 71 cathiechen 2019-01-19 06:35:19 PST
Comment on attachment 359016 [details]
Patch

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

Hi Simon,
Thanks for the review :)

>> Source/WebCore/ChangeLog:12
>> +        1. Create a zero-height anonymous parent for marker (to eleminate the
> 
> eleminate -> eliminate

Done

>> Source/WebCore/ChangeLog:14
>> +        2. Reposition the position of marker after all list item's children
> 
> "Reposition the position of marker after" -> "Reposition the marker to be after"

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:80
>> +static void forceLogicalHeight(RenderElement& element, Length&& height)
> 
> Don't call the argument 'element'. That makes it sound like an HTMLElement.

Done. Changed it to renderer.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:125
>> +    // Prepare for block direction align.
> 
> This comment doesn't add anything.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:127
>> +        // Deal with the situation of render tree changed.
> 
> This comment doesn't add anything.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:187
>> +                    m_builder.attach(listItemRenderer, WTFMove(newMarkerRenderer), beforeChild);
> 
> All these repeated "m_builder.attach(listItemRenderer, <something>, beforeChild);" makes me think this code could be refactored to be much cleaner. Generally, if you have code that has multiple calls to a function with different parameters, you should factor it to have a single call.
> 
> You could have a function or lambda that returns the appropriate marker (original or new), and just have one call to m_builder.attach().

Done. Thanks:)
Comment 72 cathiechen 2019-01-19 06:40:19 PST
Created attachment 359606 [details]
Patch
Comment 73 cathiechen 2019-01-19 06:42:36 PST
(In reply to zalan from comment #70)

> Thanks for fixing them all. Do you mind if I take a final look before you
> cq+ your updated patch? Thanks.

That is great! The new patch has been uploaded. Thanks:)
Comment 74 Manuel Rego Casasnovas 2019-02-07 14:56:24 PST
(In reply to zalan from comment #70)
> Thanks for fixing them all. Do you mind if I take a final look before you
> cq+ your updated patch? Thanks.

Pinging Zalan... :-)
Comment 75 zalan 2019-02-14 10:27:20 PST
(In reply to Manuel Rego Casasnovas from comment #74)
> (In reply to zalan from comment #70)
> > Thanks for fixing them all. Do you mind if I take a final look before you
> > cq+ your updated patch? Thanks.
> 
> Pinging Zalan... :-)
:( will review this over the weekend.
Comment 76 zalan 2019-02-17 21:31:55 PST
Any reason why you didn't make this generic rule of _always_ wrapping the marker inside a dedicated anonymous block box? It would surely simplify some of the "new parent" logic here.
Comment 77 cathiechen 2019-02-17 22:03:11 PST
(In reply to zalan from comment #76)
> Any reason why you didn't make this generic rule of _always_ wrapping the
> marker inside a dedicated anonymous block box? It would surely simplify some
> of the "new parent" logic here.

Hi zalan,

Thanks for the reply:)

The original thought of this patch is to fix the issues. And it would increase the cost for adding an additional anonymous block compared to the original algorithm.

I do agree the logic would get simpler if we make it a generic rule.

Maybe we could do it in the next patch?
Comment 78 cathiechen 2019-04-02 04:33:23 PDT
Hi zalan@,

I filed a bug to make zero-height a common rule. What do you think? :)
Comment 79 cathiechen 2019-04-02 04:34:01 PDT
(In reply to cathiechen from comment #78)
> Hi zalan@,
> 
> I filed a bug to make zero-height a common rule. What do you think? :)


https://bugs.webkit.org/show_bug.cgi?id=196489
Comment 80 cathiechen 2019-04-02 04:47:15 PDT
Created attachment 366487 [details]
Patch
Comment 81 cathiechen 2019-04-02 04:49:18 PDT
Comment on attachment 366487 [details]
Patch

Rebase
Comment 82 cathiechen 2019-04-02 04:49:31 PDT
Rebase