665528 - Allow placeholders for <input type=number>

Status: RESOLVED FIXED

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

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 2

[Mounir Lamouri (: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.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> IsSingleLineTextControl() should return false

Ah, I see. Yeah, okay lets do the forwarding thing.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 5

[Mounir Lamouri (: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.

Comment 6

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a69852984c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4a69852984c