[webkit-reviews] review denied: [Bug 76118] Add vendor-specific CSS attribute that allows placeholder and value text to show an ellipsis when not focused : [Attachment 122381] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jon Lee <jonlee at apple.com>'s request
for review:
Bug 76118: Add vendor-specific CSS attribute that allows placeholder and value
text to show an ellipsis when not focused
https://bugs.webkit.org/show_bug.cgi?id=76118

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.