Closed Bug 950464 Opened 6 years ago Closed 6 years ago

Select-All button in text-selection action-bar copies text instead of selecting text

Categories

(Firefox for Android :: Text Selection, defect)

29 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified
fennec 28+ ---

People

(Reporter: aaronmt, Assigned: wesj)

References

()

Details

(Keywords: dataloss, reproducible, testcase)

Attachments

(1 file, 1 obsolete file)

See test-case. Invoke focus into the field, long-tap on text and opt to select-all. Selecting-all will select-all text. Now tap anywhere in the range to de-select all text and tap the select-all button that remains in the action-bar. Instead of selecting all text, the entire contents of the field are copied and wiped clear.

--
Nightly (12/15)
LG Nexus 5 (Android 4.4.2)
It might be cutting the entire text but the toast mentions 'Text copied to clipboard'.
Related example: (?) 

https://phonebook.mozilla.org, write something in the main search field, long-tap on it and opt to select-all. Now, de-select the text, tap the field and tap the select-all button again in the action-bar. Nothing happens this time around.
Attached patch Patch (obsolete) — Splinter Review
So in this case, the caret is showing when you hit "select all". That causes us to call startSelection in the hopes of setting things up for selection (setting the right mode, selecting, and adding handles), after which we'd normally call select all. In this case, we don't have a point to refer to though. I tried creating a fake point, which works if the text box is small and has no padding.

If the textbox DOES have padding, we wind up creating a selection that's some distance from the fake point we created, and the SelectionHandler will reject it.

This just removes those attempts to create a fake selection point and assumes if you don't pass one in, that we'd rather just select all. That also simplifies the selectAll code a little bit.
Attachment #8348373 - Flags: review?(markcapella)
Attachment #8348373 - Flags: review?(margaret.leibovic)
Oh we should talk in irc. I've got a series of patches to SelectionHandler in the works that addresses this situation as a step towards some other work I'm doing in the module.

See bug 947284 for the latest patches and margarets review, though I'm planning to post a set of revisions later tonight.

I think we're both trying to allow SelectionHandler.startSelection() to function without any start point provided, and additionally to modify the selection method used as a result.

I'm also trying to work into it where we toss out the methods' proximity checking function. I don't see a valid reason for it.

I have an eye after that to expanding the textSelection method used in startSelection() to differ as appropriate in some conditions based on element type and if/not the startPoint was provided.

Provides flexibility for #text / TextArea / etc. to use SELECT_WORDNOSPACE, SELECT_PARAGRAPH, or default _getSelectionController().selectAll(); in the right places.
Attachment #8348373 - Flags: review?(markcapella)
Comment on attachment 8348373 [details] [diff] [review]
Patch

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

I think this is looking good, I just have some comments to try to make this logic easier to follow. There are definitely some similar changes to what capella is doing in bug 947284, but I think that we should go ahead and try to land something here, since we need to uplift this to aurora.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +230,5 @@
>  
>      // Clear any existing selection from the document
>      this._contentWindow.getSelection().removeAllRanges();
>  
> +    let hasPoint = aX != undefined && aY != undefined;

Let's take a step back here... (modulo bug 950785) we only call startSelection from two places:

1) starting a selection on a word, where we pass coordinates: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2129

2) select all, where right now it looks like we pass coordinate, but we actually don't: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#494

Instead of creating this `hasPoint` flag, I think it would be better to create a `selectAll` flag, since that's really the different behavior we're trying to define.

@@ +283,5 @@
> +      // Do not select text far away from where the user clicked
> +      if (distance > maxSelectionDistance) {
> +        this._closeSelection();
> +        return false;
> +      }

startSelection is too damn big! Since you're putting all this logic inside an if statement anyway, let's take this opportunity to move it into a helper function. The logic here could look something like:

// Do not select text far away from where the user clicked
if (!selectAll && this._selectionTooFarFromClick(aX, aY, scroll, positions)) {
  this._closeSelection();
  return false;
}

Also, I know you didn't write this code, and I actually reviewed it in bug 772280, but I feel like it would be better to do this check before we start the selection, rather than bailing after starting (in fact, I wonder if the user sees a selection flash in this case). To do this we'd need to change around the logic, since we wouldn't be able to use positions to do the calculation, but maybe we could do something just with aElement. However, let's file a follow-up bug for this, since this sounds risky and regression-prone.

@@ +488,5 @@
>    isSelectionActive: function sh_isSelectionActive() {
>      return (this._activeType == this.TYPE_SELECTION);
>    },
>  
>    selectAll: function sh_selectAll(aElement, aX, aY) {

I think you should get rid of these aX/aY parameters. We never actually pass in valid values for them, so they just cause confusion.

To further reduce confusion, you could also get rid of this unused consumer:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6235

@@ +496,5 @@
> +      let selectionController = this._getSelectionController();
> +      selectionController.selectAll();
> +      this._updateCacheForSelection();
> +      this._positionHandles();
> +    }

In bug 947284, capella noticed this and was starting to work on addressing it by just replacing selectAll with a call to startSelection with undefined aX/aY.

However, I suppose this may look janky if we already have an active selection, which we just want to extend to select all. And now that I'm thinking about it more, I do like the abstraction of having a "selectAll" API, so I don't know that we should actually change that.
Attachment #8348373 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Patch v2Splinter Review
Lets try this
Attachment #8348373 - Attachment is obsolete: true
Attachment #8348969 - Flags: review?(margaret.leibovic)
Comment on attachment 8348969 [details] [diff] [review]
Patch v2

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

I didn't even think of this as a solution, but I like it! Options object FTW.

Update the inline documentation before landing!

::: mobile/android/chrome/content/SelectionHandler.js
@@ +222,5 @@
>     * Called from browser.js when the user long taps on text or chooses
>     * the "Select Word" context menu item. Initializes SelectionHandler,
>     * starts a selection, and positions the text selection handles.
>     *
>     * @param aX, aY tap location in client coordinates.

You should update this whole block of documentation.

@@ +233,5 @@
>  
>      // Clear any existing selection from the document
>      this._contentWindow.getSelection().removeAllRanges();
>  
> +    if (aOptions.mode != this.SELECT_ALL) {

I think it would be more straightforward to check if the mode is SELECT_ALL, then select all in that case.

You could even do:

if (mode == SELECT_ALL) {
  // select all
} else if (mode == SELECT_AT_POINT) {
  // select at point
} else {
  Cu.reportError("Unknown selection mode!");
  this._deactivate();
  return;
}

Or just check at the beginning of the function that we have a valid mode to avoid this _deactivate business.

@@ +264,2 @@
>  
> +    if (!aOptions.mode == this.SELECT_ALL && !this.selectionNearClick(scroll.X + aOptions.x,

Since you explicitly made SELECT_AT_POINT a mode, you could just check for that here.

@@ +274,4 @@
>      return true;
>    },
>  
> +  selectionNearClick: function(aX, aY, aPositions) {

Add some docs above this to explain what it's for. Also, I like prefixing helper methods with an underscore to imply their role.

::: mobile/android/chrome/content/browser.js
@@ +6242,1 @@
>    },

This selectWord function is also unused, so you could just get rid of it (even though I see a patch got started in bug 950785).
Attachment #8348969 - Flags: review?(margaret.leibovic) → review+
Duplicate of this bug: 945976
https://hg.mozilla.org/mozilla-central/rev/336da7d0b51a
https://hg.mozilla.org/mozilla-central/rev/12d601c7f0ef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
tracking-fennec: ? → 28+
Comment on attachment 8348969 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 768667
User impact if declined: Select all doesn't work
Testing completed (on m-c, etc.): landed on mc a day ago
Risk to taking this patch (and alternatives if risky): low risk. Fixup for a new feature that makes some new things possible.
String or IDL/UUID changes made by this patch: None.
Attachment #8348969 - Flags: approval-mozilla-aurora?
Attachment #8348969 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Build: Nightly 29.0a1 2014-01-10 and Aurora 28.0a2 2014-01-10
Device: Galaxy Nexus / HTC Desire HD
OS: Android 4.3/2.3.1

Using steps mentioned in comment #0 i am no longer able to see the wrong behavior. Tapping the select all will select all text each time. 

Setting this as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.