[Webkit-unassigned] [Bug 192980] Eliminate line-breaks and disappear of list marker

[bugzilla-daemon at webkit.org]

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

--- Comment #48 from cathiechen <cathiechen at igalia.com> ---
Comment on attachment 358114
  --> https://bugs.webkit.org/attachment.cgi?id=358114
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358114&action=review

>> Source/WebCore/ChangeLog:37
>> +
> 
> Nit: You need to regenrate ChangeLog as some changes from the patch are missing (for example the change in RenderBlock::isSelfCollapsingBlock()).

Done

>> Source/WebCore/rendering/RenderListItem.cpp:472
>> +        assert(baselineProvider);
> 
> Why assert and not ASSERT?

Done

>> Source/WebCore/rendering/RenderListItem.cpp:474
>> +        offset = box->firstLineBaseline();
> 
> So this is done because RenderBlock::firstLineBaseline() is protected,
> wouldn't make sense to make it public?
> Specially if RenderBox::firstLineBaseline() is already public.
> 
> Is this ending up calling RenderBlock::firstLineBaseline() or it only calls RenderBox::firstLineBaseline(),
> if it only goes to the RenderBox method we don't need to actually call it as it only returns "Optional<int>()".

Yes, this would call RenderBlock::firstLineBaseline().

Checked the patch that changed it to protect.
https://chromium.googlesource.com/chromium/src/+/a5fd6dfebaee8cd6ed7f0c08e82306b4b44323e1

Done.

>> Source/WebCore/rendering/RenderListItem.cpp:496
>> +            offsetValue -= o->logicalTop();
> 
> This is confusing, on a first sight I thought it was doing the same than the first loop.
> But one is using baselineProvider and another the m_marker->parentBox(), which I guess can be different.
> Could we put these two loops closer in the code and explain what they're doing
> we could even avoid that if baselineProvider == m_marker->parentBox(), but I don't know if that ever happens.

Yes, they are different. Normally, baselineProvider is the child of list item which contains content. m_marker->parentBox() is the zero-height marker container.
My intent is:
offsetValue = (baseline of baselineProvider + offset from baselineProvider to listItem) - (baseline of marker container - offset from marker container to listItem).
offsetValue is the distance marker need to move.

Added comment to explain them.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:37
>> +    bool inside = is<RenderListMarker>(marker) ? downcast<RenderListMarker>(marker).isInside() : false;
> 
> Why we need a variable for this if it's only used in one place.

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:53
>> +                return &downcast<RenderBlock>(child);
> 
> Could you add a comment explaining this change?

Done.

=====================
// For outside marker, return if we meet !overflow:visible, root writing-mode,
// or is<RenderBlock>(child) && !is<RenderBlockFlow>(child)(flex, grid, table, etc.).
// The result would trigger setNeedsBlockDirectionAlign(true). And it will be the baselineProvider.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:84
>> +    element.setStyle(WTFMove(style));
> 
> If this is only needed to trigger needs layout, why not doing:
>   element.setLogicalHeight(WTFMove(height));
>   element.setNeedsLayout();
> 
> If this work I'd rename the method to something like:
>   setLogicalHeightAndForceLayout()

Looks like element.setLogicalHeight has been removed. And RenderObject just provide const RenderStyle.

I think maybe this's reasonable in order to limit the change of style in setStyle.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:137
>> +                // E.g: <li><span><div>text<div><span></li>
> 
> This particular case seems to be already working "fine" in WebKit and different from Firefox and Chromium.
> Check the following example:
>   <ul>
>     <li><span></span><div>text</div></li>
>     <li><span>foo</span><div>text</div></li>
>   </ul>

Sorry this example is causing misunderstands. I've added more explanation to it.
===============


// If currentParent isn't the ancestor of newParent, marker might generate a new empty line.
// We need to remove marker here. So it could be inserted to the proper position, and no extra line generated.
// E.g: <li><span><div>text<div><span></li> After the change of render tree, it might become:
// RenderListItem
//    RenderBlock (anonymous, marker container)
//      RenderListMarker
//      RenderInline (span)
//    RenderBlock (anonymous)
//      RenderBlock (div)
//        RenderText
//    RenderBlock (anonymous)
//      RenderInline (span)
// In this case, It would case an empty line because of marker. So we have to reattach marker

-- 
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/20190101/05e1dd79/attachment.html>