[webkit-reviews] review denied: [Bug 88705] Text with text-overflow:ellipsis and text-align:right is left aligned : [Attachment 146695] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Benjamin Poulain <benjamin at webkit.org>'s request for
review:
Bug 88705: Text with text-overflow:ellipsis and text-align:right is left
aligned
https://bugs.webkit.org/show_bug.cgi?id=88705

Attachment 146695: Patch
https://bugs.webkit.org/attachment.cgi?id=146695&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=146695&action=review


> Source/WebCore/ChangeLog:9
> +	   an ellipsis and where it should be. Because of that, text is
layouted as if does

Typo: “layouted”

> Source/WebCore/ChangeLog:13
> +	   did not follow the allignment.

What about centered and justified text? (I’m not saying the change log should
answer this, I’m just curious).

> Source/WebCore/ChangeLog:15
> +	   This patch change the position of lines with ellipsis after layout
to follow the allignment.

Typo: “change”.

> Source/WebCore/ChangeLog:50
> +	   * rendering/InlineBox.cpp:
> +	   (WebCore::InlineBox::canAccommodateEllipsis):
> +	   (WebCore::InlineBox::placeEllipsisBox):
> +	   * rendering/InlineBox.h:
> +	   (InlineBox):
> +	   * rendering/InlineFlowBox.cpp:
> +	   (WebCore::InlineFlowBox::canAccommodateEllipsis):
> +	   (WebCore::InlineFlowBox::placeEllipsisBox):
> +	   * rendering/InlineFlowBox.h:
> +	   (InlineFlowBox):
> +	   * rendering/InlineTextBox.cpp:
> +	   (WebCore::InlineTextBox::placeEllipsisBox):
> +	   * rendering/InlineTextBox.h:
> +	   (InlineTextBox):
> +	   * rendering/RenderBlockLineLayout.cpp:
> +	   (WebCore::RenderBlock::deleteEllipsisLineBoxes):
> +	   (WebCore::RenderBlock::checkLinesForTextOverflow):
> +	   * rendering/RenderDeprecatedFlexibleBox.cpp:
> +	   (WebCore::RenderDeprecatedFlexibleBox::applyLineClamp):
> +	   * rendering/RootInlineBox.cpp:
> +	   (WebCore::RootInlineBox::placeEllipsis):
> +	   (WebCore::RootInlineBox::placeEllipsisBox):
> +	   (WebCore::RootInlineBox::adjustPosition):
> +	   * rendering/RootInlineBox.h:
> +	   (RootInlineBox):

Even though you give an overview and a summary of the change above, it is best
to include per-function comments in the change log.

> Source/WebCore/rendering/InlineBox.h:266
> +    virtual float placeEllipsisBox(bool ltr, float visibleLeftEdge, float
visibleRightEdge, float ellipsisWidth, float &truncatedWidth, bool&);

It took me a while to understand what “truncatedWidth” was based on its name,
but I am not sure I have a better name to suggest for it.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2692
> +	       bool firstLine = (curr == firstRootBox());

I guess this is a very explicit way to check for being the first line. I would
have just initialized a boolean to true and reset it to false at the bottom of
the loop. If you’re doing the above check, I’m not even sure the local variable
is needed. Finally, I don’t think the parentheses around the expression are
needed.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2694
> +	       float logicalLeft =
pixelSnappedLogicalLeftOffsetForLine(logicalHeight(), firstLine);
> +	       float availableLogicalWidth =
logicalRightOffsetForLine(logicalHeight(), false) - logicalLeft;

I don’t think it’s correct to pass logicalHeight() here. Don’t you need to pass
the logical top of the current line?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2696
> +	       updateLogicalWidthForAlignment(textAlign, 0l, logicalLeft,
totalLogicalWidth, availableLogicalWidth, 0);

Why is the null pointer written as “0l” here? Is it really okay to not pass the
trailing space run? What about the last parameter, in the case of justified
text?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2701
> +	       if (ltr)
> +		   curr->adjustPosition((logicalLeft - curr->logicalLeft()),
0);
> +	       else
> +		   curr->adjustPosition(-(curr->logicalLeft() - logicalLeft),
0);

I think adjustPosition takes physical coordinates, not logical ones.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2736
> +		   float totalLogicalWidth;

Maybe you should call this “truncatedWidth” as well?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2740
> +		   float availableLogicalWidth =
pixelSnappedLogicalRightOffsetForLine(logicalHeight(), firstLine) -
logicalLeft;

Again, passing logicalHeight() instead of the line’s logical top. Also, no need
to subtract 0.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2741
> +		   updateLogicalWidthForAlignment(textAlign, 0l, logicalLeft,
totalLogicalWidth, availableLogicalWidth, 0);

Same questions as above about the trailing space run and the expansion
opportunity count.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2745
> +		   if (ltr)
> +		       curr->adjustPosition(logicalLeft, 0);
> +		   else
> +		       curr->adjustPosition(-(availableLogicalWidth -
(logicalLeft + totalLogicalWidth)), 0);

Again, passing a logical offset to a function that takes a physical offset.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:983
> +	   float truncatedWidth = 0;
> +	   lastVisibleLine->placeEllipsis(truncatedWidth, anchorBox ?
ellipsisAndSpaceStr : ellipsisStr, leftToRight, blockLeftEdge, blockRightEdge,
totalWidth, anchorBox);

I guess it’s ok to not fix the same bug for line-clamp right now, but you
should at least add a FIXME about it.

> Source/WebCore/rendering/RootInlineBox.cpp:125
> +void RootInlineBox::placeEllipsis(float &truncatedWidth, const AtomicString&
ellipsisStr,  bool ltr, float blockLeftEdge, float blockRightEdge, float
ellipsisWidth,

Adding this as the first parameter doesn’t feel right to me. One option is to
make this the return value of this function. Or you could add it as the
next-to-last parameter.

> Source/WebCore/rendering/RootInlineBox.cpp:242
> +	   ellipsis->setX(ellipsis->x() + dx);
> +	   ellipsis->setY(ellipsis->y() + dy);

Can you just call ellipsisBox->adjustPosition()?

> Source/WebCore/rendering/RootInlineBox.h:63
> +    LayoutUnit truncatedWidth() const;
> +

What’s this?