864589 - Show/hide text selection handles if a selection is programatically added/removed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[:Margaret Leibovic]

This was originally half fixed in bug 767065, but I removed that logic in bug 667243, since it didn't work well with our new caretPositionFromPoint approach.

Comment 1

[Mark Capella [:capella]]

Attachment: bug864589

This appears to solve for this and the test case in bug 767065, as well as solving bug 904288 the right way.

I have one minor concern, let's see if you notice :-)

Comment 2

[:Margaret Leibovic]

Comment on attachment 790198 [details] [diff] [review]
bug864589

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

I'm impressed that you aren't afraid to jump right into the platform side of things. This seems like a less hacky approach than what I did back in bug 767065!

However, I want Ehsan to take a look at these platform changes before I start reviewing the JS side of things :)

Comment 3

[Mark Capella [:capella]]

margaret: fyi - yah - when I started out with a11y I did some tinkering with text selection, notably here https://bugzilla.mozilla.org/show_bug.cgi?id=614585

Comment 4

[:Margaret Leibovic]

Comment on attachment 790198 [details] [diff] [review]
bug864589

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

LGTM. I'm wondering what your concern is! :)

::: mobile/android/chrome/content/SelectionHandler.js
@@ +479,1 @@
>          selection.removeAllRanges();

This is the opposite order of what was in my patch from bug 767065, but I did update it to do this before removing the selection listener from the code:
http://hg.mozilla.org/mozilla-central/annotate/c8d7e2709f01/mobile/android/chrome/content/SelectionHandler.js#l402

So this is correct.

Comment 5

[:Margaret Leibovic]

I wonder if this platform change is something that will help out metro. Cc'ing jimm in case it is.

Comment 6

[Mark Capella [:capella]]

Attachment: bug864589

I'm reposting this for review as a little research confirmed it needed help a) to better target the notification reasons we're interested in, and b) allow our reasons to co-exist with, vs. supercede, other bit flags that may be present.

Comment 8

[Mark Capella [:capella]]

Copying aaronmt ... fyi ... this should confirm the fix for bug 767065, as well as solving bug 904288.

Comment 9

[:Margaret Leibovic]

Comment on attachment 791111 [details] [diff] [review]
bug864589

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

r+ to the JS changes. Thanks for being so thorough!

Comment 10

[(no longer active)]

Comment on attachment 791111 [details] [diff] [review]
bug864589

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

::: layout/generic/nsSelection.cpp
@@ +4444,5 @@
>    if (!firstRange)
>      return NS_ERROR_FAILURE;
>  
> +  if (mFrameSelection) {
> +    short reason = mFrameSelection->PopReason() | nsISelectionListener::COLLAPSETOSTART_REASON;

int16_t here and below.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +176,5 @@
>  
> +  notifySelectionChanged: function sh_notifySelectionChanged(aDocument, aSelection, aReason) {
> +    // If the selection was collapsed to Start or to End, always close it
> +    if (aReason & Ci.nsISelectionListener.COLLAPSETOSTART_REASON ||
> +        aReason & Ci.nsISelectionListener.COLLAPSETOEND_REASON) {

Please don't rely on operator precedence like this!  Use parenthesis to make the precedence explicit.

Comment 11

[Mark Capella [:capella]]

Push to try
https://tbpl.mozilla.org/?tree=Try&rev=4055707ad9bb

Comment 12

[Mark Capella [:capella]]

>Whack-a-mole<
https://hg.mozilla.org/integration/fx-team/rev/2404f5888de3

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/2c6cf76338b5 under suspicion of breaking the build a few pushes after it landed (See https://tbpl.mozilla.org/php/getParsedLog.php?id=26657945&tree=Fx-Team for the bustage.)

Comment 14

[Mark Capella [:capella]]

re-push because uuid's are real things
https://hg.mozilla.org/integration/fx-team/rev/5a5d5ebec1fc

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5a5d5ebec1fc