Assume you have the following case: <div id="container" style="overflow: auto"> <!-- content to be flowed in regions --> <div id="article" style="-webkit-flow-into: flow"></div> <!-- regions that will fragment the content --> <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)
Created attachment 222432 [details] Patch
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
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) {
Created attachment 222446 [details] Take 2
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.
Created attachment 222447 [details] Fixed styling issue
Created attachment 222453 [details] Final patch
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&
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.
Created attachment 222563 [details] Integrated feedback from anttik
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.
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.
Created attachment 222567 [details] Patch for landing
Comment on attachment 222567 [details] Patch for landing Clearing flags on attachment: 222567 Committed r163018: <http://trac.webkit.org/changeset/163018>
(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.
All patches landed, and related regression fixed. Closing.
Comment on attachment 222563 [details] Integrated feedback from anttik Clearing review flag now that patch is landed.