[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
--- Comment #69 from cathiechen <cathiechen at igalia.com> ---
Comment on attachment 359016
View in context: https://bugs.webkit.org/attachment.cgi?id=359016&action=review
>> + 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/
>> + // 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?
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.
>> + // 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.
Done and reword the example.
>> + // RenderInline (span)
> So we could remove these two lines if we remove the second SPAN.
Sorry... See the new explain of this example.
>> + // 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.
Yeah, it's line break.
>> + 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