monocles moving when swiping up/down through gmail email draft

VERIFIED FIXED in Firefox 28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: kjozwiak, Assigned: azasypkin)

Tracking

unspecified
Firefox 29
x86_64
Windows 8.1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 verified, firefox29 verified)

Details

(Whiteboard: [release28] [defect] p=8)

Attachments

(4 attachments, 2 obsolete attachments)

When you have some text selected while using Gmail to create a new email, sliding the email up or down will reposition the monocles rather than the monocles staying with the selected text. I've reproduced this on both Nightly and Aurora builds.

- I've attached a few screenshots to illustrate the issue

Steps to reproduce the issue:

1) Open Firefox Metro
2) Log into Gmail and select "Compose"
3) Paste a few paragraphs into the email (enough so you can scroll through the email)
4) Select a few sentences with the selection monocle (should have two monocles on each end of the selected text)
5) Swipe either up or down to move through the email. You should notice the monocles will also move rather than staying with the selected text

Current Behavior:

- When a user has some text selected in an email using Gmail, swiping up and down so they can view the rest of the email shouldn't move the monocles

Expected Behavior:

- When the user swipes through the email, the monocles should stay with the selected text and reposition

Found the issue using the following builds:
- 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/
No longer blocks: 943071
Looks like we aren't picking up the scroll offset of the editable content sub doc of the draft.
Whiteboard: [triage] [defect] p=0 → [release28] [defect] p=0
Whiteboard: [release28] [defect] p=0 → [release28] [defect] p=3
Assignee: nobody → azasypkin
Blocks: metrov1it23
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] [defect] p=3 → [release28] [defect] p=8
Posted file test-case.zip
Very simplified test case, but with the same structure and problem as Gmail composer has.
Posted patch wip.diff (obsolete) — Splinter Review
Looks like the problem is the following: Gmail email composer uses iframe (in design mode) inside scrollable div. When we select anything inside email draft and trying to scroll then entire iframe is scrolled inside this scrollable container rather then just content inside its own iframe. To improve  performance we calculate _contentOffset only once(on selection start) and reuse it on every subsequent selection update event. But in Gmail case _contentOffset actually changes while we scroll.

Here is just quick hacky fix to show that recalculating _contentOffset on every selection update solves issue. But it's not a solution though as recalculating _contentOffset seems too heavy operation.
You could pass a flag to onSelectionUpdate when the apz transform end event occurs, that way you only recalculate the offset when we need to.
Yeah, it'll reduce number of recalculations, but I see that onSelectionUpdate is triggered by apzc frequently enough. Probably it can be short term solution.
Added isInitiatedByAZPC flag to reduce number of contentOffset recalculations. Also will add follow-up bug for hiding monocles when underlying selection isn't visible like in Gmail case as it seems a good candidate for a separate bug.
Attachment #8366175 - Attachment is obsolete: true
Attachment #8367343 - Flags: review?(jmathies)
This test failed for me at line 68, the monocles weren't visible after the drag and there was no selection. I think what happens here is that the apzc is getting in the way of the taps. I swapped the first native tap out for a dom tap and things worked. You should throw this at try and retrigger a few times to be sure you have a reliable test.

-    sendNativeTap(selectMenuItem);
+    sendElementTap(gWindow, selectMenuItem);
Comment on attachment 8367343 [details] [diff] [review]
selection monocles for iframe inside scrollable div.diff

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

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +305,5 @@
>        this._onFail("_onSelectionUpdate was called without proper view set up");
>        return;
>      }
>  
> +    // TODO(azasypkin): Investigate performance impact of calculating content

I'd take this out, personally I'm not a fan of todos in code. Just make sure to file the follow up bug.
Attachment #8367343 - Flags: review?(jmathies) → review+
(In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> Created attachment 8367343 [details] [diff] [review]
> selection monocles for iframe inside scrollable div.diff
> 
> Added isInitiatedByAZPC flag to reduce number of contentOffset
> recalculations. Also will add follow-up bug for hiding monocles when
> underlying selection isn't visible like in Gmail case as it seems a good
> candidate for a separate bug.

There's a post v1 improve selection story tracker you can attach these new selection bugs too as well - bug 957244.

Thanks!
Blocks: 965388
(In reply to Jim Mathies [:jimm] from comment #10)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> > Created attachment 8367343 [details] [diff] [review]
> > selection monocles for iframe inside scrollable div.diff
> > 
> > Added isInitiatedByAZPC flag to reduce number of contentOffset
> > recalculations. Also will add follow-up bug for hiding monocles when
> > underlying selection isn't visible like in Gmail case as it seems a good
> > candidate for a separate bug.
> 
> There's a post v1 improve selection story tracker you can attach these new
> selection bugs too as well - bug 957244.
> 
> Thanks!
Thanks for review! I'll post Try link as soon as it becomes available, seems it's closed currently.

Sure, will attach related bugs to 957244.
Blocks: 965411
Blocks: 957244
Replaced [TODO] with follow-up bug 965411. Tests have passed on Try: https://tbpl.mozilla.org/?tree=Try&rev=017f3c7d5748
Attachment #8367343 - Attachment is obsolete: true
Attachment #8367820 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/f811709a2392
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [release28] [defect] p=8 → [release28] [defect] p=8[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f811709a2392
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [release28] [defect] p=8[fixed-in-fx-team] → [release28] [defect] p=8
Target Milestone: --- → Firefox 29
Tested on a Surface Pro 2 device using the STR from the description with latest Aurora, latest Nightly and Firefox 28 beta 2 builds.

Marking this as verified as I've logged bug 971699 for the remaining issue.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.