Code cleanup - remove unneeded onFocus() call in Text Selection




Firefox for Android
Text Selection
5 years ago
4 years ago


(Reporter: capella, Unassigned)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: [lang=js][mentor=capella][good first bug])


(1 attachment)

Comment hidden (empty)

Comment 1

5 years ago
Spun off from bug 912983 comment 5 as a unique patch ...

Complete the following

::: mobile/android/chrome/content/SelectionHandler.js
@@ +304,5 @@
>    _initTargetInfo: function sh_initTargetInfo(aElement) {
>      this._targetElement = aElement;
> +
> +    if (this._targetElement instanceof Ci.nsIDOMNSEditableElement) {
> +      this._targetElement.focus();

|but why do we need to set focus like this? Shouldn't the element already have focus if we're preparing to show the caret?|


5 years ago
Whiteboard: [lang=java][mentor=capella][good-first-bug] → [lang=java][mentor=capella][good first bug]
Created attachment 8348976 [details] [diff] [review]

I have removed the unneeded call to focus on the element. Is there anything else I need to do?

Comment 3

5 years ago
Well, I'm curious if you tested it at all? Are you building locally? That looks like what we had in mind  :)


5 years ago
Whiteboard: [lang=java][mentor=capella][good first bug] → [lang=js][mentor=capella][good first bug]
(In reply to Mark Capella [:capella] from comment #3)
> Well, I'm curious if you tested it at all? Are you building locally? That
> looks like what we had in mind  :)

I was able to build and install it on my tablet, but I am unsure of how I would test this. What would you recommend be the best way to test?

Comment 5

5 years ago
Well, that bit of code is in SelectionHandler, which allows us to provide the orange caret or orange end-to-end handles you see when you tap into an editable field, or long-tap a text word to select it.

That particular piece was provided with the thought that if we're selecting text in an editable field, then of course the next thing the user might do is Cut/Paste etc.These are options that require the targetElement to be focused for input. So we just ensured it is focused, to be sure at that point in time.

In hindsight, we're thinking we can't hit that code, if the element isn't focused in the first place. This is just working too hard, and should be eliminated unless we bump into a *duh* moment and remember why we might have needed that thing.

I'd say to test, just remove the code as per your patch, build and install the .apk on your device, then perform various test selection events you might normally do, highlight test on the screen, move the handles around, hightlight text in both editable and non-editable fields, tap into an editable field, see if you get the cursor caret to display, move the caret around, type characters and see what happens at the point where the curosr/caret is displayed as you do, and etc  :-)

We're not expecting to see problems. So "proving" you didn't isn't realistic. But if you spot something during testing, we need to explore it and react appropriately.

Try it, see if you break anything :-)

The we can have you request the actual review from the module owners, and after fixing anything they find, I'll help you move things into production / nightly to join the release process and you're done!

And you win another bug to work on  ;-)


5 years ago
After some initial testing using this jsfiddle there were some problems I have noticed.

On a long-tap of an editable field element, the editable field does not become focused and the keyboard is not displayed as well as the selected text does not become highlighted between the orange end-to-end handles.

Another behavior I have noticed with the orange end-to-end handle is when dragging the left handle or right handle around, there is a chance for the currently selected text to be deleted from the editable field. The same behavior happens in the current version of Firefox for Android 26.0.1.

I'll continue to do some additional testing.

Comment 7

5 years ago
fyi Re:| deleteing text while dragging handles|
I'm seeing that also in testing for bug 927882, so that's not due to your change.

re: |long-tap of an editable field element, the editable field does not become focused|

There's a recent bug regarding editable fields not being recognized as focusable so no caret/selection handles are started ... bug see bug 951716 comment #5

So, also, most likely not you, but let us know what you find?

Comment 8

4 years ago
When you think you've shook out this patch enough, you can submit if for review to margaret. With her approval, we can get your change moved ahead.

Comment 9

4 years ago
well, I'm glad we waited ... this specific bit of code needs to stay put and be expanded on for use in bug 969229 which I'm landing today ...
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.