Closed Bug 949404 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Attached patch bug949404 (obsolete) — Splinter Review
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)
Attached patch bug949404 (obsolete) — Splinter Review
Attachment #8347247 - Attachment is obsolete: true
Attachment #8347269 - Flags: review?(chrislord.net)
Flags: needinfo?(botond)
Attached patch bug949404Splinter Review
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
You need to log in before you can comment on or make changes to this bug.