Closed Bug 1099298 Opened 10 years ago Closed 9 years ago

Proper fix for bug 1099104

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(2 files)

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,
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.
Assignee: nobody → botond
Flags: needinfo?(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?
Flags: needinfo?(tnikkel)
That code will go away with containerless scrolling.
(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!
Attachment #8526957 - Flags: review?(tnikkel) → review+
Attachment #8526958 - Flags: review?(tnikkel) → review+
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
(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
https://hg.mozilla.org/mozilla-central/rev/a5271ded6fff
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.