[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

Manuel Rego Casasnovas <rego at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rego at igalia.com
 Attachment #357940|                            |review-
              Flags|                            |

--- Comment #18 from Manuel Rego Casasnovas <rego 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

Thanks for the patch, I did an initial review with lots of comments.
I didn't have time to look into the tests yet, but it'd be nice to use WPT instead to share them with Chromium and Firefox.

This was fixed in Chromium first, could you link the Chromium bug in "See also" field? Thanks.

> 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"

> 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.

> Source/WebCore/ChangeLog:12
> +        1. Create a zero-height anonymouse parent for marker(to eleminate the

Typo: s/anonymouse/anonymous/
Nit: Missing space before "("

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

Typo: s/has/have/

> 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.

> Source/WebCore/ChangeLog:36
> +        (WebCore::RenderTreeBuilder::List::updateItemMarker):

And the same in this case.

> 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/ ?

> 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?

> 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?

> 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.

> 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.

> 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();

> Source/WebCore/rendering/RenderListItem.cpp:477
> +        baselineProvider = m_marker->containingBlock();

Again I guess we're sure containingBlock() is never null here.

> Source/WebCore/rendering/RenderListItem.cpp:482
> +    if (offset != -1) {

I'd change this for something like:
  if (offset == -1)
      return;

> Source/WebCore/rendering/RenderListItem.cpp:489
> +        // BaselinePosition is workable when marker is an image.

What do you mean here by "is workable"?

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

Nit: s/Ascent/ascent/

> 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?

> Source/WebCore/rendering/RenderListItem.cpp:496
> +            const FontMetrics& markerFontMetrics = m_marker->style().fontMetrics();
> +            offset -= markerFontMetrics.ascent(markerRoot.baselineType());

Nit: I'd write this as:
  offset -= m_marker->style().fontMetrics().ascent(markerRoot.baselineType());

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

Nit: s/setNeedBlockDirectionAlign/setNeedsBlockDirectionAlign/ and s/m_needBlockDirectionAlign/m_needsBlockDirectionAlign/

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

Nit: s/needBlockDirectionAlign/needsBlockDirectionAlign/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:52
> -        if (!is<RenderBlock>(child) || is<RenderTable>(child) || is<RenderRubyAsBlock>(child))
> -            break;
> +        if (current.hasOverflowClip())
> +            return ¤t;
> +
> +        if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())))
> +            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?

> 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"

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:120
> +    // Prepare for block direction align

Nit: Comments should finish in a "dot".

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

Nit: s/IsInside/isInisde/

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:130
> +                    forceLogicalHeight(*currentParent, Length(style.logicalHeight()));

Isn't enough just calling setLogicalHeigth()?

> 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?

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:139
> +                forceLogicalHeight(*currentParent, Length(0, Fixed));

Same question than before regarding setLogicalHeight().

> Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp:152
> +                    forceLogicalHeight(*markerContainer, Length(0, Fixed));

Ditto.

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

s/acting/behavior/

> 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).

-- 
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/20181223/22be1064/attachment-0001.html>