[webkit-reviews] review granted: [Bug 192980] [css-lists] Fix list marker issues related to line breaks and markers disappearance : [Attachment 359016] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 15 03:18:20 PST 2019
Manuel Rego Casasnovas <rego at igalia.com> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 192980: [css-lists] Fix list marker issues related to line breaks and
markers disappearance
https://bugs.webkit.org/show_bug.cgi?id=192980
Attachment 359016: Patch
https://bugs.webkit.org/attachment.cgi?id=359016&action=review
--- 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-0
01.html [ ImageOnlyFailure ]
More information about the webkit-reviews
mailing list