Bug 129032 - [CSS Regions] Overset computation is incorrect in some cases
Summary: [CSS Regions] Overset computation is incorrect in some cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2014-02-19 00:30 PST by Andrei Bucur
Modified: 2014-03-03 09:10 PST (History)
7 users (show)

See Also:


Attachments
Repro (903 bytes, text/html)
2014-02-19 00:32 PST, Andrei Bucur
no flags Details
Patch (32.42 KB, patch)
2014-02-26 06:54 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch v2 (32.97 KB, patch)
2014-03-03 07:53 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch for landing (32.92 KB, patch)
2014-03-03 08:26 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Bucur 2014-02-19 00:30:31 PST
Overset is incorrectly computed for regions with auto-height (by taking into account visual overflow). See the attached test case.
Comment 1 Andrei Bucur 2014-02-19 00:32:42 PST
Created attachment 224602 [details]
Repro
Comment 2 Andrei Bucur 2014-02-26 06:54:14 PST
Created attachment 225250 [details]
Patch
Comment 3 Mihnea Ovidenie 2014-03-03 02:19:42 PST
Comment on attachment 225250 [details]
Patch

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

r-
I like the patch and i think is moving in the right direction.However, i would like you to take another look at it.
As a comment, i would have probably created another patch before with the movement of regionOversetChange methods from RenderRegion to RenderNamedFlowFragment, without functionality change, making the review easier.

> Source/WebCore/ChangeLog:10
> +        1. Regions overflow no longer trigger an overset changed event. This is beacuse

beacuse -> because

> Source/WebCore/ChangeLog:25
> +

5. The regionOverset property is moved from RenderRegion to RenderNamedFlowFragment.

> Source/WebCore/dom/WebKitNamedFlow.cpp:77
> +    if (namedFlowFragment->generatingElement()->regionOversetState() == RegionOverset)

No need for exposing the generatingElement() here. If you write something like:
return (namedFlowFragment->regionOversetState() == RegionOverset)
you achieve the same result since the method you moved to RenderNamedFlowFragment, regionOversetState() has the assertion in place. I also prefer using a ternary operator here.

> Source/WebCore/dom/WebKitNamedFlow.cpp:119
> +        if (namedFlowFragment->generatingElement()->regionOversetState() == RegionEmpty)

No need for assert here if you write:
if (namedFlowFragment->regionOversetState() == RegionEmpty)

> Source/WebCore/rendering/FlowThreadController.cpp:249
> +        flowRenderer->dispatchRegionEvents();

Worth a comment in the changelog explaining why you chose this layout phase for dispatching the event(s).

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:307
> +void RenderNamedFlowThread::dispatchRegionEvents()

This deserves a comment in the changelog since you are talking about events, while right now you are using this function to dispatch only the regions overset change event. I assume you want this function to be used also when we support the regionFragmentChange.
Btw, i would rename this function to dispatchRegionChainEvents or dispatchNamedFlowEvents since you are not dispatching an event for a region but rather you are dispatching the events for the named flow.

I also think it is worth explaining in the ChangeLog why regionLayoutUpdate is not triggered here, in the same place as regionOversetChange.

> Source/WebCore/rendering/RenderNamedFlowThread.h:153
> +    // Used to store the content bottom until the moment we need to update the overset state.

I would move this comment into the changelog.

> LayoutTests/fast/regions/cssom/webkit-named-flow-overset.html:62
> +    // Overset should be false since the shadow of the content overflows the last region.

Maybe we can make this comment clearer. Something like:
"Overset should be false even if the content shadow overflows the last region since the visual overflow does not influence the overset property.
Comment 4 Andrei Bucur 2014-03-03 07:53:31 PST
Created attachment 225651 [details]
Patch v2
Comment 5 Mihnea Ovidenie 2014-03-03 08:10:33 PST
Comment on attachment 225651 [details]
Patch v2

r=me but please make sure that you have an accurate ChangeLog before landing. For instance, Changelog mentions firstEmptyRegionIndex which was modified in the previous patch but not in this one.
Comment 6 Andrei Bucur 2014-03-03 08:26:33 PST
Created attachment 225654 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2014-03-03 09:10:28 PST
Comment on attachment 225654 [details]
Patch for landing

Clearing flags on attachment: 225654

Committed r164988: <http://trac.webkit.org/changeset/164988>
Comment 8 WebKit Commit Bot 2014-03-03 09:10:34 PST
All reviewed patches have been landed.  Closing bug.