Closed
Bug 914825
Opened 10 years ago
Closed 10 years ago
[regression] wide page cannot be scrolled vertically
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
Tracking | Status | |
---|---|---|
firefox26 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
4.14 KB,
patch
|
botond
:
review+
kats
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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 3•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → botond
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
(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...
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Try job: https://tbpl.mozilla.org/?tree=Try&rev=807e77dad665
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
(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?
Assignee | ||
Comment 12•10 years ago
|
||
(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?)
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Updated•10 years ago
|
blocking-b2g: --- → koi?
Updated•10 years ago
|
Keywords: regression
Assignee | ||
Comment 16•10 years ago
|
||
(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
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
Comment on attachment 804599 [details] [diff] [review] bug914825.patch compsosition has an extra s in it.
Attachment #804599 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 19•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #804606 -
Flags: feedback?(bugmail.mozilla) → feedback+
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/4932106f16e5
https://hg.mozilla.org/mozilla-central/rev/4932106f16e5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•10 years ago
|
blocking-b2g: koi? → koi+
Updated•10 years ago
|
status-firefox26:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•