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

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

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)

[Radu Stavila]

Created attachment 222432 [details]
Patch

[Antti Koivisto]

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]

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]

Created attachment 222446 [details]
Take 2

[WebKit Commit Bot]

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]

Created attachment 222447 [details]
Fixed styling issue

[Radu Stavila]

Created attachment 222453 [details]
Final patch

[Antti Koivisto]

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 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]

Created attachment 222563 [details]
Integrated feedback from anttik

[Antti Koivisto]

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 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]

Created attachment 222567 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Brent Fulgham]

All patches landed, and related regression fixed. Closing.

[Brent Fulgham]

Comment on attachment 222563 [details]
Integrated feedback from anttik

Clearing review flag now that patch is landed.