Bug 61095 - REGRESSION (r63854-63958): placeholder not shown for number inputs
Summary: REGRESSION (r63854-63958): placeholder not shown for number inputs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-05-18 14:51 PDT by Scott Kyle
Modified: 2011-06-20 10:17 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.20 KB, patch)
2011-06-17 19:09 PDT, Sam Weinig
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.25 MB, application/zip)
2011-06-17 20:26 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Kyle 2011-05-18 14:51:44 PDT
<input type="number" placeholder="pin code">

The placeholder will not show up.
Comment 1 Paul Knight 2011-05-18 15:00:11 PDT
<rdar://problem/9464120>
Comment 2 Joseph Pecoraro 2011-05-18 15:05:51 PDT
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.
Comment 3 Joseph Pecoraro 2011-05-18 15:07:03 PDT
Well, that is probably used in many places. Maybe add a "InputType::supportsPlaceholder"
which is specific for this.
Comment 4 Daniel Bates 2011-05-18 15:36:31 PDT
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>.
Comment 5 Joseph Pecoraro 2011-05-18 15:45:01 PDT
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
Comment 6 Daniel Bates 2011-05-18 15:54:01 PDT
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">.
Comment 7 Theresa O'Connor 2011-06-16 15:21:23 PDT
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
Comment 8 Theresa O'Connor 2011-06-16 15:35:59 PDT
The HTML spec has been updated to allow placeholder="" on <input type=number>: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12974
Comment 9 Sam Weinig 2011-06-17 18:52:27 PDT
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.
Comment 10 Sam Weinig 2011-06-17 19:09:00 PDT
Created attachment 97683 [details]
Patch
Comment 11 WebKit Review Bot 2011-06-17 19:34:25 PDT
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
Comment 12 Joseph Pecoraro 2011-06-17 19:57:26 PDT
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?
Comment 13 WebKit Review Bot 2011-06-17 20:26:16 PDT
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
Comment 14 WebKit Review Bot 2011-06-17 20:26:42 PDT
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
Comment 15 Sam Weinig 2011-06-18 11:50:00 PDT
(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.
Comment 16 Sam Weinig 2011-06-18 11:51:05 PDT
Fixed in r89194.
Comment 17 Sam Weinig 2011-06-18 11:52:28 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=62934 to track landing a test.
Comment 18 Joseph Pecoraro 2011-06-20 10:17:39 PDT
> 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*