Closed Bug 960886 Opened 6 years ago Closed 6 years ago

selection monocles being spilled over to other tabs when switching

Categories

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

x86_64
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: kjozwiak, Assigned: azasypkin)

References

Details

(Whiteboard: [beta28] [defect] p=5)

Attachments

(2 files, 3 obsolete files)

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/
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
No longer blocks: 943071
Looks like we aren't picking up the scroll offset of the editable content sub doc of the draft.
(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.
nice find. this should be an easy fix.
Assignee: nobody → jmathies
Whiteboard: [triage] [defect] p=0 → [beta28] [defect] p=0
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=1
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.
Assignee: jmathies → nobody
Attached patch wip (obsolete) — Splinter Review
Whiteboard: [beta28] [defect] p=1 → [beta28] [defect] p=0
Whiteboard: [beta28] [defect] p=0 → [traige] [defect] p=0
For v1 a cheaper, low risk fix would be to shut down selection control on tab switches.
Whiteboard: [traige] [defect] p=0 → [beta28] [defect] p=0
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=2
Assignee: nobody → azasypkin
Blocks: metrov1it23
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] [defect] p=2 → [beta28] [defect] p=5
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
Attachment #8364373 - Flags: review?(jmathies)
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.
Attachment #8364373 - Flags: review?(jmathies) → review-
Thanks for review!
Attachment #8364373 - Attachment is obsolete: true
Attachment #8364401 - Flags: review?(jmathies)
Comment on attachment 8364401 [details] [diff] [review]
short term solution shutdown selection v2.diff

Looks good!
Attachment #8364401 - Flags: review?(jmathies) → review+
All tests passed on Try.
Keywords: checkin-needed
Attachment #8361628 - Attachment is obsolete: true
patching file browser/metro/base/tests/mochitest/browser_selection_basic.js
bad hunk #1 @@ -313,15 +313,47 @@ gTests.push({
 (15 15 48 47)
Keywords: checkin-needed
Sorry, fixed.
Attachment #8364401 - Attachment is obsolete: true
Attachment #8364634 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/670741cc94a5
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [beta28] [defect] p=5 → [beta28] [defect] p=5[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/670741cc94a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] [defect] p=5[fixed-in-fx-team] → [beta28] [defect] p=5
Target Milestone: --- → Firefox 29
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.