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

Mon Dec 24 03:41:51 PST 2018

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

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

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

Hi Rego, Thanks very much for the thorough review:)
And sorry for the typos.

>> Source/WebCore/ChangeLog:9
>> +        is marker has been added as the child of this overflow block. And the
> 
> Nit: "is that marker has been added"

Done

>> Source/WebCore/ChangeLog:11
>> +        sibilng of a block child. To fix these two kinds of issue:
> 
> Is there a way to divide these two fixes in two separated patches, or the code is the same for both things?
> I have the feeling that we're fixing two things in this patch one releated to line breaks and another related to overflow: hidden.

Nope, it couldn't be separated.
Logically, this design would fit for all conditions. In this patch, we let li + overflow, flex, etc. use it.

>> Source/WebCore/ChangeLog:12
>> +        1. Create a zero-height anonymouse parent for marker(to eleminate the
> 
> Typo: s/anonymouse/anonymous/
> Nit: Missing space before "("

Done

>> Source/WebCore/ChangeLog:15
>> +        has been layout (marker has to adjust itselt to the content of li)
> 
> Typo: s/has/have/

Done

>> Source/WebCore/ChangeLog:30
>> +        (WebCore::RenderListItem::alignMarkerInBlockDirection):
> 
> As this method is quite complex, it'd be nice to have a summary of what it does here.

Done

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::RenderTreeBuilder::List::updateItemMarker):
> 
> And the same in this case.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:445
>> +// Align marker_inline_box in block direction according to line_box_root's baseline.
> 
> Nit: s/marker_inline_box/markerInlineBox/
> Also I guess s/line_box_root/RootInlineBox/ ?

Done

>> Source/WebCore/rendering/RenderListItem.cpp:460
>> +    }
> 
> Nit: This variable could be initialized like:
>   backToOriginalBaseline = !baselineProvider || baselineProvider->isWritingModeRoot();
> 
> If this is true couldn't we just return and avoid doing anything in this method?
> Aren't we keeping the old behavior? Or are we doing something else?

Done.

In this case, because marker might be moved to other position at last layout pass.
So we have to make sure it back to its original baseline, couldn't just return.

>> Source/WebCore/rendering/RenderListItem.cpp:463
>> +    RootInlineBox& markerRoot = markerInlineBox->root();
> 
> Is it guaranteed that markerInlineBox is never null? Should we add an ASSERT at least?

Done

>> Source/WebCore/rendering/RenderListItem.cpp:465
>> +        // If marker_ and baselineProvider share a same RootInlineBox, no need to align marker.
> 
> Nit: "marker_" I guess that name might come from Blink and it should be "m_marker" here.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:466
>> +        if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)
> 
> We don't need two ifs, we can combine both in just one if.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:473
>> +        offset = box->firstLineBaseline().value();
> 
> Wouldn't we need "valueOr()" to be safe here?
> 
> Nit: Couldn't we write this like:
>   offest = downcast<RenderBox>(baslineProvider)->firstLineBaseline().value();

Yes, using value() directly would cause a crash issue. I use Optional in the new patch.

RenderBox::firstLineBaseline() is public.
RenderBlock::firstLineBaseline() is protected.
In order to call it, I have to cast baslineProvider from RenderBlock to RenderBox.
RenderBox is the parent class, so it isn't downcast...

>> Source/WebCore/rendering/RenderListItem.cpp:477
>> +        baselineProvider = m_marker->containingBlock();
> 
> Again I guess we're sure containingBlock() is never null here.

Done

>> Source/WebCore/rendering/RenderListItem.cpp:482
>> +    if (offset != -1) {
> 
> I'd change this for something like:
>   if (offset == -1)
>       return;

Use this format in the new patch.

    if (offset)
        LayoutUnit offsetValue = offset.value();

>> Source/WebCore/rendering/RenderListItem.cpp:489
>> +        // BaselinePosition is workable when marker is an image.
> 
> What do you mean here by "is workable"?

I mean RenderListMarker::baselinePosition() return the right value when marker is image.

>> Source/WebCore/rendering/RenderListItem.cpp:491
>> +        // So use markerFontMetrics.Ascent when marker is text.
> 
> Nit: s/Ascent/ascent/

Done

>> Source/WebCore/rendering/RenderListItem.cpp:492
>> +        if (m_marker->isImage())
> 
> I see that RenderListMarker has a method baselinePosition() why we cannot use that?
> I guess it provides wrong information, then why we don't modify it?

Yes, it return baselinePosition of list item when marker isn't image. This isn't right.

Corrected baselinePosition() and lineheight() in the new patch

>> Source/WebCore/rendering/RenderListItem.cpp:496
>> +            offset -= markerFontMetrics.ascent(markerRoot.baselineType());
> 
> Nit: I'd write this as:
>   offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType());

Done

>> Source/WebCore/rendering/RenderListItem.h:64
>> +    void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign = need; }
> 
> Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/

Done

>> Source/WebCore/rendering/RenderListItem.h:65
>> +    bool needBlockDirectionAlign() const { return m_needBlockDirectionAlign; }
> 
> Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52
>> +            return &downcast<RenderBlock>(child);
> 
> I don't understand this change very well.
> For the case of isWritingModeRoot() it was breaking before (so it returned nullptr) and now it's returning the child...
> Is this the expected bahvavior? Doesn't it affect other cases?

We use getParentOfFirstLineBox to determine if this list marker should use the new design.
And use getParentOfFirstLineBox as the baselineprovider in alignMarkerInBlockDirection().

And yes, for list-style-position:inside cases, this might cause crash issues.
The new patch would deal with it.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:114
>> +    // 1. Place marker as a child of <li>. Make sure don't share parent with empty inline elements which don't generate InlineBox.
> 
> Typo: "Make sure it doesn't share parent"

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120
>> +    // Prepare for block direction align
> 
> Nit: Comments should finish in a "dot".

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:126
>> +            // So add IsInside() here.
> 
> Nit: s/IsInside/isInisde/

Done

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130
>> +                    forceLogicalHeight(*currentParent, Length(style.logicalHeight()));
> 
> Isn't enough just calling setLogicalHeigth()?

Not sure if I understand the question correctly.
We need use setStyle to make sure it would trigger needsLayout().
And it calls three times, so create a function for them.

>> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:133
>> +                // We need to remove marker here.E.g: <li><span><div>text<div><span></li>
> 
> Mmmm, what's the expected behavior for this example?

In this case, marker might occupy a anonymous block.
<li>
  <anonymous><span>m_marker</anonymous>
  <div>text<div>
  <anonymous><span></anonymous>
</li>

We need detach m_marker from render tree, and insert it to newParent. So the render tree would like this:
<li>
  <anonymous><span></anonymous>
  <div>m_marker text<div>
  <anonymous><span></anonymous>
</li>

Explained more about it in the new patch.

>> LayoutTests/ChangeLog:10
>> +        marker in li + block overflow:hiddlen child, and testing if the acting
> 
> s/acting/behavior/

Done

>> LayoutTests/fast/lists/add-inline-child-after-marker-expected.html:1
>> +<head></head><body><ul>
> 
> Tests need to start with <!DOCTYPE html> unless you're explicitly testing quirks mode (which I don't think is the case).

Done

-- 
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/20181224/0f3402b1/attachment.html>