[webkit-reviews] review denied: [Bug 29968] Selecting text with text-overflow ellipsis should not show cut off text : [Attachment 45208] Prevents the cut off text from being shown, also makes highlight cover all of ellipsis instead of allowing for partial selection (fixed style violations)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Jessie Berlin <jberlin at webkit.org>'s request for
review:
Bug 29968: Selecting text with text-overflow ellipsis should not show cut off
text
https://bugs.webkit.org/show_bug.cgi?id=29968

Attachment 45208: Prevents the cut off text from being shown, also makes
highlight cover all of ellipsis instead of allowing for partial selection
(fixed style violations)
https://bugs.webkit.org/attachment.cgi?id=45208&action=review

------- Additional Comments from mitz at webkit.org
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.