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: | P2 | Keywords: | AdobeTracked | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 57312 | ||||||||||||||||
Attachments: |
|
Description
Mihnea Ovidenie
2013-11-06 04:39:44 PST
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.
|