[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
Tue Jan 15 03:18:20 PST 2019
Manuel Rego Casasnovas <rego at igalia.com> changed:
What |Removed |Added
Attachment #359016|review? |review+, commit-queue-
--- Comment #67 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 359016
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.
> + sibilng of a block child. To fix these two kinds of issue:
Typo: s/two kinds of issue/two kind of issues/
> + 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.
> + Rebaseline test:
I'd also mention the tests that are now passing from WPT (not as rebaseline tests but somewhere in the changelog.
> + marker_container only have marker as its child, make it a self collapsing
Typo: s/only have/only has/
> + whose m_needBlockDirectionAlign is true, insert marker to a zero height
Typo: s/insert marker to/insert marker into/
> + // 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/
> + // 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.
> + // 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.
> + // 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.
> + if (!markerRenderer->isInside() && newParent && (newParent->hasOverflowClip() || !is<RenderBlockFlow>(newParent) || newParent->isWritingModeRoot()))
Why we check "newParent->isWritingModeRoot()" here?
> + // 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).
> + // 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.
> + // RenderBlock (anonymous)
> + // RenderInline (span)
So we could remove these two lines if we remove the second SPAN.
> + // 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.
> + if (originalMarker.get())
Isn't enough with using "if (originalMarker)" ?
> +# 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...
More information about the webkit-unassigned