Closed Bug 1073290 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
blocking-b2g 2.1+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: jdegeus, Assigned: roc)

References

Details

(Keywords: regression, Whiteboard: [2.1-exploratory-2])

Attachments

(3 files, 1 obsolete file)

Attached image 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
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Flags: needinfo?(dharris)
Whiteboard: [2.1-flame-test-run-2] → [2.1-exploratory-2]
[Blocking Requested - why for this release]:

Nominating this to block 2.1? Poor UX, as well as a regressoion
blocking-b2g: --- → 2.1?
QA Whiteboard: [QAnalyst-Triage?]
Component: Gaia::System::Window Mgmt → Graphics
Product: Firefox OS → Core
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Broken by Bug 967844? can you take a look Robert
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(roc)
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.
blocking-b2g: 2.1? → 2.1+
Component: Graphics → Gaia::Dialer
Product: Core → Firefox OS
Component: Gaia::Dialer → Graphics
Product: Firefox OS → Core
This looks like related to bug 967844. Kats, do you have any comment for this bug?
Flags: needinfo?(bugmail.mozilla)
Nope. roc should be back next week and hopefully will be able to take a look at it then.
Flags: needinfo?(bugmail.mozilla)
Parking with Rob.
Assignee: nobody → roc
Component: Graphics → Layout
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.
Flags: needinfo?(roc)
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); >]
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 :-(.
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.
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.
(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).
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.
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.
Attached patch fix (obsolete) — Splinter Review
Attachment #8505944 - Flags: review?(matt.woodrow)
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 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.
Attached patch fix v2Splinter Review
I fixed some sites related to nsDisplayZoom, and another one in nsLayoutUtils, but this looks like it.
Attachment #8506642 - Flags: review?(matt.woodrow)
Attachment #8505944 - Attachment is obsolete: true
Attachment #8505944 - Flags: review?(matt.woodrow)
Attachment #8506642 - Flags: review?(matt.woodrow) → review+
Roc, Is this ready to land?
Flags: needinfo?(roc)
Yes I think so. Needs a try push.
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/2dc71497e243
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please request b2g34 approval on this patch when you get a chance :)
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
Flags: needinfo?(roc)
Attachment #8506642 - Flags: approval-mozilla-b2g34?
Attachment #8506642 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.