Bug 192980 – [css-lists] Fix list marker issues related to line breaks and markers disappearance

• Status: NEW
• Component: Layout and Rendering

[cathiechen]

Eliminate line-breaks and disappear of list marker

[cathiechen]

Created attachment 357937 [details]
Patch

[Rob Buis]

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?

[cathiechen]

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.

[cathiechen]

Created attachment 357938 [details]
Patch

[EWS Watchlist]

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.

[cathiechen]

Created attachment 357939 [details]
Patch

[cathiechen]

Created attachment 357940 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Manuel Rego Casasnovas]

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

[cathiechen]

Created attachment 358035 [details]
Patch

[cathiechen]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[cathiechen]

Created attachment 358063 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[cathiechen]

Created attachment 358105 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[cathiechen]

Created attachment 358109 [details]
Patch

[cathiechen]

Created attachment 358114 [details]
Patch

[Manuel Rego Casasnovas]

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>

[cathiechen]

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

[cathiechen]

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.

[cathiechen]

Created attachment 358171 [details]
Patch

[Frédéric Wang (:fredw)]

*** Bug 13332 has been marked as a duplicate of this bug. ***

[cathiechen]

Created attachment 358673 [details]
Patch

[cathiechen]

Created attachment 358674 [details]
Patch

[Simon Fraser (smfr)]

This bug need a better title; I can't parse the current one.

[cathiechen]

Created attachment 358789 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[cathiechen]

Created attachment 358881 [details]
Patch

[cathiechen]

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

[Javier Fernandez]

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.

[cathiechen]

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

[cathiechen]

Created attachment 359016 [details]
Patch

[Manuel Rego Casasnovas]

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 ]

[Simon Fraser (smfr)]

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

[cathiechen]

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

[zalan]

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

[cathiechen]

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

[cathiechen]

Created attachment 359606 [details]
Patch

[cathiechen]

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

[Manuel Rego Casasnovas]

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

[zalan]

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

[zalan]

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.

[cathiechen]

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

[cathiechen]

Hi zalan@,

I filed a bug to make zero-height a common rule. What do you think? :)

[cathiechen]

(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

[cathiechen]

Created attachment 366487 [details]
Patch

[cathiechen]

Comment on attachment 366487 [details]
Patch

Rebase

[cathiechen]

Rebase

[Ahmad Saleem]

@Alan - Is this something needed based on IFC work or it is already fixed? If you want, I can try to rebase and see if it is easier to do and in case of any error, can ask for help? Thanks!

[zalan]

(In reply to Ahmad Saleem from comment #83)
> @Alan - Is this something needed based on IFC work or it is already fixed?
> If you want, I can try to rebase and see if it is easier to do and in case
> of any error, can ask for help? Thanks!
Not yet fixed, but will certainly be addressed by LFC (the umbrella project).

[Greg Whitworth]

While not exactly the same as some of the examples above I think this usecase may be triggering this issue although I haven't looked at the webkit code. This was hit by one of our Salesforce devs as they were building out a component. Here is the codepen which you'll see that Safari renders differently from Chrome and Firefox: https://codepen.io/gowrs/pen/rNgwoNJ

[zalan]

(In reply to Greg Whitworth from comment #85)
> While not exactly the same as some of the examples above I think this
> usecase may be triggering this issue although I haven't looked at the webkit
> code. This was hit by one of our Salesforce devs as they were building out a
> component. Here is the codepen which you'll see that Safari renders
> differently from Chrome and Firefox: https://codepen.io/gowrs/pen/rNgwoNJ

RenderView at (0,0) size 1006x566 renderer (0x1130047a0) layout box (0x0) layout id->[52]
  HTML RenderBlock at (0,0) size 1006x566 renderer (0x113005620) layout box (0x0) node (0x113053f70) layout id->[52]
    BODY RenderBody at (8,8) size 990x542 renderer (0x113005840) layout box (0x0) node (0x11305c670) layout id->[52]
      UL RenderBlock at (0,0) size 990x18 renderer (0x113007020) layout box (0x0) node (0x113240e60) layout id->[46]
        LI RenderListItem at (40,0) size 950x18 renderer (0x113241020) layout box (0x0) node (0x113240f40) layout id->[46]
          P RenderBlock at (0,0) size 950x18 renderer (0x113007120) layout box (0x113007760) node (0x1130e77d0) layout id->[46]
            RenderListMarker (::marker) at (-17,0) size 7x18 renderer (0x113241130) layout box (0x113007920) layout id->[46]
            #text RenderText renderer (0x113007340) layout box (0x113007800) node (0x1113999e0) length->(6) "list 1" layout id->[n/a]

yeah, this is certainly the same bug where WebKit puts the list marker _inside_ the block container initiated by the <p> element and positions it at -17px to the left. It simply means the list marker overflows its block container and when 'overflow: hidden' is applied we simply clip it. :(

I was going to open a bug but I believe the issues described in this ticket may be the the cause of the problem I encountered. If this is not the same please let me know and I will open a new bug for it.

It seems if we use non-inline displays for text input fields in a list item then the marker is placed above the top of the input field. If we include placeholder text in the input field the marker disappears entirely.

JSFiddle (compare Safari to Chrome or Firefox): https://jsfiddle.net/x8zeo2u7/

[Greg Whitworth]

@zalan

> yeah, this is certainly the same bug where WebKit puts the list marker _inside_ the block container initiated by the <p> element and positions it at -17px to the left. 

Thanks - is there any plan to address this? I'm curious why the prior patch wasn't landed and if that has some insights into why this hasn't been addressed. Thank you.