960886 - selection monocles being spilled over to other tabs when switching

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: Monocles spilled over to other tab while switching

If you have text selected via the monocles (Gmail email draft in this instance), switching to another tab will spill over the monocles to the new tab. If you tap or interact with the new tab, the monocles will disappear. I've reproduced this with both Nightly and Aurora builds (see below for specific builds)

- Attached a screenshot to illustrate the issue

Steps to reproduce the issue:

1) Open Firefox Metro
2) Create two different tabs (first tab: Gmail, second tab: Bing)
3) Log into Gmail (first tab) and start creating a new email (I dumped a few paragraphs of text into the email)
4) Select several sentences using the selection monocle
5) Once you have two monocles (one at each end of the selected text), switch to the second tab (Bing) you created in Step #2 (notice the monocles spilled over to the Bing tab)

Current Behavior:

- Selection monocles are being spilled over when switching to other tabs currently opened

Expected Behavior:

- When a user switches to another tab, the monocles shouldn't spill over

Used the following builds to find the issue:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-16-00-40-03-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-16-03-02-50-mozilla-central/

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Simpler way to reproduce the issue:

1) Open Firefox Metro
2) Open two tabs (Tab #1: Wikipedia, Tab #2: Bing)
3) Select a few sentences under Wikipedia (should have two selection monocles at both ends of the selected text)
3) Switch over to the second tab (Bing), the monocles will spill over

Comment 2

[Jim Mathies [:jimm]]

Looks like we aren't picking up the scroll offset of the editable content sub doc of the draft.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #2)
> Looks like we aren't picking up the scroll offset of the editable content
> sub doc of the draft.

nix that, wrong bug.

Comment 4

[Jim Mathies [:jimm]]

nice find. this should be an easy fix.

Comment 5

[Jim Mathies [:jimm]]

Actually this is bit more complex that I originally thought since selection for the tab may be in a sub document. I'll come back to this.

Comment 6

[Jim Mathies [:jimm]]

Attachment: wip

Comment 7

[Jim Mathies [:jimm]]

For v1 a cheaper, low risk fix would be to shut down selection control on tab switches.

Comment 8

[Oleg Zasypkin [:azasypkin]]

Attachment: short term solution shutdown selection v1.diff

Implemented short term solution as suggested. Using separate "case" branches for URLChanged and TabSelect events as we'll need to divide it for long term solution anyway.

Tests are currently running on Try: https://tbpl.mozilla.org/?tree=Try&rev=795544b7cc0d

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8364373 [details] [diff] [review]
short term solution shutdown selection v1.diff

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

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +1070,5 @@
>        case "URLChanged":
>          this._shutdown();
>          break;
>  
> +      case "TabSelect":

You can just add this to case "URLChanged" above.

::: browser/metro/base/tests/mochitest/browser_selection_basic.js
@@ +330,5 @@
> +    let lastTab = yield addTab(chromeRoot + "browser_selection_basic.html");
> +
> +    // Switch back to the initial tab
> +    Browser.selectedTab = initialTab;
> +    yield waitForEvent(Elements.tabList, "TabSelect");

You probably want to set up this listener before you switch tabs, since the event might fire before waitForEvent registers the listener.

let promise = waitForEvent(Elements.tabList, "TabSelect");
Browser.selectedTab = initialTab;
yield promise;

@@ +339,5 @@
> +    yield waitForCondition(()=>SelectionHelperUI.isSelectionUIVisible);
> +
> +    // Switch to another tab
> +    Browser.selectedTab = lastTab;
> +    yield waitForEvent(Elements.tabList, "TabSelect");

same here, or just skip this since I think your waitForCondition below will suffice.

Comment 10

[Oleg Zasypkin [:azasypkin]]

Attachment: short term solution shutdown selection v2.diff

Thanks for review!

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8364401 [details] [diff] [review]
short term solution shutdown selection v2.diff

Looks good!

Comment 12

[Oleg Zasypkin [:azasypkin]]

All tests passed on Try.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

patching file browser/metro/base/tests/mochitest/browser_selection_basic.js
bad hunk #1 @@ -313,15 +313,47 @@ gTests.push({
 (15 15 48 47)

Comment 14

[Oleg Zasypkin [:azasypkin]]

Attachment: short term solution shutdown selection v2 merged.diff

Sorry, fixed.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/670741cc94a5

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcc00e89ec8f

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed using the steps from the description and comment 1 on a Surface Pro 2 with Firefox 28 beta 2, latest Nightly and Aurora builds.