1003741 - Error (NS_ERROR_FAILURE) when I try to get input type "number" selectionStart in FF 29

Status: VERIFIED FIXED

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

Description

[Dikareva Elena]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

I try to get selectionStart, selectionEnd or selectionDirection from input with type "number" in new version of FF (29.0)


Actual results:

But error has occurred, in previous versions it did not happen.


Expected results:

I know that Chrome refused to support these properties since version 33 and and also returns an error. But there is a workaround: you can change type of input to "text" then get needed properties and change type back. I really need to define these properties, is there any way to do this?

Comment 1

[:Gijs (he/him)]

Jonathan, I'm not familiar with what the spec says and if this is invalid/wontfix, or something we should try to support. Could you clarify?

As far as compatibility goes, I did notice that in the testcase, Chrome doesn't reset the cursor position when changing the input type, and we do (which makes selection* kind of useless).

(In reply to Dikareva Elena from comment #0)
> But error has occurred, in previous versions it did not happen.

Previous versions didn't actually support type="number", though... On the other hand, I guess we fell back to text boxes and that "just worked".

Comment 2

[Boris Zbarsky [:bzbarsky]]

HTMLInputElement::GetSelectionRange depends on the frame being nsITextControlFrame.  It seems to me like nsNumberControlFrame could implement that and forward all the methods to mTextField's frame...

As far as I can tell, per spec this should work.

Comment 3

[(no longer active)]

(In reply to comment #2)
> HTMLInputElement::GetSelectionRange depends on the frame being
> nsITextControlFrame.  It seems to me like nsNumberControlFrame could implement
> that and forward all the methods to mTextField's frame...

Yes, that seems like the right fix to me.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: nsNumberControlFrame implements nsITextControlFrame

Is this something like what you had in mind Boris?

I'm not really sure I understand how do_QueryFrame works, but it looks like the only way to get a nsITextControlFrame reference out of nsNumberControlFrame is the following?

+nsITextControlFrame*
+HTMLInputElement::GetTextControlFrame()
+{
+  nsITextControlFrame* textControlFrame;
+  if (mType == NS_FORM_INPUT_NUMBER) {
+    nsNumberControlFrame* numberControlFrame = do_QueryFrame(GetPrimaryFrame());
+    textControlFrame = static_cast<nsITextControlFrame*>(numberControlFrame);
+  } else {
+    textControlFrame = do_QueryFrame(GetPrimaryFrame());
+  }
+
+  return textControlFrame;
+}
+

It looks hacky to me.

I also updated some tests to be number-friendly, but we may also want to have a different test altogether instead of modifying the existing one.

Thanks!

Comment 5

[(no longer active)]

Comment on attachment 8417138 [details] [diff] [review]
nsNumberControlFrame implements nsITextControlFrame

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

Stealing because I know this code and bz is busy.  :-)

Your patch looks very good, I'd happily review a modified version of the patch according to the comments below.  Please let me know if this makes sense.  Thanks!

::: content/html/content/src/HTMLInputElement.cpp
@@ +1179,5 @@
> +{
> +  nsITextControlFrame* textControlFrame;
> +  if (mType == NS_FORM_INPUT_NUMBER) {
> +    nsNumberControlFrame* numberControlFrame = do_QueryFrame(GetPrimaryFrame());
> +    textControlFrame = static_cast<nsITextControlFrame*>(numberControlFrame);

No, you shouldn't need to do this.  Instead, add a NS_QUERYFRAME_ENTRY(nsITextControlFrame) to nsNumberControlFrame.cpp next to the other NS_QUERYFRAME_ENTRY entries there.  do_QueryFrame basically calls a virtual function on the frame class that searches through a static table of UUIDs to see if there is a match and then casts |this| to the desired pointer type and returns.

Once you do this, you can get rid of all of your modifications to this class (except for the change to SupportsSetRangeText which is still needed.)

::: layout/forms/nsNumberControlFrame.cpp
@@ +257,5 @@
> +nsITextControlFrame*
> +nsNumberControlFrame::GetTextFieldFrame()
> +{
> +  HTMLInputElement* textField = HTMLInputElement::FromContent(mTextField);
> +  return do_QueryFrame(textField->GetPrimaryFrame()); 

Nit: there are some places in your patch where you're introducing trailing whitespaces.  Please fix them.

@@ +450,5 @@
> +{
> +  nsITextControlFrame* fieldFrame = GetTextFieldFrame();
> +  if (fieldFrame) {
> +    return fieldFrame->SetSelectionRange(aSelectionStart, aSelectionEnd,
> +                                                aDirection);

Nit: indentation.

@@ +462,5 @@
> +{
> +  nsITextControlFrame* fieldFrame = GetTextFieldFrame();
> +  if (fieldFrame) {
> +    return fieldFrame->GetSelectionRange(aSelectionStart, aSelectionEnd,
> +                                                aDirection);

Here too.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: nsNumberControlFrame implements nsITextControlFrame

Thank you for taking the review Ehsan :)

Oh I totally missed NS_QUERYFRAME_ENTRY! I added some lines in my .vimrc to highlight in a bright red trailing whitespaces so I hopefully won't have this problem again.

I should have addressed everything in this patch, thank you.

Comment 7

[Dikareva Elena]

Attachment: FF input.png

Sample of selection in input with type "number"

Comment 8

[Dikareva Elena]

You can see in the screenshot (attach FF input.png) that inside input with type "number" some text selected.
How do I determine input's selectionStart (selectionEnd and selectionDirection) position?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Can GetTextFieldFrame really return null?

Comment 10

[(no longer active)]

(In reply to Dikareva Elena from comment #8)
> You can see in the screenshot (attach FF input.png) that inside input with
> type "number" some text selected.
> How do I determine input's selectionStart (selectionEnd and
> selectionDirection) position?

Giovanni, does your patch fix this?  If yes, can you please add a test for it?

Comment 11

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Can GetTextFieldFrame really return null?

I don't think so.

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #10)
> (In reply to Dikareva Elena from comment #8)
> > You can see in the screenshot (attach FF input.png) that inside input with
> > type "number" some text selected.
> > How do I determine input's selectionStart (selectionEnd and
> > selectionDirection) position?
> 
> Giovanni, does your patch fix this?  If yes, can you please add a test for
> it?

I'm not sure I understand what is to be fixed. If you mean the possibility of using selectionStart, selectionEnd and selectionDirection on a type="number" element, yes I'm, fixing this. Also I think the modification I did in the tests should be sufficient to test it

-  var SupportedTypes = ["text", "search", "url", "tel", "password", "textarea"];
+  var SupportedTypes = ["text", "search", "url", "tel", "password", "textarea", "number"];

(In reply to  Boris Zbarsky [:bz] from comment #9)
> Can GetTextFieldFrame really return null?
Looking closer it probably can't. Should I remove the checks?

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #10)
> > (In reply to Dikareva Elena from comment #8)
> > > You can see in the screenshot (attach FF input.png) that inside input with
> > > type "number" some text selected.
> > > How do I determine input's selectionStart (selectionEnd and
> > > selectionDirection) position?
> > 
> > Giovanni, does your patch fix this?  If yes, can you please add a test for
> > it?
> 
> I'm not sure I understand what is to be fixed. If you mean the possibility of
> using selectionStart, selectionEnd and selectionDirection on a type="number"
> element, yes I'm, fixing this. Also I think the modification I did in the tests
> should be sufficient to test it
> 
> -  var SupportedTypes = ["text", "search", "url", "tel", "password",
> "textarea"];
> +  var SupportedTypes = ["text", "search", "url", "tel", "password",
> "textarea", "number"];

Err, nevermind, forgot that this test also tests for the selectionStart/End stuff.

> (In reply to  Boris Zbarsky [:bz] from comment #9)
> > Can GetTextFieldFrame really return null?
> Looking closer it probably can't. Should I remove the checks?

Yes please.

Comment 14

[(no longer active)]

Comment on attachment 8417155 [details] [diff] [review]
nsNumberControlFrame implements nsITextControlFrame

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

This is very close!

::: layout/forms/nsNumberControlFrame.cpp
@@ +259,5 @@
> +nsITextControlFrame*
> +nsNumberControlFrame::GetTextFieldFrame()
> +{
> +  HTMLInputElement* textField = HTMLInputElement::FromContent(mTextField);
> +  return do_QueryFrame(textField->GetPrimaryFrame());

You can just do:

  return do_QueryFrame(GetAnonTextControl()->GetPrimaryFrame());

@@ +419,5 @@
> +NS_IMETHODIMP
> +nsNumberControlFrame::GetEditor(nsIEditor **aEditor)
> +{
> +  nsITextControlFrame* fieldFrame = GetTextFieldFrame();
> +  if (fieldFrame) {

Please get rid of the null checks as discussed.

Comment 15

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: nsNumberControlFrame implements nsITextControlFrame

Thanks for your patient Ehsan! I really appreciate it.

This should be it. Thanks.

Comment 16

[(no longer active)]

Comment on attachment 8417743 [details] [diff] [review]
nsNumberControlFrame implements nsITextControlFrame

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

Awesome!  Thanks a lot for your patch!

Comment 17

[[ex-Mozilla] Agi Sferro | :agi]

Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=d9f5b4c802eb

Comment 18

[[ex-Mozilla] Agi Sferro | :agi]

Ok the Try is green. Time to land! Than you!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/146061b195fe

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/146061b195fe

Comment 21

[Paul Silaghi, QA [:pauly]]

The cursor is still reset after selecting the number in the testcase. Is this expected?
32.0a1 (2014-06-02), win 7 x64

Comment 22

[[ex-Mozilla] Agi Sferro | :agi]

No it's not expected, but it looks like the bug you are referring to is Bug 981248. I think the cursor is resetting because javascript is changing the type from number to text making the control lose focus (and thus the selection).

Comment 23

[Paul Silaghi, QA [:pauly]]

Ok then, but what is actually fixed here?
I still don't see anything in the console when trying to select some digits in the testcase.

Comment 24

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #23)
> Ok then, but what is actually fixed here?
> I still don't see anything in the console when trying to select some digits
> in the testcase.

AFAICT, if you use this testcase: http://jsbin.com/mosux/2/edit instead, you can see console logs and things just work - the changing of the input type in the original testcase is to work around a bug in Chrome. Having it work when that workaround is applied is subject of another bug. This bug fixes the case where people try to use selectionStart/End etc. directly on an <input type="number"> without changing the type.

Can you re-verify with this new testcase? :-)

Comment 25

[:Gijs (he/him)]

Ehsan, is this safely upliftable? Might make sense to improve the version of input type="number" we ship on the ESR-branch-to-be...

Comment 26

[Paul Silaghi, QA [:pauly]]

(In reply to :Gijs Kruitbosch from comment #24)
> Can you re-verify with this new testcase? :-)
Change the input value to a 5 digits number.
Select with mouse last 2 digits in the output.
Console result: 3
Shouldn't be 2 ?

Comment 27

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #26)
> (In reply to :Gijs Kruitbosch from comment #24)
> > Can you re-verify with this new testcase? :-)
> Change the input value to a 5 digits number.
> Select with mouse last 2 digits in the output.
> Console result: 3
> Shouldn't be 2 ?

No? The cursor being before the first digit would be 0, before the second digit 1, and so forth. As the cursor is before the 4th digit, the value is 3, and that's correct.

Comment 28

[Paul Silaghi, QA [:pauly]]

Ty.
Verified fixed 32.0a1 (2014-06-05), win 7 x64

Comment 29

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #25)
> Ehsan, is this safely upliftable? Might make sense to improve the version of
> input type="number" we ship on the ESR-branch-to-be...

You mean uplifting to Aurora, right?  Then yes.

Comment 30

[[ex-Mozilla] Agi Sferro | :agi]

Comment on attachment 8417743 [details] [diff] [review]
nsNumberControlFrame implements nsITextControlFrame

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 635240
User impact if declined: @selectionStart, @selectionEnd attributes won't work on type="number" input controls, it wont be in the next Firefox ESR.
Testing completed (on m-c, etc.): on m-c since 5/6
Risk to taking this patch (and alternatives if risky): should't pose substantial risks
String or IDL/UUID changes made by this patch: none

Comment 31

[[ex-Mozilla] Agi Sferro | :agi]

Verified that the patch applies cleanly.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c3bd98786d7

Comment 33

[(no longer active)]

This has an automated test, no point in verifying it manually.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

So why did we decide to do against the spec?
I don't see number supporting selection stuff in 
https://html.spec.whatwg.org/multipage/forms.html#do-not-apply

Comment 35

[Boris Zbarsky [:bzbarsky]]

There's a longstanding spec issue at https://www.w3.org/Bugs/Public/show_bug.cgi?id=24796 afaict.

But yes, it's not clear to me whether there was a principled decision here.  What do other UAs do?