Bug 88705 – Text with text-overflow:ellipsis and text-align:right is left aligned

• Component: Text
• Status: RESOLVED FIXED

[Benjamin Poulain]

Text with text-overflow:ellipsis and text-align:right is left aligned.

<rdar://problem/11099423>

[Benjamin Poulain]

Created attachment 146695 [details]
Patch

Comment on attachment 146695 [details]
Patch

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?

[Benjamin Poulain]

I will make an update.


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

There is nothing special about center and justified, updateLogicalWidthForAlignment() returns the correct offset for those cases.
I should have included that in the new tests case, I will do that.

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

I thought this was clearer than the other options.
Apparently not, so I changed it as you suggested.

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

This line was copied from "RenderBlock::startAlignedOffsetForLine()". The 0l is likely a typo in the original code.

Justified text does not really matter if we are truncating because the text is either already too short, or it will appear too short. There is not expansion with truncation.

Regarding the trailing space run, its width has already been modified by the layout when we are shifting the line. Since we have truncated text, the trailing space run could be behind the truncation and is irrelevant for the text align code.
I am new to this code so I might be completely wrong there, but I don't think the trailing space run is relevant because of the truncation.


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

Yep, you are right.

This made me realize the Ellipsis box is computed in physical coordinate. The whole ellipsis code is broken in vertical writing mode (no truncation, and the ellipsis is placed horizontally) :(

I will address the vertical mode separately as it is unrelated with the alignment issues.

[Benjamin Poulain]

Created attachment 147922 [details]
Patch

Comment on attachment 147922 [details]
Patch

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

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

Typo: “layed” instead of “laid”.

> Source/WebCore/rendering/InlineBox.h:75
> +    void adjustLogicalPosition(float dx, float dy)

We should reserve “x” and “y” for the physical axes.

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

You should use curr->lineTop() here, not logicalTop().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2726
> +        LayoutUnit blockRightEdge = logicalRightOffsetForLine(curr->logicalTop(), firstLine);
> +        LayoutUnit blockLeftEdge = logicalLeftOffsetForLine(curr->logicalTop(), firstLine);

Same here.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2740
> +                float truncatedWidth = pixelSnappedLogicalRightOffsetForLine(curr->logicalTop(), firstLine);

Same here.

[Benjamin Poulain]

Committed r121085: <http://trac.webkit.org/changeset/121085>