Summary: | Sometimes unable to scroll fixed div when the body is scrollable | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | buildbot, cmarcelo, commit-queue, esprehn+autocc, glenn, hello, jamesr, kondapallykalyan, luiz, rniwa, simon.fraser, thorton, tonikitoo, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2015-11-08 22:29:43 PST
Created attachment 265090 [details]
Test patch to check that this doesn't break anything.
Attachment 265090 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 265090 [details] Test patch to check that this doesn't break anything. Attachment 265090 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/407032 New failing tests: tiled-drawing/scrolling/non-fast-region/wheel-handler-inside-fixed.html tiled-drawing/scrolling/non-fast-region/wheel-handler-on-fixed.html tiled-drawing/scrolling/non-fast-region/wheel-handler-fixed-child.html Created attachment 265094 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 265104 [details]
Patch
Comment on attachment 265104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265104&action=review > Source/WebCore/page/FrameView.cpp:1079 > +LayoutRect FrameView::scrollableBoundsInflatedForMainframeScrollableArea(const LayoutRect& uninflatedBounds) const I think this needs a better name. It's really "boundsInflatedForScrolling", and only applies to things inside position:fixed, which the name should imply. > Source/WebCore/rendering/RenderLayer.cpp:1494 > + ASSERT(absoluteBoundingBoxRect.x() < intMaxForLayoutUnit && absoluteBoundingBoxRect.y() < intMaxForLayoutUnit && absoluteBoundingBoxRect.width() < intMaxForLayoutUnit && absoluteBoundingBoxRect.height() < intMaxForLayoutUnit); The assert doesn't add anything. > Source/WebCore/rendering/RenderLayer.cpp:1495 > + absoluteBoundingBoxRect = IntRect(renderer().view().frameView().scrollableBoundsInflatedForMainframeScrollableArea(LayoutRect(absoluteBoundingBoxRect))); I don't think this is the right level to do this inflation; I think the caller should do it. > LayoutTests/tiled-drawing/scrolling/scroll-in-fixed-div-when-mainframe-scrolled.html:1 > +<html> Couldn't this test just dump the non-fast scrollable region? Comment on attachment 265104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265104&action=review Thanks for taking a look -- I'll have a new version up soon. >> Source/WebCore/rendering/RenderLayer.cpp:1494 >> + ASSERT(absoluteBoundingBoxRect.x() < intMaxForLayoutUnit && absoluteBoundingBoxRect.y() < intMaxForLayoutUnit && absoluteBoundingBoxRect.width() < intMaxForLayoutUnit && absoluteBoundingBoxRect.height() < intMaxForLayoutUnit); > > The assert doesn't add anything. Ok -- I'll remove. >> Source/WebCore/rendering/RenderLayer.cpp:1495 >> + absoluteBoundingBoxRect = IntRect(renderer().view().frameView().scrollableBoundsInflatedForMainframeScrollableArea(LayoutRect(absoluteBoundingBoxRect))); > > I don't think this is the right level to do this inflation; I think the caller should do it. Good point -- the scrollable area's bounding box should still return the actual bounds >.<. I'll move it into ScrollingCoordinator::absoluteNonFastScrollableRegionForFrame. >> LayoutTests/tiled-drawing/scrolling/scroll-in-fixed-div-when-mainframe-scrolled.html:1 >> +<html> > > Couldn't this test just dump the non-fast scrollable region? I thought it would be good to have an end to end test to verify the change to NFSR behavior results in being able to scroll. I can change this into an NFSR dump test instead. Created attachment 265113 [details]
Patch
Created attachment 265114 [details]
Patch
Comment on attachment 265114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265114&action=review > Source/WebCore/platform/ScrollableArea.h:232 > + virtual IntRect scrollableAreaBoundingBox(bool& /* isScrollableAreaFixed */) const { return scrollableAreaBoundingBox(); } > virtual IntRect scrollableAreaBoundingBox() const = 0; Don't add an overload. Just add a bool*=nullptr param. Comment on attachment 265114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265114&action=review >> Source/WebCore/platform/ScrollableArea.h:232 >> virtual IntRect scrollableAreaBoundingBox() const = 0; > > Don't add an overload. Just add a bool*=nullptr param. Got it -- fixed. Created attachment 265119 [details]
Added bool* param to scrollableAreaBoundingBox.
Comment on attachment 265119 [details] Added bool* param to scrollableAreaBoundingBox. View in context: https://bugs.webkit.org/attachment.cgi?id=265119&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:137 > + bool isScrollableAreaFixed; isScrollableAreaFixed -> isInsideFixed > Source/WebCore/rendering/RenderLayer.cpp:1489 > +IntRect RenderLayer::scrollableAreaBoundingBox(bool* isScrollableAreaFixed) const isScrollableAreaFixed -> isInsideFixed > Source/WebCore/rendering/RenderLayer.h:877 > + virtual IntRect scrollableAreaBoundingBox(bool* isScrollableAreaFixed = nullptr) const override; isScrollableAreaFixed -> isInsideFixed Committed r192193: <http://trac.webkit.org/changeset/192193> Follow-up: Windows build fix in <http://trac.webkit.org/changeset/192197>. *** Bug 150658 has been marked as a duplicate of this bug. *** |