monocles moving when swiping up/down through gmail email draft

VERIFIED FIXED in Firefox 28

Status

P2
normal
VERIFIED FIXED
5 years ago
5 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)

(Reporter)

Description

5 years ago
Created attachment 8361518 [details]
selection monocles moved when swiping through email

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/
(Reporter)

Comment 1

5 years ago
Created attachment 8361519 [details]
monocles are being repositioned when swiping through email
(Reporter)

Updated

5 years ago
No longer blocks: 943071

Comment 2

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

Updated

5 years ago
Whiteboard: [triage] [defect] p=0 → [release28] [defect] p=0

Updated

5 years ago
Whiteboard: [release28] [defect] p=0 → [release28] [defect] p=3

Updated

5 years ago
Assignee: nobody → azasypkin
Blocks: 961497
No longer blocks: 838081
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] [defect] p=3 → [release28] [defect] p=8
(Assignee)

Comment 3

5 years ago
Created attachment 8366004 [details]
test-case.zip

Very simplified test case, but with the same structure and problem as Gmail composer has.
(Assignee)

Comment 4

5 years ago
Created attachment 8366175 [details] [diff] [review]
wip.diff

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.

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #8366175 - Attachment is obsolete: true
Attachment #8367343 - Flags: review?(jmathies)

Comment 8

5 years ago
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 9

5 years ago
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+

Comment 10

5 years ago
(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!
(Assignee)

Updated

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 965411

Updated

5 years ago
Blocks: 957244
Created attachment 8367820 [details] [diff] [review]
selection monocles for iframe inside scrollable div.diff

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+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [release28] [defect] p=8[fixed-in-fx-team] → [release28] [defect] p=8
Target Milestone: --- → Firefox 29
https://hg.mozilla.org/releases/mozilla-aurora/rev/c9cf586ec5d9
status-firefox28: --- → fixed
status-firefox29: --- → fixed
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
status-firefox28: fixed → verified
status-firefox29: fixed → verified
You need to log in before you can comment on or make changes to this bug.