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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 19 06:25:55 PST 2019


https://bugs.webkit.org/show_bug.cgi?id=192980

--- Comment #69 from cathiechen <cathiechen at igalia.com> ---
Comment on attachment 359016
  --> https://bugs.webkit.org/attachment.cgi?id=359016
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190119/712855c9/attachment.html>


More information about the webkit-unassigned mailing list