1115370 - TextSelection lost abruptly on unrelated Tab:pagehide messages

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Attachment: bugSelect.diff

The SelectionHandler listens for pagehide events on BrowserApp.deck, in order to detect page navigation, and History traversal.

(We currently allow only the currentTab to have active TextSelection action).

Unrelated pagehide events triggered by background tabs are heard and cause seemingly random loss of TextSelection (handles or caret).

Simple STR: Open several tabs/pages with FF set to 'reopen tabs' on start. Swipe close FF, then re-start. Long-press some text word on a page and wait to observe loss of selection.

Comment 1

[Mark Capella [:capella]]

Attachment: bug1115370test.diff

Modify SelectionHandler for corrected pagehide logic.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8541257 [details] [diff] [review]
bugSelect.diff

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

Good catch. r+ with comment addressed.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +226,5 @@
> +        // We only care about events on the selected tab.
> +        let tab = BrowserApp.getTabForWindow(aEvent.originalTarget.defaultView);
> +        if (tab == BrowserApp.selectedTab) {
> +          this._closeSelection();
> +        }

The same reasoning would be true for "blur" events, so I think you should just add this if statement around the current _closeSelection call we have to handle both "pagehide" and "blur" events.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8541889 [details] [diff] [review]
bug1115370test.diff

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

::: mobile/android/base/tests/roboextender/testSelectionHandler.html
@@ +207,5 @@
>  
>    }).then(function() {
>      sh.startSelection(inputNode);
> +    sh.handleEvent({ type: "pagehide", originalTarget: {} });
> +    return ok(sh.isSelectionActive(), "unrelated pagehide should not close active selection");

I think you should add a new testcase for this, rather than replacing the current one, since we should still make sure that "pagehide" on the selected tab does the right thing.

Comment 4

[Mark Capella [:capella]]

Attachment: bug1115370.diff

I combined the two patches, and updated the test to include doing nothing on unrelated pages, and properly acting on current page.

I'd like to move this as-is, specific to the demonstrated issue and consider the "blur" case in a different bug for a couple for reasons. 

First, in the absence of a real world issue, and considering overall SelectionHandler code, I'm not sure there's an issue involving "blur".

In the second case, I'm not sure the solution here would apply easily to "blur". For those events, we "useCapture" to also act on individual editable/non-editable element blur. Would BrowserApp.getTabForWindow(aEvent.originalTarget.defaultView) be correct in all cases?

For example, in quick testing I can make this happen:
https://www.dropbox.com/s/bygz6btoc7xx4h1/bug1115370_oops.png?dl=0

(We don't close, when we should)

Comment 5

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=517908aa1dc3

Comment 6

[Mark Capella [:capella]]

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

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/af6aa5d204c4