Bug 76118 – Add text-overflow support that allows placeholder and value text to show an ellipsis when not focused

• Component: Forms
• Status: RESOLVED FIXED

[Jon Lee]

<rdar://problem/9271742>

[Jon Lee]

Created attachment 122141 [details]
Patch

[Jon Lee]

Created attachment 122169 [details]
Patch

Rebase onto ToT, and get rid of old, inapplicable comment

[Kent Tamura]

This is a new feature.  You have to announce this in webkit-dev.

http://www.webkit.org/coding/adding-features.html

[WebKit Review Bot]

Comment on attachment 122169 [details]
Patch

Attachment 122169 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11222259

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Jon Lee]

I have some additional cleanup and test fixing to do. Obsoleting patch.

[Jon Lee]

Created attachment 122381 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122381 [details]
Patch

Attachment 122381 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11149507

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/getComputedStyle/computed-style.html

[Darin Adler]

Comment on attachment 122381 [details]
Patch

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

Can either of the new tests be done as ref tests? There still would be dependency on font size, but using a reference test would eliminate dependency on other aspects of text field appearance.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1809
> +            if (style->controlTextOverflow())
> +                return cssValuePool->createIdentifierValue(CSSValueEllipsis);
> +            return cssValuePool->createIdentifierValue(CSSValueClip);

Coding style here is strange. We don’t use "!= 0" to compare with the constant zero, but we usually do use "!= x" when comparing with a named constant like TextOverflowClip. This was just copied and pasted from the case above, but it’s curious in both cases.

It’s mildly unpleasant that we can’t somehow use the code in CSSPrimitiveValueMappings.h to do this for us since it already has a switch statement to convert values.

> Source/WebCore/html/HTMLInputElement.cpp:693
> +bool HTMLInputElement::textShouldBeTruncated() const
> +{
> +    return document()->focusedNode() != this
> +        && isTextField()
> +        && renderer()
> +        && renderer()->style()->controlTextOverflow() == TextOverflowEllipsis;
> +}

Generally speaking new HTMLInputElement code should not use functions like isTextField. Instead, virtual functions should be added to the InputType class, and the logic should go there.

This function is called by the HTMLInputElement’s renderer. This makes the isTextField() check and the renderer() check both redundant; the existence of the renderer already establishes both facts. And the style logic is the kind of thing that normally goes into renderer code. So I would suggest moving this entire function into RenderTextControlSingleLine and not making any changes to HTMLInputElement and HTMLTextFormControlElement at all.

RenderTextControlSingleLine already has code that checks focus, specifically RenderTextControlSingleLine::capsLockStateMayHaveChanged, so this would not be something new. Looking at that function makes me wonder if you also need to check frame->selection()->isFocusedAndActive() as it does. What do we want these fields to look like when the field has focus, but the window does not.

> Source/WebCore/html/HTMLInputElement.h:123
> +    virtual bool textShouldBeTruncated() const;

This override should be marked OVERRIDE.

Or maybe this can be removed (see above) or be a non-virtual function (see below).

> Source/WebCore/html/HTMLTextFormControlElement.h:56
> +    virtual bool textShouldBeTruncated() const;

Why is this needed? I don’t see any callers calling this on an HTMLTextFormControlElement*. They seem to have an HTMLInputElement*.

If this can be removed, then textShouldBeTruncated can be made non-virtual.

[Jon Lee]

Comment on attachment 122381 [details]
Patch

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

>> Source/WebCore/html/HTMLInputElement.cpp:693
>> +}
> 
> Generally speaking new HTMLInputElement code should not use functions like isTextField. Instead, virtual functions should be added to the InputType class, and the logic should go there.
> 
> This function is called by the HTMLInputElement’s renderer. This makes the isTextField() check and the renderer() check both redundant; the existence of the renderer already establishes both facts. And the style logic is the kind of thing that normally goes into renderer code. So I would suggest moving this entire function into RenderTextControlSingleLine and not making any changes to HTMLInputElement and HTMLTextFormControlElement at all.
> 
> RenderTextControlSingleLine already has code that checks focus, specifically RenderTextControlSingleLine::capsLockStateMayHaveChanged, so this would not be something new. Looking at that function makes me wonder if you also need to check frame->selection()->isFocusedAndActive() as it does. What do we want these fields to look like when the field has focus, but the window does not.

I will work on moving this check out to the renderer.

As for the focus issue, I believe we want the text field to look the same as if it was not styled with this property. That is, if the placeholder is visible, changing the active window should show a clipped placeholder. If there is text, changing the active window should keep it clipped, and should not return the scroll position to the left (or right as the case may be). I will add a test for this.

>> Source/WebCore/html/HTMLInputElement.h:123
>> +    virtual bool textShouldBeTruncated() const;
> 
> This override should be marked OVERRIDE.
> 
> Or maybe this can be removed (see above) or be a non-virtual function (see below).

Done.

>> Source/WebCore/html/HTMLTextFormControlElement.h:56
>> +    virtual bool textShouldBeTruncated() const;
> 
> Why is this needed? I don’t see any callers calling this on an HTMLTextFormControlElement*. They seem to have an HTMLInputElement*.
> 
> If this can be removed, then textShouldBeTruncated can be made non-virtual.

Done.

> LayoutTests/fast/css/webkit-control-text-overflow-focus-placeholder.html:16
> +</html>

This test could be a ref test. I will convert it.

> LayoutTests/fast/css/webkit-control-text-overflow-focus-value.html:16
> +</html>

This test could be a ref test. I will convert it.

> LayoutTests/fast/css/webkit-control-text-overflow-input.html:37
> +    </p>

I feel the above section might be overkill to try to do as a ref test, but...

> LayoutTests/fast/css/webkit-control-text-overflow-input.html:65
> +    </p>

... everything up to this point could be a ref test. I will split it out into another one.

I also realize now that I forgot to add RTL tests.

[Jon Lee]

(In reply to comment #8)
> (From update of attachment 122381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122381&action=review
> 
> Can either of the new tests be done as ref tests? There still would be dependency on font size, but using a reference test would eliminate dependency on other aspects of text field appearance.
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1809
> > +            if (style->controlTextOverflow())
> > +                return cssValuePool->createIdentifierValue(CSSValueEllipsis);
> > +            return cssValuePool->createIdentifierValue(CSSValueClip);
> 
> Coding style here is strange. We don’t use "!= 0" to compare with the constant zero, but we usually do use "!= x" when comparing with a named constant like TextOverflowClip. This was just copied and pasted from the case above, but it’s curious in both cases.
> 
> It’s mildly unpleasant that we can’t somehow use the code in CSSPrimitiveValueMappings.h to do this for us since it already has a switch statement to convert values.
> 
Forgot to comment on this-- I will look into this and see if there's a way to avoid it.

[Jon Lee]

(In reply to comment #10)
> > It’s mildly unpleasant that we can’t somehow use the code in CSSPrimitiveValueMappings.h to do this for us since it already has a switch statement to convert values.

Offline I spoke with Andreas, who told me they might be doing other optimizations to make this code moot, but I've filed a bug, since this is a pattern pervasive in that file: bug 76305.

[Jon Lee]

Created attachment 122513 [details]
Patch

[Jon Lee]

Comment on attachment 122513 [details]
Patch

Converted some tests to ref tests, added rtl cases, and got rid of some unnecessary cases. Moved all of the WebCore code to RenderTextControlSingleLine. The issue cited in CSSComputedStyleDeclaration has been punted to bug 73605.

[Jon Lee]

Created attachment 122516 [details]
Same patch as before, rebased on ToT for EWS

[Dave Hyatt]

I disagree with this patch and believe we should just made text-overflow work this way instead of introducing a new CSS property.

See my response to webkit-dev.

[Jon Lee]

Switch to using text-overflow instead of custom property, based on discussion in webkit-dev and www-style lists.

[Jon Lee]

Created attachment 123194 [details]
Patch

[Jon Lee]

Get rid of custom -webkit-control-text-overflow attribute. Moved tests from webkit-control-text... to text...

Comment on attachment 123194 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add vendor-specific CSS attribute that allows placeholder and value text to show an ellipsis when not focused

Not true anymore

[Jon Lee]

Comment on attachment 123194 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Add vendor-specific CSS attribute that allows placeholder and value text to show an ellipsis when not focused
> 
> Not true anymore

doh! i'll retitle.

[Jon Lee]

Committed r105451: http://trac.webkit.org/changeset/105451