1073290 - [Card View] Bottom icons within call log of dialer overlap call logs entries when viewing from Card View/Task Manager

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jordan de Geus(JordanD)]

Attachment: 2014-09-25-15-12-27.png

Description:
When users are in Dialer and viewing the call log page, if they have more than one page full of call logs and attempt to open Card View/Task Manager, the user will see the buttons at the base of the screen will be overlapping 

Setup:
Have more than 10 entries within dialer/call log

Repro Steps:
1) Update a Flame device to BuildID: 20140925000204
2) Enter Dialer> Select Call Log
3) Press and hold Home button
4) Observe base of dialer within task manager
  
Actual:
Call log buttons at the base of the screen overlap call entries when viewing in card view
  
Expected: 
Overlapping of buttons do not occur
  
Environmental Variables:
Device: Flame 2.1 (319mb)
BuildID: 20140925000204
Gaia: 8061ab487d42cbc49b329fd68b9ca90e0fe477e6
Gecko: e970bc96f8b5
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
    
Repro frequency: 3/3
See attached: Screenshot and Logcat

Comment 1

[Jordan de Geus(JordanD)]

This issue DOES occur on Flame 2.1 (512mb)

Actual: Bottom buttons when viewing call log from Card View are overlapping

Flame 2.1 KitKat Base (512mb)

Environmental Variables:
Device: Flame 2.1
BuildID: 20140925000204
Gaia: 8061ab487d42cbc49b329fd68b9ca90e0fe477e6
Gecko: e970bc96f8b5
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

-------------------------------------------------------------------------------------

This issue DOES NOT occur on Flame 2.2 (319mb), Flame 2.0 (319mb)

Actual: Overlapping does not occur

Flame 2.2 KitKat Base (319mb)

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20140925040206
Gaia: c5d2e2f4ebf5f370d6003517057dcd47493dec90
Gecko: 32acbe1d64dc
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.0 KitKat Base (319mb)

Environmental Variables:
Device: Flame 2.0
BuildID: 20140924183011
Gaia: 87ee41fcb3f9a14d7a8bb67f1dd7fd95a6bcd0f0
Gecko: b1cb27078909
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Jordan de Geus(JordanD)]

Attachment: logcat_20140925_1613.txt

Comment 3

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

[Blocking Requested - why for this release]:

Nominating this to block 2.1? Poor UX, as well as a regressoion

Comment 4

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

The whole 2.1 Aurora branch is affected with this bug, therefore the window is found in central.

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20140902021513
Gaia: 44bf2e3bc5ddea9db9a8c851bd353cb234aa883c
Gecko: 39a8d9b2b639
Version: 34.0a1 (2.1 master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20140902025713
Gaia: 44bf2e3bc5ddea9db9a8c851bd353cb234aa883c
Gecko: ea4cfd84e417
Version: 34.0a1 (2.1 master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Gaia the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=39a8d9b2b639&tochange=ea4cfd84e417

Caused by Bug 967844.

Comment 5

[Joshua Mitchell (Inactive)]

Broken by Bug 967844? can you take a look Robert

Comment 6

[No-Jun Park [:njpark]]

FYI: In master branch, the close button (X) was moved to the bottom of the image, so it masks the bug.  The bug is briefly visible during the transition.

Comment 7

[Peter Chang[:pchang]]

This looks like related to bug 967844. Kats, do you have any comment for this bug?

Comment 8

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

Nope. roc should be back next week and hopefully will be able to take a look at it then.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Parking with Rob.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The call log entries are placed in this layer:

ClientTiledPaintedLayer (0xafc2e400) [clip=(x=0, y=135, w=480, h=605)] [transform=[ 1 0; 0 1; 0 135; ]] [bounds=(x=0, y=0, w=480, h=765)] [visible=< (x=0, y=0, w=480, h=765); >] [opaqueContent] [metrics0={ cb=(x=0.000000, y=135.000000, w=480.000000, h=605.000000) sr=(x=0.000000, y=0.000000, w=320.000000, h=510.000000) s=(0,0) dp=(x=0.000000, y=0.000000, w=320.000000, h=510.000000) cdp=(x=0.000000, y=0.000000, w=320.000000, h=510.000000) color=rgba(0, 0, 0, 0) scrollId=5 scrollParent=4 z=1.500 }] [metrics1={ cb=(x=0.000000, y=0.000000, w=480.000000, h=741.500000) sr=(x=0.000000, y=0.000000, w=320.000000, h=494.333344) s=(0,0) dp=(x=0.000000, y=0.000000, w=320.000000, h=494.333344) cdp=(x=0.000000, y=0.000000, w=320.000000, h=494.333344) color=rgba(255, 255, 255, 1) scrollId=4 scrollParent=3 z=1.500 }] [valid=< (x=0, y=0, w=480, h=765); >]

It looks like the contents of the layer are not being clipped.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On the compositor side, the clip rect is still correct:

PaintedLayerComposite (0xaab22000) [shadow-clip=(x=0, y=135, w=480, h=605)] [shadow-transform=[ 1 0; 0 1; 0 135; ]] [shadow-visible=< (x=0, y=0, w=480, h=765); >] [clip=(x=0, y=135, w=480, h=605)] [transform=[ 1 0; 0 1; 0 135; ]] [bounds=(x=0, y=0, w=480, h=765)] [visible=< (x=0, y=0, w=480, h=765); >] [opaqueContent] [metrics0={ cb=(x=0.000000, y=135.000000, w=480.000000, h=605.000000) sr=(x=0.000000, y=0.000000, w=320.000000, h=510.000000) s=(0,0) dp=(x=0.000000, y=0.000000, w=320.000000, h=510.000000) cdp=(x=0.000000, y=0.000000, w=320.000000, h=510.000000) color=rgba(0, 0, 0, 0) scrollId=5 scrollParent=4 z=1.500 }] [metrics1={ cb=(x=0.000000, y=0.000000, w=480.000000, h=741.500000) sr=(x=0.000000, y=0.000000, w=320.000000, h=494.333344) s=(0,0) dp=(x=0.000000, y=0.000000, w=320.000000, h=494.333344) cdp=(x=0.000000, y=0.000000, w=320.000000, h=494.333344) color=rgba(255, 255, 255, 1) scrollId=4 scrollParent=3 z=1.500 }] [valid=< (x=0, y=0, w=480, h=765); >]

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is weird because the drawWindow call is using DRAW_WIDGET_LAYERS and it's going through the compositor to take a snapshot. So it's unclear to me how that can show a clipping bug while normal compositing does not :-(.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We construct a temporary BasicLayerManager to paint (for the drawWindow screenshot). Since we're not building retained layers, FrameLayerBuilder's ContainerState::Finish skips calling PostprocessRetainedLayers, so SetupScrollingMetadata is never called and scrollport clipping is not applied to the constructed layers. This is a bug.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is difficult to fix.

ScrollFrameHelper::BuildDisplayList computes usingDisplayport and only clips its display items to the scrollport when usingDisplayport is true. In that case, scrollport clipping is set on each layer via ScrollFrameHelper::ComputeFrameMetrics, when this scrollframe is an ancestor of the animated geometry root for the layer.

In this case, usingDisplayport is true but there is no retained layer for which this scrollframe is an ancestor of the animated geometry root. This could happen in a few ways --- forced-inactive layers (e.g. when doing fallback client-side rendering for CSS features not supported in the compositor), or BasicLayers layer flattening).

So the basic problem is that at display list construction time, we don't know whether a scrollframe's usingDisplayport is going to be honored or not, so we don't really know whether to clip its display items or not.

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> This is difficult to fix.
> 
> ScrollFrameHelper::BuildDisplayList computes usingDisplayport and only clips
> its display items to the scrollport when usingDisplayport is true.

You mean when usingDisplayport is *false*, right? That threw me for a bit.

Without a displayport, ScrollFrameHelper clips the display items to the scrollport. With one, we leave the items unclipped, and add a Layer clip during layer building (via a call to ComputeFrameMetrics).

Comment 16

[Matt Woodrow (:mattwoodrow)]

One option would be to have ScrollFrameHelper::BuildDisplayList add the clips regardless, but add some sort of flag to the clip to indicate that it should be ignored when we get a real displayport layer.

Since we probably need to combine these flagged clips with other non-flagged clips, I think we'd need to move the current mClipRect value into the rounded rect list. That way we can make the 'ignore if we get a displayport layer' flag a property of the rounded rect list entry, and combining clips can still work in a somewhat sane way.

I'm sure there's other variations along the theme of putting the clip data on the display item and discarding it when we've built layer, possibly keeping two DisplayItemClips per item.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chatted with Matt on IRC. We agreed that if BasicLayers flattening conflicts with async scrolling, we should prefer async scrolling. This basically means that when APZC is enabled on a platform, BasicLayers flattening can be completely disabled on that platform. That leaves us with the following action items:

1) When APZC is enabled, disable BasicLayers flattening.
2) In scrollframes, when rendering a non-retained layer tree, ignore displayports. It's pointless to render a layer to fill a displayport when we know there can be no async scrolling of that layer. This item alone will fix this bug.
3) Refactor displaylist code to decide during display list construction when forced-inactive layers are needed, and use that information in scrollframes to disable displayport display list construction.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8505944 [details] [diff] [review]
fix

You should also at least change nsSubDocumentFrame::BuildDisplayList, it also gets the display port. And there are probably other places too.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8505944 [details] [diff] [review]
fix

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

Like tn said, we need to check this in nsSubDocumentFrame too. Might be worth putting the code somewhere shared.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

I fixed some sites related to nsDisplayZoom, and another one in nsLayoutUtils, but this looks like it.

Comment 22

[Tony Chung [:tchung]]

Roc, Is this ready to land?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes I think so. Needs a try push.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3b05ea4decec

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc71497e243

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2dc71497e243

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Please request b2g34 approval on this patch when you get a chance :)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8506642 [details] [diff] [review]
fix v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 967844
User impact if declined: Incorrect screenshots in some cases
Testing completed: Just a trunk landing
Risk to taking this patch (and alternatives if risky): Seems relatively low risk
String or UUID changes made by this patch: none

Comment 29

[sarsenyev]

Verified the issue is fixed on 2.2 master build

(X) was moved to the bottom of the image and no overlapping text appears

----------------------------------------
Device: Flame 2.2 Master KK (319mb, Full Flash)
BuildID: 20141024040202
Gaia: d893a9b971a0f3ee48e5a57dca516837d92cf52b
Gecko: a5ee2769eb27
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Adding verifyme for 2.1 verification once the patch has been uplifted.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f249dc879450

Comment 31

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.1.

Result: Overlapping issue does not occur in the card view.

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141028001203
Gaia: a0174f7166745256aaca1cb3aa9f894033fbffa6
Gecko: 43bda3541f6b
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0