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

[bugzilla-daemon at webkit.org]

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

Manuel Rego Casasnovas <rego at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #359016|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #67 from Manuel Rego Casasnovas <rego 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

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 ]

-- 
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/20190115/3acc9747/attachment-0001.html>