TextSelection lost abruptly on unrelated Tab:pagehide messages

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 37
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted 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
Status: NEW → ASSIGNED
Posted 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]
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.
Attachment #8541257 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8541889 - Flags: review?(margaret.leibovic) → 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:
https://www.dropbox.com/s/bygz6btoc7xx4h1/bug1115370_oops.png?dl=0

(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+
https://hg.mozilla.org/mozilla-central/rev/af6aa5d204c4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.