Bug 129301 - [CSS Regions] Scrollable regions
Summary: [CSS Regions] Scrollable regions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2014-02-25 05:24 PST by Radu Stavila
Modified: 2014-03-05 13:57 PST (History)
7 users (show)

See Also:


Attachments
Patch (25.05 KB, patch)
2014-02-28 08:04 PST, Radu Stavila
hyatt: review-
Details | Formatted Diff | Diff
Reviewed patch (52.81 KB, patch)
2014-03-05 06:42 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Reviewed patch (52.77 KB, patch)
2014-03-05 06:59 PST, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 2014-02-25 05:24:17 PST
When the last region in a region chain has overflow:scroll, the content should be scrollable.
Comment 1 Radu Stavila 2014-02-28 08:04:22 PST
Created attachment 225465 [details]
Patch
Comment 2 Radu Stavila 2014-02-28 08:21:56 PST
Created additional issues for remaining region scroll problems:

https://bugs.webkit.org/show_bug.cgi?id=129485
https://bugs.webkit.org/show_bug.cgi?id=129487
Comment 3 Dave Hyatt 2014-02-28 11:58:43 PST
Comment on attachment 225465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225465&action=review

r-

> Source/WebCore/rendering/RenderBlock.cpp:2343
> -    if (hasOverflowClip())
> -        scrolledOffset.move(-scrolledContentOffset());
> +    if (scrollParent->hasOverflowClip())
> +        scrolledOffset.move(-scrollParent->scrolledContentOffset());

A better way to do this would be to add a virtual function that takes the paintoffset and returns the scrolledOffset. Then you could override that virtual function in RenderNamedFlowThread to do the correct adjustment. That avoids putting isRenderNamedFlowThread queries right into RenderBlock.

Is patching only painting (and not repainting invalidation) really sufficient? It looks suspicious that painting is being patched but not repainting.

> Source/WebCore/rendering/RenderLayer.cpp:5481
> +        if (clipRectsContext.region->isRenderNamedFlowFragment()) {
> +            RenderBlockFlow& fragmentContainer = toRenderNamedFlowFragment(clipRectsContext.region)->fragmentContainer();
> +            if (fragmentContainer.hasOverflowClip()) {
> +                IntSize scrolledContentOffset = fragmentContainer.scrolledContentOffset();
> +                layerBoundsWithVisualOverflow.inflateX(scrolledContentOffset.width());
> +                layerBoundsWithVisualOverflow.inflateY(scrolledContentOffset.height());
> +            }
> +        }

Not really happy with this special case sitting right in the calculateRects code. Maybe it could be moved to a helper or something.

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:76
> +    // Regions do not inherit the overflow:scroll style,
> +    // scrolling is performed by the region's container.
> +    if (parentStyle.overflowX() != OSCROLL)
> +        style.get().setOverflowX(parentStyle.overflowX());
> +    else
> +        style.get().setOverflowX(OVISIBLE);
> +    
> +    if (parentStyle.overflowY() != OSCROLL)
> +        style.get().setOverflowY(parentStyle.overflowY());
> +    else
> +        style.get().setOverflowY(OVISIBLE);

This doesn't seem right. What about overflow:auto? We have helper methods for this, i.e., scrollsOverflow that you should be using.

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:267
> +bool RenderNamedFlowFragment::shouldClipFlowThreadContent() const
> +{
> +    if (RenderRegion::shouldClipFlowThreadContent())
> +        return true;
> +    
> +    return isLastRegion() && (style().regionFragment() == BreakRegionFragment || fragmentContainer().style().overflowY() == OSCROLL);
> +}

Checking the style directly once you have a renderer isn't the way you should be doing this. Overflow isn't always applied from style. You should use the scrollsOverflow renderer methods.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:307
> +    // Take the scrolled offset of the region into consideration.
> +    RenderBlockFlow& fragmentContainer = fragment.fragmentContainer();
> +    if (fragmentContainer.hasOverflowClip()) {
> +        IntSize scrolledContentOffset = fragmentContainer.scrolledContentOffset();
> +        visualOverflowRect.inflateX(scrolledContentOffset.width());
> +        visualOverflowRect.inflateY(scrolledContentOffset.height());
> +    }

This placement looks suspicious to me with flipped block writing modes.
Comment 4 Andrei Bucur 2014-03-03 08:22:09 PST
Comment on attachment 225465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225465&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2337
> +    if (isRenderNamedFlowThread() && paintInfo.renderNamedFlowFragment)

I think this doesn't work well with self-painting content such as relatively positioned elements. I suppose it's better to use the fact we are painting the flow thread layer in the region at a certain offset, based on the portion offset, and somehow adjust that for scrolling instead of patching the render tree.
We know the region has a self-painting layer and we offset the flow thread and the graphics context to paint only portions of the flow thread. Scrolling is a bit like that, offsetting content from its default position, so maybe we can do it all at once.
Comment 5 Radu Stavila 2014-03-05 06:42:43 PST
Created attachment 225878 [details]
Reviewed patch
Comment 6 Andrei Bucur 2014-03-05 06:58:29 PST
Comment on attachment 225878 [details]
Reviewed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225878&action=review

Much better I must say :).

> Source/WebCore/rendering/RenderLayer.cpp:5292
> +    IntSize scrolledContentOffset = namedFlowFragment->fragmentContainer().hasOverflowClip() ? namedFlowFragment->fragmentContainer().scrolledContentOffset() : IntSize();

Why use IntSize() and not always the scrolledContentOffset()? For unscrollable boxes it should be IntRect() anyway, right?

> Source/WebCore/rendering/RenderNamedFlowFragment.cpp:253
> +    return isLastRegion() && (style().regionFragment() == BreakRegionFragment || fragmentContainer().scrollsOverflow());

scrollsOverflow() is always false if hasOverflowClip() is false. It's not required any more.
Comment 7 Radu Stavila 2014-03-05 06:59:59 PST
Created attachment 225880 [details]
Reviewed patch
Comment 8 Radu Stavila 2014-03-05 07:01:51 PST
Comment on attachment 225878 [details]
Reviewed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225878&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:5292
>> +    IntSize scrolledContentOffset = namedFlowFragment->fragmentContainer().hasOverflowClip() ? namedFlowFragment->fragmentContainer().scrolledContentOffset() : IntSize();
> 
> Why use IntSize() and not always the scrolledContentOffset()? For unscrollable boxes it should be IntRect() anyway, right?

scrolledContentOffset() asserts that (hasOverflowClip == true)
Comment 9 Dave Hyatt 2014-03-05 13:12:08 PST
Comment on attachment 225880 [details]
Reviewed patch

This looks much nicer than previous approach.
Comment 10 WebKit Commit Bot 2014-03-05 13:57:13 PST
Comment on attachment 225880 [details]
Reviewed patch

Clearing flags on attachment: 225880

Committed r165130: <http://trac.webkit.org/changeset/165130>
Comment 11 WebKit Commit Bot 2014-03-05 13:57:16 PST
All reviewed patches have been landed.  Closing bug.