Bug 61095 – REGRESSION (r63854-63958): placeholder not shown for number inputs

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Scott Kyle]

<input type="number" placeholder="pin code">

The placeholder will not show up.

[Paul Knight]

<rdar://problem/9464120>

[Joseph Pecoraro]

HTMLFormControl calls supportsPlaceholder() which looks at the InputType.

  InputType:isTextType
    • FALSE by default
    • TRUE for BaseTextInputType <input type="text"> and what else?
    • FALSE for NumberInputType because it does not override it, it subclasses
    from TextFieldInputType which states:
    // The class represents types of which UI contain text fields.
    // It supports not only the types for BaseTextInputType but also type=number.
  
So if we wanted, we could probably just move the override to TextFieldInputType
instead of BaseTextInputType.

[Joseph Pecoraro]

Well, that is probably used in many places. Maybe add a "InputType::supportsPlaceholder"
which is specific for this.

[Daniel Bates]

From bisecting the Mac nightly builds, the regression appeared in (63854,  r63958]: <http://trac.webkit.org/log/trunk/?rev=63958&stop_rev=63855&limit=103>.

[Joseph Pecoraro]

Probably:

-    virtual bool supportsPlaceholder() const { return isTextField(); }
+    virtual bool supportsMaxLength() const { return isTextType(); }
+    bool isTextType() const;
+
+    virtual bool supportsPlaceholder() const { return isTextType() || inputType() == ISINDEX; }

And isTextType() does not include NUMBER.
http://trac.webkit.org/changeset/63942/trunk

[Daniel Bates]

As per 4.10.7.1.13 of the HTML5 draft spec. <http://dev.w3.org/html5/spec/Overview.html#number-state>: "The following content attributes must not be specified and do not apply to [<input type="number">: accept, alt, ... placeholder, ..., and width." So, placeholder is not a valid attribute for <input type="number">.

[Theresa O'Connor]

This is a bug in the spec. I've filed a spec bug to have this fixed: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12974

[Theresa O'Connor]

The HTML spec has been updated to allow placeholder="" on <input type=number>: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12974

[Sam Weinig]

I have a patch for this, but the test is currently blocked on https://bugs.webkit.org/show_bug.cgi?id=62923.  I will try and work around that.

[Sam Weinig]

Created attachment 97683 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97683 [details]
Patch

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

New failing tests:
fast/forms/number-placeholder.html

[Joseph Pecoraro]

Comment on attachment 97683 [details]
Patch

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

> Source/WebCore/html/NumberInputType.h:73
> +    virtual bool supportsPlaceholder();

Nit: These functions could all be const. I didn't follow the webkit-dev discussion about constness too closely, but since these don't even access state it should be good.

> LayoutTests/fast/forms/number-placeholder.html:24
> +        shouldBe("window.getComputedStyle(document.getElementById('input'), '-webkit-input-placeholder').getPropertyValue('color')", "'rgb(255, 0, 0)'");

This appears to work because getComputedStyle only returns valid styles if a placeholder was rendered (a Renderer / style calculation was done for the pseudoclass at some point). However, as a web developer I would think that checking for the computed style of the placeholder would work even if a placeholder didn't exist. If that were the case, this test would not actually check if the placeholder existed or not. In fact, if you delete the placeholder after getting its computed style, the computed style check would still pass even though there isn't a placeholder.

Is there a less brittle way to test this? Hopefully one without resorting to pixel tests / dump render tree output?

[WebKit Review Bot]

Comment on attachment 97683 [details]
Patch

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

New failing tests:
fast/forms/number-placeholder.html

[WebKit Review Bot]

Created attachment 97686 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Sam Weinig]

(In reply to comment #12)
> (From update of attachment 97683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97683&action=review
> 
> > Source/WebCore/html/NumberInputType.h:73
> > +    virtual bool supportsPlaceholder();
> 
> Nit: These functions could all be const. I didn't follow the webkit-dev discussion about constness too closely, but since these don't even access state it should be good.

Ok. I changed them to be const.

> > LayoutTests/fast/forms/number-placeholder.html:24
> > +        shouldBe("window.getComputedStyle(document.getElementById('input'), '-webkit-input-placeholder').getPropertyValue('color')", "'rgb(255, 0, 0)'");
> 
> This appears to work because getComputedStyle only returns valid styles if a placeholder was rendered (a Renderer / style calculation was done for the pseudoclass at some point). However, as a web developer I would think that checking for the computed style of the placeholder would work even if a placeholder didn't exist. If that were the case, this test would not actually check if the placeholder existed or not. In fact, if you delete the placeholder after getting its computed style, the computed style check would still pass even though there isn't a placeholder.
> 
> Is there a less brittle way to test this? Hopefully one without resorting to pixel tests / dump render tree output?

I am not really sure about the semantics of pseudo elements in getComputedStyle, but your argument is compelling.  Regardless, the test as written does not seem to pass 100% of the time (setTimeout too fragile) and a render tree dump does not seem to include the placeholder, so I am going to land this as is (minus the test for now) and land a better test when I can.

[Sam Weinig]

Fixed in r89194.

[Sam Weinig]

Filed https://bugs.webkit.org/show_bug.cgi?id=62934 to track landing a test.

[Joseph Pecoraro]

> I am not really sure about the semantics of pseudo elements in getComputedStyle,
> but your argument is compelling.  Regardless, the test as written does not seem to
> pass 100% of the time (setTimeout too fragile) and a render tree dump does not
> seem to include the placeholder, so I am going to land this as is (minus the test for
> now) and land a better test when I can.

In the past pixel tests have been used for checking placeholder. For example:
fast/forms/input-placeholder-visibility*