1099298 - Proper fix for bug 1099104

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Back story:

  - Bug 1076163 fixed a long-standing APZ issue with rendering pages
    that use CSS transforms

  - However, it broke rendering of normal pages on Fennec (bug 1099104).
    A temporary fix was applied there, which essentially partially
    reverts bug 1076163, and thus breaks pages with CSS transforms
    again.

This bug is to fix the issue properly,

Comment 1

[Botond Ballo [:botond]]

The proximate cause of the problem is that on Fennec, in the ComputeFrameMetrics() call for the RCD-RSF (root content document's root scroll frame), aContainerParameters.m[X|Y]Scale does not include the pres shell resolution of the RCD.

I did some preliminary investigation into why this is the case, and this is what I've discovered:

  - The container parameters originate in nsDisplayList::PaintRoot(),
    where the scale is set to the display root document's pres shell
    resolution.

  - On B2G, the display root document is the RCD, so the RCD's pres
    shell resolution is picked up here, and all is well.

  - On Fennec:

     - The display root document is not the RCD, so nothing
       interesting is picked up in PaintRoot().

     - During frame-layer-building, ProcessDisplayItems() is called 
       on the display list that contains the display item for the 
       RCD. Since the RCD is a subdocument, and since it has a pres
       shell resolution, the display item for the RCD is an
       nsDisplayResolution.

         - ProcessDisplayItems() calls BuildLayer() on the
           nsDisplayResolution. nsDisplayResolution::BuildLayer()
           constructs a container params object with the
           RCD's pres shell resolution, and calls
           nsDisplaySubDocument::BuildLayer() with that

            --> but this container params object never makes
                it back out to ProcessDisplayItems()

     - The ComputeFrameMetrics() call for the RCD happens later
       in ProcessDisplayItems(), after calling BuildLayer(),
       but it passes in the old container parameters that don't
       contain the RCD's pres shell resolution.

This seems like it might be a bug introduced during multi-layer-apz, since, IIRC, before that ComputeFrameMetrics() (then called RecordFrameMetrics()) would be called by BuildLayer(), and so the RCD's pres shell resolution would make it to the RFM call.

Timothy, do you agree that this is a bug? If so, do you have a suggestion for fixing it? I'm thinking of a fix along one of the following lines:

  - If the container parameters are stored in the layer somewhere, we
    can query them when calling ComputeFrameMetrics().

  - We could get BuildLayer() to communicate the modified parameters
    back to its caller, e.g. by takng them by non-const reference.

but you might have a better idea.

Comment 2

[Timothy Nikkel (:tnikkel)]

I think the easiest way to fix this is to just make nsDisplaySubDocument::ComputeFrameMetrics add the local resolution in to the container parameters. Sound good?

Comment 3

[Timothy Nikkel (:tnikkel)]

That code will go away with containerless scrolling.

Comment 4

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #2)
> I think the easiest way to fix this is to just make
> nsDisplaySubDocument::ComputeFrameMetrics add the local resolution in to the
> container parameters. Sound good?

Yeah, that's much simpler. Thanks!

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 1 - Add the pres shell resolution to the container layer parameters in nsDisplaySubDocument::ComputeFrameMetrics()

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 2 - Revert the temporary fix in bug 1099104

Comment 7

[Botond Ballo [:botond]]

I tested manually on Fennec (to make sure the STR in bug 1099104 aren't regressed) and on B2G (to make sure the apz-css-transforms stage 1 fix is in effect again). Both look good.

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c733dc16a778

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> Try push:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c733dc16a778

There were some build failures on B2G because my patches were on top of an incorrectly-rebased patch for bug 1073081. Here's a new, B2G-only Try push with that fixed: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9421578e3021

Comment 9

[Botond Ballo [:botond]]

Try pushes look clean.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5271ded6fff
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a9b9b54acc

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5271ded6fff