Closed Bug 1003741 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- verified

People

(Reporter: yelena.dikareva, Assigned: Agi)

References

()

Details

(Keywords: testcase)

Attachments

(2 files, 2 obsolete files)

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?
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".
Blocks: 344616
Component: Untriaged → DOM: Core & HTML
Flags: needinfo?(jwatt)
Keywords: testcase
Product: Firefox → Core
Summary: Error when I try to get input type "number" selectionStart in FF 29 → Error (NS_ERROR_FAILURE) when I try to get input type "number" selectionStart in FF 29
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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!
Assignee: nobody → agi.novanta
Status: NEW → ASSIGNED
Attachment #8417138 - Flags: review?(bzbarsky)
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: 29 Branch → Trunk
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.
Attachment #8417138 - Flags: review?(bzbarsky)
Attachment #8417138 - Flags: review-
Attachment #8417138 - Flags: feedback+
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.
Attachment #8417138 - Attachment is obsolete: true
Attachment #8417155 - Flags: review?(ehsan)
Attached image FF input.png
Sample of selection in input with type "number"
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?
Can GetTextFieldFrame really return null?
(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?
Flags: needinfo?(agi.novanta)
(In reply to Boris Zbarsky [:bz] from comment #9)
> Can GetTextFieldFrame really return null?

I don't think so.
See Also: → 1005593
(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?
Flags: needinfo?(agi.novanta)
(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 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.
Attachment #8417155 - Flags: review?(ehsan) → review-
Thanks for your patient Ehsan! I really appreciate it.

This should be it. Thanks.
Attachment #8417155 - Attachment is obsolete: true
Attachment #8417743 - Flags: review?(ehsan)
Comment on attachment 8417743 [details] [diff] [review]
nsNumberControlFrame implements nsITextControlFrame

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

Awesome!  Thanks a lot for your patch!
Attachment #8417743 - Flags: review?(ehsan) → review+
Ok the Try is green. Time to land! Than you!
Whiteboard: checkin-needed
Flags: needinfo?(jwatt)
https://hg.mozilla.org/mozilla-central/rev/146061b195fe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
The cursor is still reset after selecting the number in the testcase. Is this expected?
32.0a1 (2014-06-02), win 7 x64
Flags: needinfo?(agi.novanta)
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).
Flags: needinfo?(agi.novanta)
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.
(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? :-)
Flags: needinfo?(paul.silaghi)
Ehsan, is this safely upliftable? Might make sense to improve the version of input type="number" we ship on the ESR-branch-to-be...
Flags: needinfo?(ehsan)
(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 ?
Flags: needinfo?(paul.silaghi) → needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Ty.
Verified fixed 32.0a1 (2014-06-05), win 7 x64
Status: RESOLVED → VERIFIED
(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.
Flags: needinfo?(ehsan)
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
Attachment #8417743 - Flags: approval-mozilla-aurora?
Attachment #8417743 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified that the patch applies cleanly.
Keywords: checkin-needed
Keywords: verifyme
This has an automated test, no point in verifying it manually.
Keywords: verifyme
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
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?
Depends on: 1293570
You need to log in before you can comment on or make changes to this bug.