[keyboard] With APZ turned on and multiple APZCs the app is not correctly repainted when the keyboard is dismissed

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
mozilla29
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 8346480 [details] [diff] [review]
update.all.compositionbounds.poc.patch

Steps to reproduce:
 - Populate Gaia with some data. |cd $GAIA; make reference-workload-medium| with the device plugged in.
 - Open the sms app
 - Open a sms thread that has a scrollable list (one of the 'big' stuff should be ok)
 - Open the keyboard by clicking on the input field at the bottom of the list
 - Click the back button

Expected result:
 - the view goes back to the previous view and it renders fine

Actual result:
 - the view goes back to the previous view and it renders fine while the transition is ongoing and then repaint to a half screen. Opening the thread again result into it beeing painted incorrectly.


Kats, the above wip solve the issues but this sounds like the wrong way to fix it. What I don't understand is that the apzc composition bounds sounds to be set to the correct values by NotifyLayersUpdated but it does not seems to repaint correctly.

Kats, botond, do you guys have any idea what can cause this ?
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(botond)
Even with this dirty hack I see the layout completely exploded sometimes on the sms app once the text box has grown artificially due to the user entering line break.
Maybe the issue is that we never update the metrics.mViewport values for subframes. So it ends up that the layer metrics is wrong.
As mentioned on IRC, based on the patch I think the most useful thing to do would be to dump out mFrameMetrics and aLayerMetrics at that point so we can see what the difference in the two is. If they are the same then the content repaint request should have no real effect, and it's surprising to me that the patch does anything at all.

As for mViewport, it is only meaningful on FrameMetrics that are for the root scrollable layer in a document, and even then they represent the CSS viewport (i.e. the layout viewport) and so I don't think it should affect what gets repainted and what doesn't. I could be wrong though, we haven't spent a lot of time looking at mViewport so far because it hasn't really caused any problems.
Flags: needinfo?(bugmail.mozilla)
I found that for the sms app the root issue is an issue with the resize event. I attached a patch to bug 942863 for that.

I still see something wrong with the FireText app. So I keep this bug opened until I understand the issue better.
Seems like I can reproduce that on UI tests as well.
I think that one of the common point between UI tests and FireText is that they both display their some content inside an iframe.
Dumping some values I see that sometimes in RecvUpdateFrame that the mViewport of mLastRootMetrics has a height of 230, when the viewport of the newSubFrameMetrics has a viewport of 396.

When the 2 values are set correctly (460/396) then the app repaint correctly.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7)
> Dumping some values I see that sometimes in RecvUpdateFrame that the
> mViewport of mLastRootMetrics has a height of 230, when the viewport of the
> newSubFrameMetrics has a viewport of 396.
> 
> When the 2 values are set correctly (460/396) then the app repaint correctly.

Actually that's not true :/
Ok. The issue seems to be that when we are clamping the display port in http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#118 then the scrollable rect can have out of date information (maybe a reflow is still ongoing?), and this set the display port to wrong values because of http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#81
Created attachment 8347247 [details] [diff] [review]
bug949404

I was initially going to ask a f? to a layout folk but since this bug is mostly the same as bug 830256, then I think the patch makes is just about not regressing it.
Assignee: nobody → 21
Attachment #8346480 - Attachment is obsolete: true
Attachment #8347247 - Flags: review?(chrislord.net)
Comment on attachment 8347247 [details] [diff] [review]
bug949404

After discussions on IRC, instead of duplicating code a new method will be added on FrameMetrics: GetExpandedScrollableRect.
Attachment #8347247 - Flags: review?(chrislord.net)
Created attachment 8347269 [details] [diff] [review]
bug949404
Attachment #8347247 - Attachment is obsolete: true
Attachment #8347269 - Flags: review?(chrislord.net)
Flags: needinfo?(botond)
Created attachment 8347271 [details] [diff] [review]
bug949404

Seems like I messed up the commit message.
Attachment #8347269 - Attachment is obsolete: true
Attachment #8347269 - Flags: review?(chrislord.net)
Attachment #8347271 - Flags: review?(chrislord.net)
Comment on attachment 8347271 [details] [diff] [review]
bug949404

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

Lgtm, nice catch :)
Attachment #8347271 - Flags: review?(chrislord.net) → review+
https://hg.mozilla.org/integration/b2g-inbound/rev/76280df86771
Status: NEW → ASSIGNED
blocking-b2g: --- → 1.3+
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/76280df86771

Target milestone tracks m-c landing. Status flags track branch uplifts.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
https://hg.mozilla.org/releases/mozilla-aurora/rev/662ea4febe89
status-b2g-v1.3: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
status-firefox29: --- → fixed

Updated

5 years ago
Depends on: 951751
You need to log in before you can comment on or make changes to this bug.