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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Maybe the issue is that we never update the metrics.mViewport values for subframes. So it ends up that the layer metrics is wrong.

Comment 3

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

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.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Seems like I can reproduce that on UI tests as well.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I think that one of the common point between UI tests and FireText is that they both display their some content inside an iframe.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug949404

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug949404

Seems like I messed up the commit message.

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 8347271 [details] [diff] [review]
bug949404

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

Lgtm, nice catch :)

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/b2g-inbound/rev/76280df86771

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76280df86771

Target milestone tracks m-c landing. Status flags track branch uplifts.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/662ea4febe89