Bug 123886

Summary:

[CSSRegions] Unable to scroll a scrollable container for regions using mouse wheel

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

CSS

Assignee:

Radu Stavila <stavila>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, esprehn+autocc, glenn, koivisto, kondapallykalyan, mitz, stavila, WebkitBugTracker

Priority:

Keywords:

AdobeTracked

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

57312

Attachments:

Description	Flags
Patch	koivisto: review-
Take 2	none
Fixed styling issue	none
Final patch	none
Integrated feedback from anttik	none
Patch for landing	none

Mihnea Ovidenie

Reported 2013-11-06 04:39:44 PST

Assume you have the following case: <div id="container" style="overflow: auto">  <div id="article" style="-webkit-flow-into: flow"></div>  <div id="region" style="-webkit-flow-from: flow"></div> <div id="region2" style="-webkit-flow-from: flow"></div> <div id="region3" style="-webkit-flow-from: flow"></div> <div id="region4" style="-webkit-flow-from: flow"></div> </div> Assume that there is enough content in the flow to fill the regions and the size of the container is less than the size of regions so that the container gets vertical scrollbar. Case1. If the mouse cursor is over the flow thread fragmented content inside a region, the user is unable to scroll the content inside the container. Case2. However, if the mouse is outside the flow thread content but still inside the container, then the user is able to scroll the container content. There *should be* a possibility to scroll the container content even when the mouse cursor is over the flow thread content in regions. (Case1)

Attachments
Patch (33.90 KB, patch) 2014-01-28 05:43 PST, Radu Stavila	koivisto: review-	Details Formatted Diff Diff
Take 2 (27.87 KB, patch) 2014-01-28 08:41 PST, Radu Stavila	no flags	Details Formatted Diff Diff
Fixed styling issue (27.88 KB, patch) 2014-01-28 08:45 PST, Radu Stavila	no flags	Details Formatted Diff Diff
Final patch (27.89 KB, patch) 2014-01-28 08:59 PST, Radu Stavila	no flags	Details Formatted Diff Diff
Integrated feedback from anttik (27.75 KB, patch) 2014-01-29 03:34 PST, Radu Stavila	no flags	Details Formatted Diff Diff
Patch for landing (28.82 KB, patch) 2014-01-29 05:36 PST, Radu Stavila	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radu Stavila

Comment 1 2014-01-28 05:43:31 PST

Created attachment 222432 [details] Patch

Antti Koivisto

Comment 2 2014-01-28 06:10:19 PST

Comment on attachment 222432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222432&action=review > Source/WebCore/rendering/RenderBox.cpp:763 > +bool RenderBox::scroll(ScrollDirection direction, ScrollGranularity granularity, float multiplier, Element** stopElement, Node* startNode, IntPoint absolutePoint) This should probably be tightened to take Element* for start. Text node's renderer is never a RenderBox. The method signature is pretty confusing in general. What do "startNode" and "absolutePoint" mean in context of scrolling? > Source/WebCore/rendering/RenderBox.h:455 > + virtual bool scroll(ScrollDirection, ScrollGranularity, float multiplier = 1, Element** stopElement = 0, Node* startNode = 0, IntPoint absolutePoint = IntPoint()); nullptr Please consider factoring this into multiple functions with sensible method signatures and no default parameters. > Source/WebCore/rendering/RenderFlowThread.cpp:467 > +RenderRegion* RenderFlowThread::regionFromAbsolutePointAndBox(IntPoint absolutePoint, const RenderBox* flowedBox) > +{ > + if (!flowedBox) > + return 0; Please make flowedBox parameter a reference and don't call the function if it is null. I think the function can be const. > Source/WebCore/rendering/RenderFlowThread.cpp:470 > + RenderRegion* startRegion = 0; > + RenderRegion* endRegion = 0; Use nullptr everywhere. > Source/WebCore/rendering/RenderFlowThread.cpp:474 > + if (!startRegion) > + return 0; nullptr > Source/WebCore/rendering/RenderFlowThread.cpp:477 > + for (RenderRegionList::iterator iter = m_regionList.find(startRegion); iter != m_regionList.end(); ++iter) { > + RenderRegion* region = *iter; Please use C++11 range-for syntax. > Source/WebCore/rendering/RenderFlowThread.cpp:486 > + return 0; nullptr

Antti Koivisto

Comment 3 2014-01-28 06:52:47 PST

I guess range-for won't work here since you are not iterating from begin(). You should still use auto for the iterator and save the end iterator: for (auto it = m_regionList.find(startRegion), end = m_regionList.end(); it != end; ++iter) {

Radu Stavila

Comment 4 2014-01-28 08:41:05 PST

Created attachment 222446 [details] Take 2

WebKit Commit Bot

Comment 5 2014-01-28 08:42:57 PST

Attachment 222446 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderBox.cpp:784: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Radu Stavila

Comment 6 2014-01-28 08:45:32 PST

Created attachment 222447 [details] Fixed styling issue

Radu Stavila

Comment 7 2014-01-28 08:59:03 PST

Created attachment 222453 [details] Final patch

Antti Koivisto

Comment 8 2014-01-28 09:27:19 PST

Comment on attachment 222446 [details] Take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=222446&action=review > Source/WebCore/page/EventHandler.cpp:284 > +static inline bool scrollNode(float delta, ScrollGranularity granularity, ScrollDirection positiveDirection, ScrollDirection negativeDirection, Node* node, Element** stopElement, IntPoint absolutePoint) It would be better to use const IntPoint& for consistency. That's what most of the code does. > Source/WebCore/page/EventHandler.cpp:293 > RenderBox* enclosingBox = node->renderer()->enclosingBox(); > float absDelta = delta > 0 ? delta : -delta; > - return enclosingBox->scroll(delta < 0 ? negativeDirection : positiveDirection, granularity, absDelta, stopElement); > + > + return enclosingBox->scrollWithWheelEventLocation(delta < 0 ? negativeDirection : positiveDirection, granularity, absDelta, node, stopElement, absolutePoint); Here we get enclosingBox() from node renderer which is renderer() itself if it is a RenderBox. We invoke scrollWithWheelEventLocation() on it with the node as argument... > Source/WebCore/rendering/RenderBox.cpp:785 > + RenderLayer* l = layer(); > + if (l && l->scroll(direction, granularity, multiplier)) { Please use full words as variables names, 'layer' not 'l' (you can do this->layer() to avoid collision). > Source/WebCore/rendering/RenderBox.cpp:801 > + RenderBox* flowedBox = this; > + > + if (startNode) { > + if (RenderBox* box = startNode->renderBox()) > + flowedBox = box; > + } ... so here I think startNode->renderBox() will be either null or equivalent to 'this' in the first recursive call. Seems like the code could be simplified and clarified by replacing Node* startNode argument with RenderBox* startBox. Then the code above can be replaced with RenderBox* flowedBox = startBox ? startBox : this; What do you think? > Source/WebCore/rendering/RenderFlowThread.h:118 > + RenderRegion* regionFromAbsolutePointAndBox(IntPoint, const RenderBox& flowedBox); const IntPoint&

Radu Stavila

Comment 9 2014-01-29 02:54:56 PST

Comment on attachment 222446 [details] Take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=222446&action=review >> Source/WebCore/rendering/RenderBox.cpp:785 >> + if (l && l->scroll(direction, granularity, multiplier)) { > > Please use full words as variables names, 'layer' not 'l' (you can do this->layer() to avoid collision). Yeah, this was copied from the scroll method. I'll rename it. >> Source/WebCore/rendering/RenderBox.cpp:801 >> + } > > ... so here I think startNode->renderBox() will be either null or equivalent to 'this' in the first recursive call. Seems like the code could be simplified and clarified by replacing Node* startNode argument with RenderBox* startBox. > > Then the code above can be replaced with > > RenderBox* flowedBox = startBox ? startBox : this; > > What do you think? Looks good.

Radu Stavila

Comment 10 2014-01-29 03:34:22 PST

Created attachment 222563 [details] Integrated feedback from anttik

Antti Koivisto

Comment 11 2014-01-29 03:45:14 PST

Comment on attachment 222563 [details] Integrated feedback from anttik View in context: https://bugs.webkit.org/attachment.cgi?id=222563&action=review > Source/WebCore/rendering/RenderBox.cpp:796 > + RenderBox* flowedBox = startBox ? startBox : this; I don't think startBox is ever actually null so you don't need this.

Antti Koivisto

Comment 12 2014-01-29 04:07:34 PST

Comment on attachment 222563 [details] Integrated feedback from anttik View in context: https://bugs.webkit.org/attachment.cgi?id=222563&action=review > Source/WebCore/rendering/RenderBox.cpp:792 > + RenderLayer* boxLayer = layer(); > + if (boxLayer && boxLayer->scroll(direction, granularity, multiplier)) { > + if (stopElement) > + *stopElement = element(); > + return true; > + } > + > + if (stopElement && *stopElement && *stopElement == element()) > + return true; This stuff is identical with scroll() code and should really be factored into a private or static helper function.

Radu Stavila

Comment 13 2014-01-29 05:36:13 PST

Created attachment 222567 [details] Patch for landing

WebKit Commit Bot

Comment 14 2014-01-29 07:16:18 PST

Comment on attachment 222567 [details] Patch for landing Clearing flags on attachment: 222567 Committed r163018: <http://trac.webkit.org/changeset/163018>

mitz

Comment 15 2014-02-02 21:11:51 PST

(In reply to comment #14) > (From update of attachment 222567 [details]) > Clearing flags on attachment: 222567 > > Committed r163018: <http://trac.webkit.org/changeset/163018> This caused bug 128090.

Brent Fulgham

Comment 16 2014-02-04 09:33:44 PST

All patches landed, and related regression fixed. Closing.

Brent Fulgham

Comment 17 2014-02-04 09:34:09 PST

Comment on attachment 222563 [details] Integrated feedback from anttik Clearing review flag now that patch is landed.

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