Closed Bug 1224216 Opened 6 years ago Closed 5 years ago

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

Categories

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

45 Branch
defect
Not set
normal

Tracking

(firefox45 affected, fennec46+)

RESOLVED FIXED
Tracking Status
firefox45 --- affected
fennec 46+ ---

People

(Reporter: justdave, Assigned: capella)

References

Details

Attachments

(2 files)

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.
Attachment #8686614 - Attachment mime type: text/plain → text/html
Attachment #8686614 - Attachment filename: file_1224216.txt → file_1224216.html
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|.
Duplicate of this bug: 1226873
tracking-fennec: --- → ?
tracking-fennec: ? → 46+
Capella, are you interested in working on a fix for this?
Flags: needinfo?(markcapella)
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
Flags: needinfo?(markcapella)
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.
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.
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
Attachment #8727293 - Flags: feedback?(margaret.leibovic)
filed: Bug 1254096 - nsIDocument::CaretPositionFromPoint() could return anonymous node/offset for <input> type=number
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.
Attachment #8727293 - Flags: feedback?(margaret.leibovic) → feedback?(bugmail.mozilla)
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).
Attachment #8727293 - Flags: feedback?(bugmail.mozilla)
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?
Flags: needinfo?(mounir)
IsSingleLineTextControl() doesn't include NUMBER on purpose, as you can see, there is a `IsTextOrNumberControl` in nsIFormControl.
Flags: needinfo?(mounir)
Comment on attachment 8727293 [details] [diff] [review]
bug1224216_PATCH_01.diff

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

r+ based on comment above.
Attachment #8727293 - Flags: review+
Ok, thanks. I agree this patch is fine in that case.
Depends on: 1254096
Try push - PATCH_01 for browser getFocusedInput() correction only
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e3dbe4ee123
If you this is low risk, we should uplift it.
Assignee: nobody → markcapella
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.
Whiteboard: keep open
(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.
Flags: needinfo?(markcapella)
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(markcapella)
Resolution: --- → FIXED
Summary: copy/paste doesn't work in type=number text fields. → copy/paste doesn't work in type=number text fields, browser ignores focusable during selection
Whiteboard: keep open
Blocks: 1255819
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.