914666 - Exclude frame border when calculating composition bounds

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Botond Ballo [:botond]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=913205#c10

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm sure a bit more detail in the bug won't hurt :)

Comment 2

[Botond Ballo [:botond]]

(In reply to Milan Sreckovic [:milan] from comment #1)
> I'm sure a bit more detail in the bug won't hurt :)

To calculate the composition bounds for a scrollable frame, we currently use nsIFrame::GetSize(). If I understand correctly, this is not quite what we want, because it includes the borders of the frame, whereas we just want the content area.

Comment 3

[Botond Ballo [:botond]]

Going to fix this, as it's needed to fix bug 1130982.

Comment 4

[Botond Ballo [:botond]]

Attachment: Exclude the frame borders from the composition bounds

Here's a first shot at this.

As changing the composition bounds is a relatively risky change, I'm going to wait for some Try results before flagging for review:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b064675353

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8569461 [details] [diff] [review]
Exclude the frame borders from the composition bounds

Looking green!

Comment 6

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

Comment on attachment 8569461 [details] [diff] [review]
Exclude the frame borders from the composition bounds

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

Looks sane to me but I'll defer the review to tn.

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8569461 [details] [diff] [review]
Exclude the frame borders from the composition bounds

By going from the frame rect to the scollport rect we are also going from including the space taken up by the scrollbars (if any, overlay scrollbars take up no space) to not including that space. That means lower down where we remove this space for the scrollbars is no longer correct.

And when we change the composition bounds we need to do the composition bounds dance. When doing the composition bounds dance you must do everything three times. So don't forget about nsLayoutUtils::CalculateRootCompositionSize and nsLayoutUtils::CalculateCompositionSizeForFrame.

Comment 8

[Botond Ballo [:botond]]

Attachment: Exclude the frame borders from the composition bounds

I updated the patch as follows:

  - Use the scroll port size for the non-RCD-RSF case in both
    ComputeFrameMetrics() and CalculateCompositionSizeForFrame().

    It doesn't really make sense to use it for the RCD-RSF case
    because we're not using the size of a scroll frame in that
    case.

  - Restrict the subtract-the-scrollbar-sizes code to the
    RCD-RSF case in both ComputeFrameMetrics() and
    CalculateCompositionSizeForFrame().

CalculateRootCompositionSize() required no changes because it only concerns the RCD-RSF case.

Comment 9

[Botond Ballo [:botond]]

New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4468526f6a04

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8570619 [details] [diff] [review]
Exclude the frame borders from the composition bounds

Sorry for the delay.

Comment 11

[Botond Ballo [:botond]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/118cb6ad69cb

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/118cb6ad69cb

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8570619 [details] [diff] [review]
Exclude the frame borders from the composition bounds

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Multi-FrameMetrics (bug 1055760).

User impact if declined: 
  Needed by bug 1130982, which is 2.2+

Testing completed: 
  Has been baking on m-c for a week (since 2015-03-07).

Risk to taking this patch (and alternatives if risky): 
  Moderate. The composition bounds is used by many things in APZ
  and other rendering code, so modifying it bears a moderate
  amount of risk. However, I feel like we understand this change
  well, and it's been baking on m-c for a week without regressions.

String or UUID changes made by this patch:
  None.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/461264b81424