960889 - monocles moving when swiping up/down through gmail email draft

Status: VERIFIED FIXED

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

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: 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/

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Attachment: monocles are being repositioned when swiping through email

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

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Attachment: test-case.zip

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

Comment 4

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Attachment: 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

[Jim Mathies [:jimm]]

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.

Comment 6

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

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.

Comment 7

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Attachment: 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.

Comment 8

[Jim Mathies [:jimm]]

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

[Jim Mathies [:jimm]]

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.

Comment 10

[Jim Mathies [:jimm]]

(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!

Comment 11

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

(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.

Comment 12

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Attachment: 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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f811709a2392

Comment 15

[Matt Brubeck (:mbrubeck)]

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

Comment 16

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

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.