Closed Bug 665528 Opened 8 years ago Closed 6 years ago

Allow placeholders for <input type=number>

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: mounir, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → jwatt
Attached patch patch (obsolete) — Splinter Review
Originally I was going to put in some machinery to forward the placeholder attribute's value to the anonymous text field grandchild. That doesn't really work though since we end up invoking placeholder code for both the <input>, and the anonymous text field. There are also a bunch of other issues that I hadn't anticipated regarding selection etc. that require a deeper change. This patch  changes things deeper in the code to make sure that we use the anonymous text field's editor state (and editor) when code wants to manipulate the <input>'s editor (e.g. to set the placeholder).

Right now I can't properly test this because I'm having a considerable headache getting nested anonymous content constructed properly (bug 917386), but I'll add some reftests once I've passed that issue.
Attachment #806617 - Flags: review?
Attachment #806617 - Flags: review? → review?(mounir)
Comment on attachment 806617 [details] [diff] [review]
patch

Review of attachment 806617 [details] [diff] [review]:
-----------------------------------------------------------------

NS_FORM_INPUT_NUMBER should not have an editor, IsSingleLineTextControl() should return false so GetEditorState() should return nullptr. It is not clear why this patch would do anything useful then.

I think you should do what we discussed: the frame should listen to @placeholder changing on the content and update the anonymous text field accordingly.
Attachment #806617 - Flags: review?(mounir) → review-
(In reply to Mounir Lamouri (:mounir) from comment #2)
> IsSingleLineTextControl() should return false

Ah, I see. Yeah, okay lets do the forwarding thing.
Attached patch patchSplinter Review
The XXXjwatt notes are just there to flag the boolean values that I'm passing for your attention. I'll remove them for checkin once you confirm you agree with the values.
Attachment #806617 - Attachment is obsolete: true
Attachment #809929 - Flags: review?(mounir)
Comment on attachment 809929 [details] [diff] [review]
patch

Review of attachment 809929 [details] [diff] [review]:
-----------------------------------------------------------------

I see that you have been enjoying the aNotify booleans :) The values you set looked right to me FWIW.

r=me with the comments applied.

::: layout/forms/nsNumberControlFrame.cpp
@@ +145,5 @@
> +                                       int32_t  aModType)
> +{
> +  if (aNameSpaceID == kNameSpaceID_None) {
> +    if (aAttribute == nsGkAtoms::placeholder) {
> +      if (mTextField) { // display:none?

Have you been able to make that happen? If <input type=number> is display:none, |this| (as the frame) will not exist. If the text field is display none, the text field content will exist but not its frame.

Unless you saw that happen, could you assume it can't happen and add an ASSERT?

@@ +260,5 @@
> +  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::placeholder)) {
> +    nsAutoString value;
> +    mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, value);
> +    mTextField->SetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, value, false); // XXXjwatt
> +  }

You can as well do:

nsAutoString placeholder;
if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, placeholder)) {
  mTextField->SetAttr(kNameSpaceId_None, nsGkAtoms::placeholder, placeholder, false);
}

(::GetAttr() returns false if the attribute is not set)

Also, leave an empty line between the type SetAttr() and this block.

::: layout/forms/nsNumberControlFrame.h
@@ +35,5 @@
>  
> +  NS_IMETHOD AttributeChanged(int32_t  aNameSpaceID,
> +                              nsIAtom* aAttribute,
> +                              int32_t  aModType) MOZ_OVERRIDE;
> +  

nit: remove this empty line with trailing whitespace.
Attachment #809929 - Flags: review?(mounir) → review+
https://hg.mozilla.org/mozilla-central/rev/e4a69852984c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.