Closed Bug 914825 Opened 6 years ago Closed 6 years ago

[regression] wide page cannot be scrolled vertically

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
blocking-b2g koi+
Tracking Status
firefox26 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

The patch for bug 904533 is causing a regression on B2G where some pages like http://people.mozilla.org/~kgupta/grid.html cannot be scrolled vertically.

The problem is that the height of the composition bounds of the frame being scrolled is the height of the content, where we were expecting it to be the height of the screen. The composition bounds is set in nsDisplayList.cpp in RecordFrameMetrics() based on the frame's size (as of bug 904533) - this may be incorrect.
Blocks: 904533
There are two problems here:

  1) The size of the root scroll frame is, I think, unexpected. 
     I filed a separate bug for this: bug 914864.

  2) Even if (1) is resolved, it's inappropriate to set the
     composition bounds of the root scroll frame to be the same
     as the frame's size, because if the page is zoomed in, the
     frame size will be larger than the screen size, but the
     composition bounds should be the screen size.

Attached is a patch that fixes (2).
Attachment #802613 - Flags: review?(tnikkel)
Attachment #802613 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 802613 [details] [diff] [review]
Clamp the composition bounds of the root scroll frame to the widget bounds

We already have a copy of the presContext in a local var, we should just use that.

The name "isRootScrollFrame" because it actually means "isRootContentDocRootScrollFrame".

Correct me if I'm wrong but I think we want to subtract the margins for (possible) scrollbars from the widget rect because that is what they are drawn on. (I may have said the opposite in another earlier bug)
Comment on attachment 802613 [details] [diff] [review]
Clamp the composition bounds of the root scroll frame to the widget bounds

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

I'm having second thoughts about this. I feel like perhaps the mCompositionBounds should be multiplied by metrics.GetParentResolution() rather than metrics.mCumulativeResolution (using the terminology from your patch on bug 912806), and that would make the clamping unnecessary. But I could be wrong. I will think about it more tomorrow morning.
Assignee: nobody → botond
Updated patch to address review comments from comment #2.

Regarding comment #3, kats and I talked about this on IRC and identified 2 issues with that approach:
 1) it would make resolution of this issue dependent on bug 914864
 2) it would give incorrect results for subframes if we later enable zooming them

so I suggest we go with the original approach.
Attachment #802613 - Attachment is obsolete: true
Attachment #802613 - Flags: review?(tnikkel)
Attachment #802613 - Flags: feedback?(bugmail.mozilla)
Attachment #803357 - Flags: review?(tnikkel)
Attachment #803357 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 803357 [details] [diff] [review]
Clamp the composition bounds of the root scroll frame to the widget bounds

Hmm, after having seen it this way I think what we actually want is to subtract the scrollbar margins from the widget rect if we are using the widget rect, otherwise subtract them from the frame rect.

Sorry to once again make you change it.
Unfortunately, this patch breaks the patch for bug 898478 - attempting to scroll the inner frame in http://people.mozilla.org/~kgupta/tmp/iframe.html?nest with both patches applied causes the content of the inner iframe to jump around badly. I'm sure not why - I'll have to debug it.
(In reply to Botond Ballo [:botond] from comment #6)
> Unfortunately, this patch breaks the patch for bug 898478 - attempting to
> scroll the inner frame in
> http://people.mozilla.org/~kgupta/tmp/iframe.html?nest with both patches
> applied causes the content of the inner iframe to jump around badly. I'm
> sure not why - I'll have to debug it.

... and now I'm not seeing this any more. I must have done the previous test with some other patch not applied by accident...
Attached patch bug914825.patch (obsolete) — Splinter Review
Updated patch to address comment #5.
Attachment #803357 - Attachment is obsolete: true
Attachment #803357 - Flags: review?(tnikkel)
Attachment #803357 - Flags: feedback?(bugmail.mozilla)
Attachment #803791 - Flags: review?(tnikkel)
Attachment #803791 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 803791 [details] [diff] [review]
bug914825.patch

Sorry again, but I think I was too vague in what I meant last time.

If we use the widget rect we should subtract the scrollbar margins from the widget rect and then use it for clamping the same way.

If we don't use the widget rect we should subtract the scrollbar margins from the scrollframe's rect.

I think this is correct based on what the scrollbars are actually applied to.

Sorry for this endless back and forth.
(In reply to Timothy Nikkel (:tn) from comment #10)
> If we use the widget rect we should subtract the scrollbar margins from the
> widget rect and then use it for clamping the same way.

Doesn't that give the same result as clamping first and then subtracting?
(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Timothy Nikkel (:tn) from comment #10)
> > If we use the widget rect we should subtract the scrollbar margins from the
> > widget rect and then use it for clamping the same way.
> 
> Doesn't that give the same result as clamping first and then subtracting?

(I guess not if the frame rect is smaller than the widget rect, but will that ever happen for the root scroll frame?)
(In reply to Botond Ballo [:botond] from comment #12)
> (In reply to Botond Ballo [:botond] from comment #11)
> > (In reply to Timothy Nikkel (:tn) from comment #10)
> > > If we use the widget rect we should subtract the scrollbar margins from the
> > > widget rect and then use it for clamping the same way.
> > 
> > Doesn't that give the same result as clamping first and then subtracting?
> 
> (I guess not if the frame rect is smaller than the widget rect, but will
> that ever happen for the root scroll frame?)

I think it could, even if it does not currently.
(In reply to Botond Ballo [:botond] from comment #9)
> Try job: https://tbpl.mozilla.org/?tree=Try&rev=807e77dad665

This try run is showing a number of test failures on B2G that look like they might be caused by this bug. I will look into them.
No longer blocks: 914993
Duplicate of this bug: 914993
blocking-b2g: --- → koi?
Keywords: regression
(In reply to Botond Ballo [:botond] from comment #14)
> (In reply to Botond Ballo [:botond] from comment #9)
> > Try job: https://tbpl.mozilla.org/?tree=Try&rev=807e77dad665
> 
> This try run is showing a number of test failures on B2G that look like they
> might be caused by this bug. I will look into them.

After pulling the latest code and rebasing my patch, the test failures have gone away: https://tbpl.mozilla.org/?tree=Try&rev=9352aa796b83
Attached patch bug914825.patch (obsolete) — Splinter Review
Updated patch to address comment #10. Hopefully the scrollbar adjustment is correct now :)
Attachment #803791 - Attachment is obsolete: true
Attachment #803791 - Flags: review?(tnikkel)
Attachment #803791 - Flags: feedback?(bugmail.mozilla)
Attachment #804599 - Flags: review?(tnikkel)
Attachment #804599 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 804599 [details] [diff] [review]
bug914825.patch

compsosition has an extra s in it.
Attachment #804599 - Flags: review?(tnikkel) → review+
Attached patch bug914825.patchSplinter Review
Updated patch to fix typo. Carrying r+.
Attachment #804599 - Attachment is obsolete: true
Attachment #804599 - Flags: feedback?(bugmail.mozilla)
Attachment #804606 - Flags: review+
Attachment #804606 - Flags: feedback?(bugmail.mozilla)
Attachment #804606 - Flags: feedback?(bugmail.mozilla) → feedback+
https://hg.mozilla.org/mozilla-central/rev/4932106f16e5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
blocking-b2g: koi? → koi+
You need to log in before you can comment on or make changes to this bug.