Closed
Bug 1115370
Opened 6 years ago
Closed 6 years ago
TextSelection lost abruptly on unrelated Tab:pagehide messages
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 37
People
(Reporter: capella, Assigned: capella)
Details
Attachments
(1 file, 2 obsolete files)
2.43 KB,
patch
|
Margaret
:
review+
|
Details | Diff | 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 | ||
Updated•6 years ago
|
Assignee: nobody → markcapella
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•6 years ago
|
||
Modify SelectionHandler for corrected pagehide logic.
Attachment #8541889 -
Flags: review?(margaret.leibovic)
Comment 2•6 years ago
|
||
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 3•6 years ago
|
||
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-
Assignee | ||
Comment 4•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8542188 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 5•6 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=517908aa1dc3
Assignee | ||
Comment 6•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/af6aa5d204c4
https://hg.mozilla.org/mozilla-central/rev/af6aa5d204c4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•1 month ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•