Bug 29968 – Selecting text with text-overflow ellipsis should not show cut off text

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Erik Arvidsson]

Created attachment 40451 [details]
Select the text

When some text has text-overflow: ellipsis and the text is selected the '...' is showing through the selection.

[Erik Arvidsson]

Created attachment 40452 [details]
Screenshot

[Erik Arvidsson]

This is windows only. It works differently on mac. On mac the text that got cut off continues to be cut of and the dots continue to be shown.

I'll update the bug to say that the text should continue to be cut off after selection and the dots needs to be using the highlight text colot.

[Jessie Berlin]

Currently working on a patch for this.

Is there a suggested way to write a test for what gets displayed in the selection (other than manual test)?

[Erik Arvidsson]

Can't you use eventSender? Send mousedown, mousemove and mouseup and thn check the selection range? The range might not correctly reflect this though. In that case a pixel test should work.

[Jessie Berlin]

(In reply to comment #4)
> Can't you use eventSender? Send mousedown, mousemove and mouseup and thn check
> the selection range? The range might not correctly reflect this though. In that
> case a pixel test should work.

The selection range won't work because the selection range can include as much of the cut off text as would be under the cursor if the text wasn't cut off (not sure if that is a bug or not). I will try a pixel test.

One other question. I have gotten the text to cut off at the right place, but I am having difficulty with painting the ellipsis the right color.

It seems that half the time (on both Mac and Windows) it is impossible to select just one of the dots in the ellipsis and half the time (it selects all at once or none), and half of the time it is possible to select just one of the dots. This seems to vary with direction and speed of selection, but not in a way that I have been able to predict. What is the correct behavior here? Are you supposed to be able to select a single dot in the ellipsis at a time?

[Jessie Berlin]

What I didn't realize before is that the ellipsis is one character (U+2026). Does anyone know why it might only paint a part of the character? (painting only one out of the three dots or two out of the three dots)

(In reply to comment #6)
> What I didn't realize before is that the ellipsis is one character (U+2026).
> Does anyone know why it might only paint a part of the character? (painting
> only one out of the three dots or two out of the three dots)

Perhaps it’s computing a selection rect based on the widths of the truncated characters. May be easier to isolate the problem by creating a selection programmatically that ends one (narrow) character after the last visible character.

[Jessie Berlin]

(In reply to comment #7)
> (In reply to comment #6)
> > What I didn't realize before is that the ellipsis is one character (U+2026).
> > Does anyone know why it might only paint a part of the character? (painting
> > only one out of the three dots or two out of the three dots)
> Perhaps it’s computing a selection rect based on the widths of the truncated
> characters. May be easier to isolate the problem by creating a selection
> programmatically that ends one (narrow) character after the last visible
> character.

Well currently there isn't any code in EllipsisBox::paint that does anything with the selection except the stuff that I have added, and that just computes whether there is a selection and changes the fill color accordingly, which should apply to the entire ellipsis character:

void EllipsisBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty)
{
    GraphicsContext* context = paintInfo.context;
    RenderStyle* style = m_renderer->style(m_firstLine);
    Color textColor = style->color();
    if (textColor != context->fillColor())
        context->setFillColor(textColor, style->colorSpace());
    bool setShadow = false;
    if (style->textShadow()) {
        context->setShadow(IntSize(style->textShadow()->x, style->textShadow()->y),
                           style->textShadow()->blur, style->textShadow()->color, style->colorSpace());
        setShadow = true;
    }

/** Start code I have recently added **/
	if (selectionState() != RenderObject::SelectionNone) {
        IntRect selectionRect = renderer()->view()->selectionBounds();
        IntRect ellipsisRect = IntRect(m_x + tx, m_y + ty, m_width, m_height);
        // Determine whether or not the selection contains the EllipsisBox. An
        // EllipsisBox is never partially selected; it is either entirely inside
        // the selection, or not selected at all.
        if (selectionRect.intersects(ellipsisRect)) {
    		Color foreground = paintInfo.forceBlackText ? Color::black : renderer()->selectionForegroundColor();
            if (foreground.isValid() && foreground != textColor)
		    	context->setFillColor(foreground, style->colorSpace());
        }
	}
/** End code I have recently added **/

    const String& str = m_str;
    context->drawText(style->font(), TextRun(str.characters(), str.length(), false, 0, 0, false, style->visuallyOrdered()), IntPoint(m_x + tx, m_y + ty + style->font().ascent()));

So drawText shouldn't be dependent on the selection rect for anything besides the color that should be applied to all three dots in the ellipsis character, unless there is something that I am missing in the later drawGlyphs that get called by drawText that tries to get the selection rect and use that.

Another uninformed guess: perhaps modifying the selection doesn’t always invalidate the entire ellipsis (RenderView::setSelection() uses RenderSelectionInfo objects to do the invalidation, and I think ultimately what gets repainted in this case depends on RenderText::selectionRectForRepaint(), which doesn’t appear to account for the ellipsis).

If my guess is correct, then when you see “partially selected ellipsis” you can correct the situation by forcing a repaint (resizing the window will do it. I don’t remember if Safari on Windows has a Repaint command in its debug menu).

Looking at your recently-added code, I don’t think it’s correct. Conceptually, it’s strange to use a geometric criterion to decide the selection state of the ellipsis. Moreover, selectionBounds() is in document coordinates, but your ellipsisRect is in layer coordinates, which can be different under transforms. Practically, this can do the wrong thing under certain transforms (a slight rotation will cause the bounding rect of the last selected character to intersect the bounding box of the character next to it).

[Jessie Berlin]

(In reply to comment #9)
> Another uninformed guess: perhaps modifying the selection doesn’t always
> invalidate the entire ellipsis (RenderView::setSelection() uses
> RenderSelectionInfo objects to do the invalidation, and I think ultimately what
> gets repainted in this case depends on RenderText::selectionRectForRepaint(),
> which doesn’t appear to account for the ellipsis).
> If my guess is correct, then when you see “partially selected ellipsis” you can
> correct the situation by forcing a repaint (resizing the window will do it. I
> don’t remember if Safari on Windows has a Repaint command in its debug menu).

Build broken right now, I will make more comments on this when I can try running it again.

> Looking at your recently-added code, I don’t think it’s correct. Conceptually,
> it’s strange to use a geometric criterion to decide the selection state of the
> ellipsis. Moreover, selectionBounds() is in document coordinates, but your
> ellipsisRect is in layer coordinates, which can be different under transforms.
> Practically, this can do the wrong thing under certain transforms (a slight
> rotation will cause the bounding rect of the last selected character to
> intersect the bounding box of the character next to it).

I wasn't aware the sets of coordinates were different. The reason I resorted to the geometric criterion to decide the selection state is that there was no overridden selectionState for EllipsisBox, and calling selectionState() resulted in the InlineTextBox::selectionState() being called, which had no bearing on whether or not the EllipsisBox was selected and would return SelectionBoth even when the selection was on a part of the text that was far away from the EllipsisBox.

Is there a better way to find the selection state of the EllipsisBox?

[Jessie Berlin]

Created attachment 43666 [details]
Shows the results of selecting that starts in the text, moves over the ellipsis and then back to just being in the text.

Causing a repaint by resizing the window does make it correct itself, but is a whole repaint overkill here?

[Jessie Berlin]

(In reply to comment #11)
> Created an attachment (id=43666) [details]
> Shows the results of selecting that starts in the text, moves over the ellipsis
> and then back to just being in the text.
> Causing a repaint by resizing the window does make it correct itself, but is a
> whole repaint overkill here?

I talked to Dan about this, and he identified another issue that is not Windows-specific:

" in my testing on Mac it seems like we just paint a selection highlight for the truncated text box, ignoring the truncation, extending into the ellipsis. Here is the test case I used:


<div id="target" style="width: 95px; white-space: pre; overflow: hidden; text-overflow: ellipsis;">Lorem ipsum dolor</div> <script> getSelection().setBaseAndExtent(target.firstChild, 0, target.firstChild, 11); </script>


If I replace the m at the end of “ipsum” with an n, the highlighted rect becomes slightly shorter (because n is narrower). This is all under “no incremental repaints” conditions, so that part doesn’t come into play. Anyway, it’s incorrect behavior that needs to be fixed, probably independently of getting the ellipsis box to paint with the correct selection state."

On Windows, the highlight doesn't extend at all into the ellipsis (stops directly after either an n or m). I thought that it might be a question of font width differences, but I couldn't get a combination of letters that would get the highlight to protrude into the ellipsis partway.

He also suggested that I base my decision of whether the ellipsis is highlighted on non-geometic criteria:

" the naïve thing to do would be for the implementation of EllipsisBox::selectionState() to walk the text boxes in the line and see if one of them satisfies this condition: it is truncated (m_truncation is not cNoTruncation), its selection state is SelectionStart or SelectionInside, and the selection end of its renderer (i.e. the endPos that RenderText::selectionStartEnd() returns) is past its truncation position (using InlineTextBox::selectionState() terminology). If it finds such a text box on the line, then it means that the selection extends into the truncated part, so the ellipsis box is “selected”. It might make more sense for RootInlineBox::selectionState() or some other RootInlineBox method to make this determination (as it walks the lines on the box anyway, and has a reference to the ellipsis box) and then stash the result in the EllipsisBox."

A problem I found trying to implement this was that the selection state of the InlineTextBox never seems to be SelectionStart or SelectionInside - it is always either SelectionNone or SelectionBoth. I am not sure why this is, and wonder if it is a bug.

However, with the criteria being SelectionBoth, it does the initial highlight color of the ellipsis correctly.

I am currently working on getting RenderText::selectionRectForRepaint() to recognize the similar situation and do the repaint of the ellipsis correctly, as Dan also suggested, since right now the ellipsis will remain the highlighted color once the selection is reduced to no longer cover the ellipsis. (Right now trying to figure out how to get the necessary selectionRect for the EllipsisBox).

(In reply to comment #12)
> A problem I found trying to implement this was that the selection state of the
> InlineTextBox never seems to be SelectionStart or SelectionInside - it is
> always either SelectionNone or SelectionBoth. I am not sure why this is, and
> wonder if it is a bug.

How are you creating the selection? I suggest doing it programmatically using getSelection().setBaseAndExtent() like in the above snippet, because I suspect that dragging across the ellipsis to select doesn’t do the right thing (in my experience, it maps to positions at the beginning of the truncated text box).

[Jessie Berlin]

(In reply to comment #13)
> (In reply to comment #12)
> > A problem I found trying to implement this was that the selection state of the
> > InlineTextBox never seems to be SelectionStart or SelectionInside - it is
> > always either SelectionNone or SelectionBoth. I am not sure why this is, and
> > wonder if it is a bug.
> How are you creating the selection? I suggest doing it programmatically using
> getSelection().setBaseAndExtent() like in the above snippet, because I suspect
> that dragging across the ellipsis to select doesn’t do the right thing (in my
> experience, it maps to positions at the beginning of the truncated text box).

It only reports SelectionNone or SelectionBoth, both when I do it programmatically (using your above snippet), and when I do it by hand.

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > A problem I found trying to implement this was that the selection state of the
> > > InlineTextBox never seems to be SelectionStart or SelectionInside - it is
> > > always either SelectionNone or SelectionBoth. I am not sure why this is, and
> > > wonder if it is a bug.
> > How are you creating the selection? I suggest doing it programmatically using
> > getSelection().setBaseAndExtent() like in the above snippet, because I suspect
> > that dragging across the ellipsis to select doesn’t do the right thing (in my
> > experience, it maps to positions at the beginning of the truncated text box).
> 
> It only reports SelectionNone or SelectionBoth, both when I do it
> programmatically (using your above snippet), and when I do it by hand.

Oh, I guess that actually makes sense when there’s only the one text box. So really it seems like you should consider all states except SelectionNone. By the way, when looking at InlineTextBox::selectionState() now, I think I discovered yet another bug: it calls RenderText::selectionStartEnd() but that is actually wrong. It should call InlineTextBox’s own selectionStartEnd(). I’ll try to make a test case that shows that it’s wrong, but in comment #12 where I say RenderText::selectionStartEnd(), I really mean InlineTextBox::selectionStartEnd().

(In reply to comment #15)
> I think I discovered yet another bug: it calls RenderText::selectionStartEnd() but that
> is actually wrong.

I was actually wrong. It only uses the values when they are correct.

[Jessie Berlin]

Created attachment 45197 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection.

[WebKit Review Bot]

Attachment 45197 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/EllipsisBox.cpp:44:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/EllipsisBox.cpp:50:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/EllipsisBox.cpp:51:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/EllipsisBox.cpp:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/rendering/EllipsisBox.cpp:82:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/InlineTextBox.cpp:307:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/InlineTextBox.cpp:308:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/InlineTextBox.cpp:483:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/InlineTextBox.cpp:496:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/RenderText.cpp:1177:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 10

[Jessie Berlin]

Comment on attachment 45197 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection.

Fixing. Apparently I misunderstood how to use check-webkit-style.

[Jessie Berlin]

Created attachment 45208 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (fixed style violations)

Fixed style violations.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45208 [details] without any errors.

Comment on attachment 45208 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (fixed style violations)

Did you test this with a ::selection pseudoclass style that specifies background and foreground colors?

> +#include "RenderView.h"

Why is this necessary?

> +    // If the text color ends up being the same as the selection background, invert the selection
> +    // background.  This should basically never happen, since the selection has transparency.
> +    if (textColor == c)
> +        c = Color(0xff - c.red(), 0xff - c.green(), 0xff - c.blue());

The text color can have transparency, too. The above code will be reached, for example, when the selection background color is solid red and the text color is rgba(255, 0, 0, 0.8).

> +    AtomicString ellipsisString() const { return m_str; }

Not sure why this is needed (see my comments below), but it’s better to return a const AtomicString&.

> +    if (root()->ellipsisBox() && (m_truncation != cNoTruncation)) {

Extra parentheses around the second expression. More importantly, you should reverse the order of the expressions, because root() is a (recursive) function call, whereas m_truncation is almost always cNoTruncation.

> +            root()->ellipsisBox()->setSelectionState(
> +                ((end >= m_truncation) && (start <= (int)(m_truncation + root()->ellipsisBox()->ellipsisString().length()))) ?
> +                RenderObject::SelectionInside : RenderObject::SelectionNone);

This condition looks very mysterious to me (and has some redundant parentheses). How does the length of the ellipsis string factor into this? The question is whether you want to highlight the ellipsis only if the selection includes the very first character after the truncation point, or whenever it includes any character after the truncation point. I think the latter makes more sense, but even if you go for the former, I think you just want the constant 1, not the length of the string “…” (if we are ever clever enough to use the string “...” if the font doesn’t have an ellipsis character, you’d still want 1). Also, change the C-style cast to a static_cast<int>().

> +        } else
> +            root()->ellipsisBox()->setSelectionState(RenderObject::SelectionNone);

It’s a bit strange that getting the selection state of an InlineTextBox acts as a setter for the selection state of an ellipsis box, which it doesn’t even “own”. Have you looked in the direction of making this the responsibility of the root line box?

> -static void paintTextWithShadows(GraphicsContext* context, const Font& font, const TextRun& textRun, int startOffset, int endOffset, const IntPoint& textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked)
> +static void paintTextWithShadows(GraphicsContext* context, const Font& font, const TextRun& textRun, int startOffset, int endOffset, const IntPoint& textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked, int maxLength)

I think maxLength should go next to endOffset. I am not a fan of this parameter’s name. Can you come up with a better name for it? Something that suggests that it’s the truncation point, perhaps?

> +    int textLength = m_len;

Can you name this “length” like you did below?

> +        sPos = sPos > m_truncation ? m_truncation : sPos;
> +        ePos = ePos > m_truncation ? m_truncation : ePos;
> +        textLength = textLength > m_truncation ? m_truncation : textLength;

I prefer using min() for this. In the last statement, can textLength ever be less than m_truncation?

> +        EllipsisBox* ellipsis = box->root()->ellipsisBox();
> +        if (ellipsis) {

You should move the definition of ellipsis into the if condition, since it’s only used in that scope. It’s inefficient to call root() on every box. You should do the fast (box->truncation() != cNoTruncation) test first, and only then look for the ellipsis box.

> +            int ePos = min(endPos - (int)box->start(), (int)box->len());
> +            int sPos = max(startPos - (int)box->start(), 0);

Instead of doing those C-style casts, use static_cast<int>(). Better yet, use min<int> and you won’t need to cast!

> +            if ((truncation != cNoTruncation) && (ePos >= truncation)
> +                && (sPos <= (int)(truncation + ellipsis->ellipsisString().length())))

Again with the extra parentheses and C-style casts. And with using the length of the ellipsis string which seems nonsensical. Maybe I’m missing something.

This is really good! Please address my important comments.

[Jessie Berlin]

I will be posting my new patch in the next attachment.

(In reply to comment #22)
> (From update of attachment 45208 [details])
> Did you test this with a ::selection pseudoclass style that specifies
> background and foreground colors?

No, I actually wasn't aware of that pseudo-element before. However, I did test it since, and the painting of the selected ellipsis matches the painting of the rest of the text. 

> 
> > +#include "RenderView.h"
> 
> Why is this necessary?

It isn't. What is necessary is RootInlineBox.h. Fixed.

> 
> > +    // If the text color ends up being the same as the selection background, invert the selection
> > +    // background.  This should basically never happen, since the selection has transparency.
> > +    if (textColor == c)
> > +        c = Color(0xff - c.red(), 0xff - c.green(), 0xff - c.blue());
> 
> The text color can have transparency, too. The above code will be reached, for
> example, when the selection background color is solid red and the text color is
> rgba(255, 0, 0, 0.8).


True. Comment fixed.

> 
> > +    AtomicString ellipsisString() const { return m_str; }
> 
> Not sure why this is needed (see my comments below), but it’s better to return
> a const AtomicString&.

This is needed because the ellipsisString can sometimes be the ellipsis character and a space afterwards (which I found an example of in RenderFlexibleBox.cpp - I am not sure how often that will be a problem for selection, but I figured it would be better to allow for that possibility). I changed it to return const AtomicString&, but would it be better to just expose the length?

> 
> > +    if (root()->ellipsisBox() && (m_truncation != cNoTruncation)) {
> 
> Extra parentheses around the second expression. More importantly, you should
> reverse the order of the expressions, because root() is a (recursive) function
> call, whereas m_truncation is almost always cNoTruncation.

Done. Also factored out the root()->ellpsisBox() so that it root() was called no more than twice here

> 
> > +            root()->ellipsisBox()->setSelectionState(
> > +                ((end >= m_truncation) && (start <= (int)(m_truncation + root()->ellipsisBox()->ellipsisString().length()))) ?
> > +                RenderObject::SelectionInside : RenderObject::SelectionNone);
> 
> This condition looks very mysterious to me (and has some redundant
> parentheses). How does the length of the ellipsis string factor into this? 

The problem is that the selection bounds are often much much longer than the text that is shown. So if I truncate a 100 char text at 15 characters and do a selection (click and drag) starting at where the 90th character should be and going to where the 95th character should be, it still considers the text to be selected. I didn't think it made much sense to show the ellipsis as selected when the selection was actually happening some 70 characters away and didn't come into contact with the ellipsis at all.
 
Hence, I wanted to make sure the start of the selection was before the end of the ellipsis (which would be at the m_truncation + lengthOfEllipsis position).
 
I added a comment to this effect and tried to refactor the code so it would be a little more clear.

> The
> question is whether you want to highlight the ellipsis only if the selection
> includes the very first character after the truncation point, or whenever it
> includes any character after the truncation point. I think the latter makes
> more sense, but even if you go for the former, I think you just want the
> constant 1, not the length of the string “…” (if we are ever clever enough to
> use the string “...” if the font doesn’t have an ellipsis character, you’d
> still want 1). Also, change the C-style cast to a static_cast<int>().

Refactoring removed the C-style cast issue.

> 
> > +        } else
> > +            root()->ellipsisBox()->setSelectionState(RenderObject::SelectionNone);
> 
> It’s a bit strange that getting the selection state of an InlineTextBox acts as
> a setter for the selection state of an ellipsis box, which it doesn’t even
> “own”. Have you looked in the direction of making this the responsibility of
> the root line box?

That was actually my initial approach. The problem with that is the root line box doesn't get its selection state recalculated when selection is lost entirely. When I had selected the text and had included the ellipsis and then clicked elsewhere on the page, the selection was getting removed from the InlineTextBox, but not from the EllipsisBox. I am not sure if that is a bug or not.
 
 I am also not really sure I fully understand why the InlineTextBox doesn't own its ellipsis. Why is that? Since the InlineTextBox knows the selection and truncation information and is part of placing the EllipsisBox (InlineTextBox::placeEllipsisBox), couldn't it also be in charge of notifying the EllipsisBox of its selection? 

> 
> > -static void paintTextWithShadows(GraphicsContext* context, const Font& font, const TextRun& textRun, int startOffset, int endOffset, const IntPoint& textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked)
> > +static void paintTextWithShadows(GraphicsContext* context, const Font& font, const TextRun& textRun, int startOffset, int endOffset, const IntPoint& textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked, int maxLength)
> 
> I think maxLength should go next to endOffset. I am not a fan of this
> parameter’s name. Can you come up with a better name for it? Something that
> suggests that it’s the truncation point, perhaps?

 Does truncationPoint work? I toyed around with the idea of truncationLength, but that seemed a bit redundant.

> 
> > +    int textLength = m_len;
> 
> Can you name this “length” like you did below?

Done.

> 
> > +        sPos = sPos > m_truncation ? m_truncation : sPos;
> > +        ePos = ePos > m_truncation ? m_truncation : ePos;
> > +        textLength = textLength > m_truncation ? m_truncation : textLength;
> 
> I prefer using min() for this. In the last statement, can textLength ever be
> less than m_truncation?

Changed to use min<int>().
 
textLength (now named length) should never be less than m_truncation. If it ever was, then it should be "truncating" to that lesser length anyways because there wouldn't be text to make up the difference.

> 
> > +        EllipsisBox* ellipsis = box->root()->ellipsisBox();
> > +        if (ellipsis) {
> 
> You should move the definition of ellipsis into the if condition, since it’s
> only used in that scope. It’s inefficient to call root() on every box. You
> should do the fast (box->truncation() != cNoTruncation) test first, and only
> then look for the ellipsis box.

Done.

> 
> > +            int ePos = min(endPos - (int)box->start(), (int)box->len());
> > +            int sPos = max(startPos - (int)box->start(), 0);
> 
> Instead of doing those C-style casts, use static_cast<int>(). Better yet, use
> min<int> and you won’t need to cast!

Changed to use min<int>()

> 
> > +            if ((truncation != cNoTruncation) && (ePos >= truncation)
> > +                && (sPos <= (int)(truncation + ellipsis->ellipsisString().length())))
> 
> Again with the extra parentheses and C-style casts. And with using the length
> of the ellipsis string which seems nonsensical. Maybe I’m missing something.

Fixed the extra parentheses and C-style casts issue. See above for the length of the ellipsis string issue

> 
> This is really good! Please address my important comments.

Thanks for the quick feedback!

[Jessie Berlin]

Created attachment 45334 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (version 3)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45334 [details] without any errors.

Comment on attachment 45334 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (version 3)

(In reply to comment #23)
> > > +    AtomicString ellipsisString() const { return m_str; }
> > 
> > Not sure why this is needed (see my comments below), but it’s better to return
> > a const AtomicString&.
> 
> This is needed because the ellipsisString can sometimes be the ellipsis
> character and a space afterwards (which I found an example of in
> RenderFlexibleBox.cpp - I am not sure how often that will be a problem for
> selection, but I figured it would be better to allow for that possibility). I
> changed it to return const AtomicString&, but would it be better to just expose
> the length?

I would prefer that, but I still don’t think it’s needed.

> > > +            root()->ellipsisBox()->setSelectionState(
> > > +                ((end >= m_truncation) && (start <= (int)(m_truncation + root()->ellipsisBox()->ellipsisString().length()))) ?
> > > +                RenderObject::SelectionInside : RenderObject::SelectionNone);
> > 
> > This condition looks very mysterious to me (and has some redundant
> > parentheses). How does the length of the ellipsis string factor into this? 
> 
> The problem is that the selection bounds are often much much longer than the
> text that is shown. So if I truncate a 100 char text at 15 characters and do a
> selection (click and drag) starting at where the 90th character should be and
> going to where the 95th character should be, it still considers the text to be
> selected. I didn't think it made much sense to show the ellipsis as selected
> when the selection was actually happening some 70 characters away and didn't
> come into contact with the ellipsis at all.
> 
> Hence, I wanted to make sure the start of the selection was before the end of
> the ellipsis (which would be at the m_truncation + lengthOfEllipsis position).

OK, I’m convinced that the ellipsis should only be highlighted if the selection starts at or before the point of truncation, but I still don’t understand why the length of the ellipsis string should factor into this. It’s not even like we’re highlighting individual characters in the ellipsis string.

>  I am also not really sure I fully understand why the InlineTextBox doesn't own
> its ellipsis. Why is that? Since the InlineTextBox knows the selection and
> truncation information and is part of placing the EllipsisBox
> (InlineTextBox::placeEllipsisBox), couldn't it also be in charge of notifying
> the EllipsisBox of its selection? 

The thing is that it’s not in charge of creating the ellipsis box. You can imagine a world where the RootInlineBox re-uses the same EllipsisBox for a given line even if the text boxes on the lines changed. But since we’re not in that world, I think what you did is fine.

>  Does truncationPoint work?

Yes.

> > > +        textLength = textLength > m_truncation ? m_truncation : textLength;
> > 
> > I prefer using min() for this. In the last statement, can textLength ever be
> > less than m_truncation?
> 
> Changed to use min<int>().
> 
> textLength (now named length) should never be less than m_truncation.

I’m sorry. What I meant was that the statement quoted above is equivalent to
        textLength = m_truncation;
so should be written as such.

I don’t want to r+ this version because I still don’t agree with the logic that takes the length of the ellipsis string into account.

And there are still redundant parentheses on line 97 of InlineTextBox.cpp!

[Jessie Berlin]

(In reply to comment #26)
> (From update of attachment 45334 [details])
> (In reply to comment #23)
> > > > +    AtomicString ellipsisString() const { return m_str; }
> > > 
> > > Not sure why this is needed (see my comments below), but it’s better to return
> > > a const AtomicString&.
> > 
> > This is needed because the ellipsisString can sometimes be the ellipsis
> > character and a space afterwards (which I found an example of in
> > RenderFlexibleBox.cpp - I am not sure how often that will be a problem for
> > selection, but I figured it would be better to allow for that possibility). I
> > changed it to return const AtomicString&, but would it be better to just expose
> > the length?
> 
> I would prefer that, but I still don’t think it’s needed.

I removed it entirely (see below)

> 
> > > > +            root()->ellipsisBox()->setSelectionState(
> > > > +                ((end >= m_truncation) && (start <= (int)(m_truncation + root()->ellipsisBox()->ellipsisString().length()))) ?
> > > > +                RenderObject::SelectionInside : RenderObject::SelectionNone);
> > > 
> > > This condition looks very mysterious to me (and has some redundant
> > > parentheses). How does the length of the ellipsis string factor into this? 
> > 
> > The problem is that the selection bounds are often much much longer than the
> > text that is shown. So if I truncate a 100 char text at 15 characters and do a
> > selection (click and drag) starting at where the 90th character should be and
> > going to where the 95th character should be, it still considers the text to be
> > selected. I didn't think it made much sense to show the ellipsis as selected
> > when the selection was actually happening some 70 characters away and didn't
> > come into contact with the ellipsis at all.
> > 
> > Hence, I wanted to make sure the start of the selection was before the end of
> > the ellipsis (which would be at the m_truncation + lengthOfEllipsis position).
> 
> OK, I’m convinced that the ellipsis should only be highlighted if the selection
> starts at or before the point of truncation, but I still don’t understand why
> the length of the ellipsis string should factor into this. It’s not even like
> we’re highlighting individual characters in the ellipsis string.

I was thinking that if the ellipsis string included something besides the ellipsis character (like the space character example), it might make sense that a valid selection could start anywhere within that ellipsis string. However, you do make a good point that we aren't highlighting individual characters in the ellipsis string, so I have changed it to  check for the selection starting at or before the point of truncation.

> 
> >  I am also not really sure I fully understand why the InlineTextBox doesn't own
> > its ellipsis. Why is that? Since the InlineTextBox knows the selection and
> > truncation information and is part of placing the EllipsisBox
> > (InlineTextBox::placeEllipsisBox), couldn't it also be in charge of notifying
> > the EllipsisBox of its selection? 
> 
> The thing is that it’s not in charge of creating the ellipsis box. You can
> imagine a world where the RootInlineBox re-uses the same EllipsisBox for a
> given line even if the text boxes on the lines changed. But since we’re not in
> that world, I think what you did is fine.
> 
> >  Does truncationPoint work?
> 
> Yes.
> 
> > > > +        textLength = textLength > m_truncation ? m_truncation : textLength;
> > > 
> > > I prefer using min() for this. In the last statement, can textLength ever be
> > > less than m_truncation?
> > 
> > Changed to use min<int>().
> > 
> > textLength (now named length) should never be less than m_truncation.
> 
> I’m sorry. What I meant was that the statement quoted above is equivalent to
>         textLength = m_truncation;
> so should be written as such.

Fixed.

> 
> I don’t want to r+ this version because I still don’t agree with the logic that
> takes the length of the ellipsis string into account.
> 
> And there are still redundant parentheses on line 97 of InlineTextBox.cpp!

Sorry, fixed in the patch that I will post immediately after this reply.

[Jessie Berlin]

Created attachment 45461 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (version 4)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45461 [details] without any errors.

Comment on attachment 45461 [details]
Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (version 4)

> +            EllipsisBox* ellipsis;
> +            if ((ellipsis = box->root()->ellipsisBox())) {

can be written as
           if (EllipsisBox* ellipsis = box->root()->ellipsisBox()) {

r=me!

[Jessie Berlin]

(In reply to comment #30)
> (From update of attachment 45461 [details])
> > +            EllipsisBox* ellipsis;
> > +            if ((ellipsis = box->root()->ellipsisBox())) {
> 
> can be written as
>            if (EllipsisBox* ellipsis = box->root()->ellipsisBox()) {
> 

Done.

> r=me!

Thanks! Committed in r52548(http://trac.webkit.org/changeset/52548)