Bug 151015

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Mac

OS:

Unspecified

Attachments:

Description	Flags
Try scrolling the page and then the sidebar	none
Test patch to check that this doesn't break anything.	none
Archive of layout-test-results from ews106 for mac-mavericks-wk2	none
Patch	none
Patch	none
Patch	none
Added bool* param to scrollableAreaBoundingBox.	simon.fraser: review+

Wenson Hsieh

Reported 2015-11-08 22:29:43 PST

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.

Attachments
Try scrolling the page and then the sidebar (964 bytes, text/html) 2015-11-08 22:29 PST, Wenson Hsieh	no flags	Details
Test patch to check that this doesn't break anything. (10.75 KB, patch) 2015-11-09 13:16 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (796.74 KB, application/zip) 2015-11-09 13:56 PST, Build Bot	no flags	Details
Patch (20.30 KB, patch) 2015-11-09 15:11 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (18.87 KB, patch) 2015-11-09 16:16 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (18.84 KB, patch) 2015-11-09 16:24 PST, Wenson Hsieh	no flags	Details Formatted Diff Diff
*Added bool param to scrollableAreaBoundingBox.** (22.74 KB, patch) 2015-11-09 16:47 PST, Wenson Hsieh	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2015-11-08 22:30:43 PST

<rdar://problem/23445723>

Wenson Hsieh

Comment 2 2015-11-09 13:16:31 PST

Created attachment 265090 [details] Test patch to check that this doesn't break anything.

WebKit Commit Bot

Comment 3 2015-11-09 13:17:59 PST

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.

Build Bot

Comment 4 2015-11-09 13:56:01 PST

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

Build Bot

Comment 5 2015-11-09 13:56:06 PST

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

Wenson Hsieh

Comment 6 2015-11-09 15:11:07 PST

Created attachment 265104 [details] Patch

Simon Fraser (smfr)

Comment 7 2015-11-09 15:24:05 PST

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?

Wenson Hsieh

Comment 8 2015-11-09 15:33:13 PST

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.

Wenson Hsieh

Comment 9 2015-11-09 16:16:48 PST

Created attachment 265113 [details] Patch

Wenson Hsieh

Comment 10 2015-11-09 16:24:21 PST

Created attachment 265114 [details] Patch

Simon Fraser (smfr)

Comment 11 2015-11-09 16:26:44 PST

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.

Wenson Hsieh

Comment 12 2015-11-09 16:40:48 PST

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.

Wenson Hsieh

Comment 13 2015-11-09 16:47:46 PST

Created attachment 265119 [details] Added bool* param to scrollableAreaBoundingBox.

Simon Fraser (smfr)

Comment 14 2015-11-09 16:54:50 PST

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

Wenson Hsieh

Comment 15 2015-11-09 17:07:44 PST

Committed r192193: <http://trac.webkit.org/changeset/192193>

Wenson Hsieh

Comment 16 2015-11-09 18:23:39 PST

Follow-up: Windows build fix in <http://trac.webkit.org/changeset/192197>.

Wenson Hsieh

Comment 17 2018-04-27 19:19:21 PDT

*** Bug 150658 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.