Created attachment 265034 [details] Try scrolling the page and then the sidebar When a page contains a scrollable fixed div and the page is scrolled, the hit-region of the fixed div does not appear to scroll with the page, so attempts to scroll the fixed div will instead scroll the page. This is easily observed in the following websites: https://docs.angularjs.org/api http://underscorejs.org https://developer.apple.com/library/safari/documentation/AppleApplications/Reference/SafariWebContent/Introduction/Introduction.html as well as the attached test case, where scrolling the page and then hovering over the sidebar and attempting to scroll it will instead cause the page to scroll again.
<rdar://problem/23445723>
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. ***