1130982 - [Text Selection] The selected characters will be flashing when scrolling in contacts app and the length of characters are longer than the input field

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Gerry Chang [:gchang]]

### Steps:
1. Open Contacts app
2. Tap + icon in right-upper corner to add a new contact
3. Scroll down to Personal field
4. Type some characters in the Email field and the character length should be longer the length of the field.
5. Long press on any char to bring up the utility bubble
6. Tap "Select all"
7. Scroll up and down a little


### Expected:
1. The selected characters should not be flashing

### Actual:
1. The selected characters are flashing

### Reproduce rate
always

### Version:
Gaia-Rev        e827781324cbde91d2434b388f5dead3303a85ee
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0552759956d3
Build-ID        20150208162504
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2

Comment 1

[Gerry Chang [:gchang]]

Attachment: log.txt

Comment 2

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: New functionality which appears broken on a standard use case.

Looks like a Graphics issue. Moving this bug in that component. Feel free to move it back here if you don't think this is the right component.

Comment 3

[No-Jun Park [:njpark]]

Kats, could you take a look at it and let me know whether we should put it in a different component?  thanks!

Comment 4

[Kartikaya Gupta (email:[e-mail])]

It looks like a panning issue. qawanted for branch checks.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Layer tree dump

I presume layer 0xab386400 is the text field in question. It looks like the difference between the transform and shadow transform here is different relative to the difference between the transform and shadow transform for its sibling layers, which explains why it's moving out of sync with the rest of the content. Probably has something to do with the two FrameMetrics on the layer.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> It looks like the
> difference between the transform and shadow transform here is different
> relative to the difference between the transform and shadow transform for
> its sibling layers

That's wrong, I can't math today. The transform seems fine. Maybe culling? I'll dig in a bit more.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

The problem is the clip, which conceptually applies only to the bottommost layer in the LayerMetrics expansion. If there are APZCs for layers higher up in the expansion then their async scrolls should impact the clip.

I kinda want to bring attachment 8552646 [details] [diff] [review] back to life too in light of this. I don't know if it makes any difference specifically but it feels correct to do that.

Comment 8

[Yeojin Chung [:YeojinC]]

This issue reproduces on Flame Master and 2.1. This issue is not related to text selection as it also reproduces without repro step 5 and 6 in comment 0. 

Result: Flickering on the entered text is observed while scrolling. 

Environmental Variables:
Device: Flame 3.0
BuildID: 20150208174128
Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec
Gecko: 3436787a82d0
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Environmental Variables:
Device: Flame 2.1
Build ID: 20150209064034
Gaia: 6ef54895100d441b37e405a64636868303d56f4c
Gecko: ab85d7772044
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

=====================================
This issue does NOT reproduce on Flame 2.0.

Result: No graphic issue is observed while scrolling the input area.

Environmental Variables:
Device: Flame 2.0
BuildID: 20150209072226
Gaia: 2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko: 3b33c4f48906
Version: 32.0 (2.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8561548 [details] [diff] [review]
Patch

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

Discussed this with Kats on IRC. Posting the convo here as we made some interesting realizations there:

[17:20] <botond> kats: we seem to have an interesting combination of multi-layer-apz and multi-FrameMetrics there, where the textbox is one of several layers that scrolls together with other layers as the page's main scrollable content ("outer" metrics)
[17:20] <botond> kats: but that layer also acts as the textbox's scrollable content ("inner" metrics)
[17:21] <kats> yup
[17:21] <botond> kats: and the problem seems to be that as the outer frame scrolls, the clip rect stays in place
[17:21] <botond> kats: so image the textbox *wasn't* scrollable, so we'd only have the outer metrics
[17:21] <kats> yeah. in terms of layering from inner to outer, we need to apply the inner apzc transform, then the clip rect, then the outer apzc transform
[17:22] <botond> kats: why doesn't the same problem happen with the clip rect then?
[17:22] <kats> botond: if the textbox wasn't scrollable i think the layers would end up on a single PaintedLayer
[17:23] <kats> i.e. we wouldn't have multi-layer-apz going on there
[17:23] <botond> kats: but suppose they're not for some other reason
[17:23] <botond> kats: like position:fixed / z-order stuff
[17:23] <botond> kats: basically, i'm wondering why the problem is specific to multi-FM
[17:23] <botond> kats: (since, the solution is specific to multi-FM)
[17:24] <kats> botond: i think the answer is that the clip rect on that layer is a function of that textbox being scrollable
[17:24] <kats> botond: if the textbox wasn't scrollable, that clip rect would either not exist or be a different rect
[17:25] <kats> botond: it is the value that it has now because it needs to prevent the textbox contents from overflowing
[17:25] <botond> kats: ah, that makes sense
[17:26] <botond> kats: in the case of multi-layer-apz, the correct clip rect is probably the clip rect for the entire scrollable frame
[17:26] <kats> right
[17:26] <botond> kats: that suggests the correct clip rect for multi-FM is: clip rect for inner frame, translated by async transform of outer frame, INTERSECT clip rect for outer frame
[17:27] <kats> botond: yeah that makes sense
[17:27] <botond> kats: i.e. my theory is that with the current patch, if you quickly scroll the outer layer so that the text box goes outside the *outer* clip, you might get incorrect rendering
[17:27] <kats> botond: that might be true. i can try it
[17:28] <kats> botond: you are correct
[17:28] <kats> i can make that textbox contents appear in the "add contact" header area
[17:28] <botond> heh :)
[17:28] <kats> how are we supposed to get that outer clip?
[17:28] <botond> kats: composition bounds of outer metrics?
[17:30] <kats> botond: that should probably work, yeah

Minusing until the rendering problem we discovered this patch to cause is resolved.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Updated as per the above comment/discussion.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update LayerMetrics::GetClip also

Resurrecting attachment 8552646 [details] [diff] [review] because in light of this bug I think it's needed for correct hit-testing. Specifically, if we use the same clip for all of the LayerMetrics instances for a Layer, but they have different async transforms, then the hit-testing tree will have incorrect clips. However if we only pick up the clip for the bottommost LayerMetrics, then the code at [1] will use the composition bounds for the rest of them, which corresponds to what we're doing in AsyncCompositionManager.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=7dcb8ac18d89#229

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8562289 [details] [diff] [review]
Patch v2

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

Holding off r+ until we clear up the AdjustForClip thing.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +623,5 @@
>      combinedAsyncTransform *= (Matrix4x4(asyncTransformWithoutOverscroll) * overscrollTransform);
> +    if (i > 0 && clipRect) {
> +      // The clip already set on the layer is with respect to "FrameMetrics[0]"
> +      // (i.e. the bottommost LayerMetrics expansion item). The clip must be
> +      // adjusted for async transforms from any other metrics on this layer.

After reading some relevant Layout code [1] [2], I think I came up with a useful characterization of the clip rect Layout calculates for layers with scrollable metrics: the intersection of the composition bounds ("scroll ports" in Layout terminology) of all the scroll frames associated with the layer.

Given that, I think we can improve this comment a bit:

  // The clip rect Layout calculates is the intersection of the composition
  // bounds of all the scroll frames at the time of the paint (when there
  // are no async transforms).
  // An async transform on a scroll frame does not affect the composition
  // bounds of *that* scroll frame, but it does affect the composition bounds
  // of the scroll frames *below* it.
  // Therefore, if we have multiple scroll frames associated with this layer,
  // the clip rect needs to be adjusted for the async transforms of the
  // scroll frames other than the bottom-most one.
  // To make this adjustment, we start with the Layout-provided clip rect,
  // and at each level other than the bottom, transform it by the async
  // transform at that level, and then re-intersect it with the composition
  // bounds at that level.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=d3b41b0ead7d#3168
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=a4786600afa5#3641

@@ +641,5 @@
>      SetShadowTransform(aLayer,
>          aLayer->GetLocalTransform() * AdjustForClip(combinedAsyncTransform, aLayer));
> +    if (clipRect) {
> +      aLayer->AsLayerComposite()->SetShadowClipRect(clipRect.ptr());
> +    }

The AdjustForClip() call on the previous line reads the shadow clip rect. I *think* we want it to read the transformed clip rect rather than the original one, though I confess I'm finding it a bit difficult to think about this.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Update the shadow clip (v3)

Updated comment and yes I agree the shadow-clip update should come before the shadow-transform update. I totally didn't even notice that, good catch!

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8563051 [details] [diff] [review]
Part 1 - Update the shadow clip (v3)

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

Thanks!

Comment 15

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd1f192c0e8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/07b8436dc3bb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3dc843772d

https://treeherder.mozilla.org/logviewer.html#?job_id=6570571&repo=mozilla-inbound

Comment 17

[Kartikaya Gupta (email:[e-mail])]

So there's a couple of issues here. The test is supposed to test nested async-scrolled content, but if you look at [1] you can see that the inner "scrollable" div is 800px tall while its contents are only 700px tall. Therefore that inner scrollable div is not scrollable at all. That's one issue, although fixing that doesn't make the test pass.

The other problem is that now the compositor side code is assuming that the clip corresponds only to the innermost scrollframe whereas layout is still intersecting the clips all the way up. For the testcase in this bug, where the textfield is entirely contained inside the parent scrollable that's not a problem. For the test though, the inner scrollable extends outside the scrollport of the outer scrollable, and so the extra clip intersection does make a difference. I don't think we can change the layout-side behaviour because it includes clips from other things like CSS clip properties.

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/07b8436dc3bb/layout/reftests/async-scrolling/nested-1.html#l9

Comment 18

[Botond Ballo [:botond]]

I'll take over this while Kats is busy dealing with bug 1121871 / bug 1134202.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - New reftest for this bug

I wrote this test which fails without the "part 1" patch, and passes with the patch. It's supposed to simulate the behaviour of the textbox in question. However when I looked at the snapshot of the test case from the failure, it had the purple leaking outside the scrollport which was unexpected to me. So that is probably worth investigating.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Also FWIW you can reproduce this easily using a desktop build with the APZ pref enabled and the apz.pan_repaint_interval pref dialed up to 1000. Load http://people.mozilla.org/~kgupta/bug/1130982.html and pan the outer frame and you'll see the inner frame leak out because it's not getting clipped right.

Comment 21

[Stephany Wilkes]

Triage is blocking based on committed feature for 2.2 not working. No seizures, please. ;)

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> Also FWIW you can reproduce this easily using a desktop build with the APZ
> pref enabled and the apz.pan_repaint_interval pref dialed up to 1000. Load
> http://people.mozilla.org/~kgupta/bug/1130982.html and pan the outer frame
> and you'll see the inner frame leak out because it's not getting clipped
> right.

Thanks, I can see it in my B2G device build as well.

With the current patches applied, that problem goes away, but the problem mentioned in comment 17, which is making the async-scrolling/nested-1.html reftest fail, appears.

This problem can be seen in the same page as well, as follows:
  - pan the outer scrollable down so that the inner scrollable is partially out of view
  - quickly pan the outer scrollable back up to bring the inner scrollable fully back into view

The region of the inner scrollable which was out of view is clipped away by Layout intersecting the scroll ports as they stood at paint time; nothing the patches currently do to adjust for async transforms brings that region back.

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 4 - Fix the nested-1 reftest so the inner div is actually scrollable

This fixes the first issue mentioned in comment 17.

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 5 - Do not clip to scrollports beyond the bottommost one on the layout side

This fixes the second issue mentioned in comment 17.

It brings the nested-1 reftest much closer to passing, but it still fails, apparently due to the fact that the composition bounds is not an exact approximation to the clip rect that layout would use.

Comment 25

[Botond Ballo [:botond]]

Attachment: Remaining reftest failure

The attached output (for use with Reftest Analyzer) shows the remaining reftest failure.

The problem is that the composition bounds of the outer scroll frame - which we are using as an approximation for the clip layout would add for that scroll frame - includes the frame's border, leading to the content of the inner scroll frame (the purple rectangle) covering the outer frame's border (black). 

In the reference page, this does not happen, suggesting that the clip layout is adding excludes the border.

Timothy, do you have any ideas for how we might be able to get a more accurate approximation to the clip rect on the compositor side?

Comment 26

[Botond Ballo [:botond]]

A possibly-crazy idea: how badly do you think things would go wrong if we used nsIScrollableFrame::GetScrollPortRect().Size() as the composition bounds size rather than nsIFrame::GetSize()?

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #26)
> A possibly-crazy idea: how badly do you think things would go wrong if we
> used nsIScrollableFrame::GetScrollPortRect().Size() as the composition
> bounds size rather than nsIFrame::GetSize()?

This sounds like exactly what we should be doing. In fact I thought there was a bug open about doing this.

Comment 28

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #27)
> (In reply to Botond Ballo [:botond] from comment #26)
> > A possibly-crazy idea: how badly do you think things would go wrong if we
> > used nsIScrollableFrame::GetScrollPortRect().Size() as the composition
> > bounds size rather than nsIFrame::GetSize()?
> 
> This sounds like exactly what we should be doing. In fact I thought there
> was a bug open about doing this.

Indeed, bug 914666. Seems like it's time to finally fix that.

Comment 29

[Botond Ballo [:botond]]

nested-1 is now passing with the patch in bug 914666 + the patches in this bug!

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 3 - New reftest for this bug

Since we now have seem to have a complete solution, time to get the remaining patches reviewed.

Comment 31

[Botond Ballo [:botond]]

Attachment: Part 4 - Fix the nested-1 reftest so the inner div is actually scrollable

Comment 32

[Botond Ballo [:botond]]

Attachment: Part 5 - Do not clip to scrollports beyond the bottommost one on the layout side

Comment 33

[Botond Ballo [:botond]]

Try push with bug 914666 + these patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2336a21ab5dc

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8569494 [details] [diff] [review]
Part 5 - Do not clip to scrollports beyond the bottommost one on the layout side

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

LGTM, but deferring review to Matt.

Comment 35

[Botond Ballo [:botond]]

Now that the dependent bug 914666 has landed, this bug is ready to land as well.

Marking checkin-needed as the tree is closed.

Comment 36

[Carsten Book [:Tomcat]]

Part 2 failed to apply:

Hunk #1 FAILED at 329
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/LayerMetricsWrapper.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 2-github-clip-89d6076

Comment 37

[Botond Ballo [:botond]]

Rebased and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/24ce2aa557c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6455f1515958
https://hg.mozilla.org/integration/mozilla-inbound/rev/344d28f3a625
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1012f97e00
https://hg.mozilla.org/integration/mozilla-inbound/rev/174bde941610

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24ce2aa557c6
https://hg.mozilla.org/mozilla-central/rev/6455f1515958
https://hg.mozilla.org/mozilla-central/rev/344d28f3a625
https://hg.mozilla.org/mozilla-central/rev/9b1012f97e00
https://hg.mozilla.org/mozilla-central/rev/174bde941610

Comment 39

[Botond Ballo [:botond]]

Attachment: Rollup patch for 2.2 uplift

Here is a roll-up patch, rebased to apply to the b2g37 branch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Multi-FrameMetrics (bug 1055760).

User impact if declined: 
  With some page structures, elements are over-clipped (less of
  the element is rendered than should be) during async scrolling.

Testing completed: 
  On m-c since 2015-03-12.

Risk to taking this patch (and alternatives if risky): 
  This patch itself is a fairly low-risk, well-understood change.
  However, please note that it depends on bug 914666, which I've
  described as moderately risky.

String or UUID changes made by this patch:
  None.

Comment 40

[bhavana bajaj [:bajaj]]

NI, njpark so he can help do some testing around the areas where this code path gets executed, to gain better confidence given the highlighted risk.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/01b6893212da

Comment 42

[No-Jun Park [:njpark]]

I tested on the locally built 2.2 gecko, and the bug is no longer reproducible, and didn't see any major graphical regression.  I will check again quickly on tomorrow's 2.2 nightly.

Comment 43

[No-Jun Park [:njpark]]

Fix is verified in:
Gaia-Rev        d0e09d5e6367e558824f9cbf691da99cedf63037
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/793d61bb0bd4
Build-ID        20150317002504
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150317.040712
FW-Date         Tue Mar 17 04:07:23 EDT 2015
Bootloader      L1TC000118D0

Comment 44

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 2.2 and Flame 3.0. Following STR, highlighted text does not flash when scrolling.

Device: Flame 2.2 (full flash 319MB KK)
BuildID: 20150317002504
Gaia: d0e09d5e6367e558824f9cbf691da99cedf63037
Gecko: 793d61bb0bd4
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0 Master (full flash 319MB KK)
BuildID: 20150317073344
Gaia: 738987bd80b0ddb4ccf853855388c2627e19dcc1
Gecko: 008b3f65a7e0
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0