Closed Bug 1115370 Opened 8 years ago Closed 8 years ago

TextSelection lost abruptly on unrelated Tab:pagehide messages


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

Not set


(Not tracked)

Firefox 37


(Reporter: capella, Assigned: capella)



(1 file, 2 obsolete files)

Attached patch bugSelect.diff (obsolete) — Splinter Review
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.
Attachment #8541257 - Flags: review?(margaret.leibovic)
Assignee: nobody → markcapella
Attached patch bug1115370test.diff (obsolete) — Splinter Review
Modify SelectionHandler for corrected pagehide logic.
Attachment #8541889 - Flags: review?(margaret.leibovic)
Comment on attachment 8541257 [details] [diff] [review]

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.
Attachment #8541257 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8541889 [details] [diff] [review]

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.
Attachment #8541889 - Flags: review?(margaret.leibovic) → review-
Attached patch bug1115370.diffSplinter Review
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:

(We don't close, when we should)
Attachment #8541257 - Attachment is obsolete: true
Attachment #8541889 - Attachment is obsolete: true
Attachment #8542188 - Flags: review?(margaret.leibovic)
Attachment #8542188 - Flags: review?(margaret.leibovic) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.