1224216 - copy/paste doesn't work in type=number text fields, browser ignores focusable during selection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Dave Miller [:justdave]]

Attachment: type=number text field test case

STR:
1) Enable 2FA on your Bugzilla account
2) Try to log into Bugzilla from the device that contains your 2FA token
3) get prompted for 2FA token
4) switch to Google Authenticator app
5) copy the token
6) switch back to Nightly
7) attempt to paste token

Actual results:
1) Cannot paste, the UI never shows up

Expected results:
1) I can paste the token.

Minimized testcase attached.

Comment 1

[Mark Capella [:capella]]

Ah, mmmm, surprised this hasn't been noticed before. It looks like a bug, here and here, in our current text SelectionHandler [0]. Basically we're disallowing text selection processing for numeric-only <input> elements.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=1be077af56e3&mark=985-985,991-991#983

This may be worth quick-fixing, though it isn't an issue for users of the Nightly/Experimental |Gecko SelectionCarets|, nor in the even-more-experimental / in-development |Gecko AccessibleCarets|.

Comment 3

[:Margaret Leibovic]

Capella, are you interested in working on a fix for this?

Comment 4

[Mark Capella [:capella]]

Sure! But unless I'm missing something, my comment 1 is still valid, and this isn't an issue any longer with the new AccessibleCarets/ActionbarHandler in nightly using the simple testcase that's attached.

In release v44.0.2 I see a failure to paste (no UI, etc) ... in nightly the solution is:

) Longpress into empty editable to expose orange AccessibleCaret and TextSelection ActionBar
) press clipboard-icon (PASTE) as provided in ActionBar

Comment 5

[:Margaret Leibovic]

I should have read the bug comments here more closely!

If this works with the new text selection carets, I'm less concerned about this. If it would be a simple fix to get a patch for 46, great, but if not, I don't think we should worry about this. We want to try to ship the new text selection carets in 47.

Comment 6

[Mark Capella [:capella]]

Oic! I looked a little last night towards fixing this for 46, but got sidetracked when I spotted and filed related:

Bug 1253534 - Suspicious code with probably reversed parms in call to IsSingleLineTextControl(bool, uint32_t)

Will see if I can move this up over the weekend.

Comment 7

[Mark Capella [:capella]]

Attachment: bug1224216_PATCH_01.diff

tl;dr; "Scratch Monkey"   :-/

<input>'s of type=text and type=number both contain anonymous internal content, but differ, in that for type=number we support possible up/down arrow controls.

For our usual type=text:
  <input>
     <DIV> - anonymous
        #text - anonymous leaf

For type=number:
  <input>
     <DIV> - anonymous
        <input> - anonymous
           <DIV> - anonymous
              #text - anonymous leaf
        <DIV> - anonymous
           <DIV> - anonymous
           <DIV> - anonymous


When focused, the DOM.activeElement always references the top-most (non-anonymous) <input>.

Interestingly, Services.focus.focusedElement references:
   the outer non-anonymous <input> for type=text, but
   the inner anonymous <input> for type=number

Importantly, The <input> bearing the <nsIEditor> follows the one returned by Services.


This has implications in browser.getFocusedInput() [0], which always starts it's search with the |node.activeElement|, but will never recognize a focused numeric, as the |node.mozIsTextField(false)| will fail. PATCH_01 will correct this, though consumers of the method that expect to find the |node.editor| will now have issues.

The implication for those consumers, like SelectionHandler.js, is that it needs to then use the inner <input> as a reference to find the |node.editor|. I considered adding a simple hack to SelectionHandlers' two main entry points startSelection() and attachCaret() that would substitute the provided HTMLInputElement with the inner anonymous nsIEditor-bearing one, knowing the whole module goes away in the very near future.

This train of thought almost works, but dies a horrible death for the final reason that nsIDocument::CaretPositionFromPoint, used by SelectionHandler._moveSelection() [1] to 
   (a) prevent handle-drag outside of editables, and 
   (b) complete caret positioning
returns null if the requested point is centered in an <input> type=number here [2], for the same issue we have in browser.getFocusedInput()... |node.mozIsTextField(false)| will fail on the outer <input>.

And there goes hope of caret positioning of active selections.

This is where I left this particular bug. I'll file a followup to see if we can do anything about the fail in nsIDocument::CaretPositionFromPoint, but doubt we're interested in moving ahead here, given a reasonable chance that it becomes moot as an issue in the following release.


FWIW, With ActionBarHandler and the new AccessibleCaretManager, there is no inherent problem, as the input reference element we're provided is always the correct one.


[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=46210f3ae078&mark=1516-1516,1522-1522#1507
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=b8449a2d5fc1&mark=990-990,999-999#984
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=deadb414ee23&mark=10771-10772,10816-10818#10770

Comment 8

[Mark Capella [:capella]]

filed: Bug 1254096 - nsIDocument::CaretPositionFromPoint() could return anonymous node/offset for <input> type=number

Comment 9

[:Margaret Leibovic]

Comment on attachment 8727293 [details] [diff] [review]
bug1224216_PATCH_01.diff

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

I don't feel qualified to review this, kats would be a better reviewer.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8727293 [details] [diff] [review]
bug1224216_PATCH_01.diff

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

It seems like a more correct fix would be to push this down a few more levels, and include NUMBER as one of the valid single-line text controls at http://mxr.mozilla.org/mozilla-central/source/dom/html/nsIFormControl.h?rev=a46c9fc47597#259

That it is missing from that list seems like an oversight, and fixing that should make mozIsTextField return true for it which would make the browser.js change unnecessary. You'd have to look at the owners for that code though and check with them to make sure it's not intentional. (If it is, it should be commented as such).

Comment 11

[Mark Capella [:capella]]

Ah, good thought. I wonder if mounir can provide context there? Perhaps it involves the EditorState(), or maybe more like it's not a "singlelinetextcontrol" with the embedding of the up/down arrow controls?

Comment 12

[Mounir Lamouri (:mounir)]

IsSingleLineTextControl() doesn't include NUMBER on purpose, as you can see, there is a `IsTextOrNumberControl` in nsIFormControl.

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 8727293 [details] [diff] [review]
bug1224216_PATCH_01.diff

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

r+ based on comment above.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Ok, thanks. I agree this patch is fine in that case.

Comment 15

[Mark Capella [:capella]]

Try push - PATCH_01 for browser getFocusedInput() correction only
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e3dbe4ee123

Comment 16

[:Margaret Leibovic]

If you this is low risk, we should uplift it.

Comment 17

[Mark Capella [:capella]]

This patch updates one piece of the issue with the bug in browser. Bug 1254096 will be required also. 

Then (I need to loop back to my patch and re-verify) there's one last not-terribly difficult piece to change in SelectionHandler itself to ensure consistent use of the correct anonymous <input> element.

I've not looped back and checked yet, as while I'm sure these first two pre-req's are correct and should be completed in either case, I'm still not sure how the timing of the final patch and the juggling of three uplifts will appeal to you.

With AccessibleCarets going back a release, I can see uplifting these early as realistic, but will let you decide.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/23a464848044

Comment 19

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #17)
> This patch updates one piece of the issue with the bug in browser. Bug
> 1254096 will be required also. 
> 
> Then (I need to loop back to my patch and re-verify) there's one last
> not-terribly difficult piece to change in SelectionHandler itself to ensure
> consistent use of the correct anonymous <input> element.
> 
> I've not looped back and checked yet, as while I'm sure these first two
> pre-req's are correct and should be completed in either case, I'm still not
> sure how the timing of the final patch and the juggling of three uplifts
> will appeal to you.
> 
> With AccessibleCarets going back a release, I can see uplifting these early
> as realistic, but will let you decide.

Thanks for the synopsis. Given that the old selection handles aren't enabled on Nightly, I don't see us getting much testing value there, so we should uplift to Aurora for testing as soon as we think is reasonable and let this bake there before deciding to uplift to beta.

Why did you mark this bug as "leave open"? If there isn't another patch that's going to land soon, I'd prefer to handle any remaining issues in new bugs.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23a464848044

Comment 21

[Mark Capella [:capella]]

Ok, good thought on baking in aurora. I left this marked "keep open" for the final change to SelectionHandler to use the proper <input> field consistently, forgetting your preference in this area.

I'll adjust for that.