Bug 129032

Created attachment 224602 [details] Repro

Created attachment 225250 [details] Patch

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.

Created attachment 225651 [details] Patch v2

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.

Created attachment 225654 [details] Patch for landing

Comment on attachment 225654 [details] Patch for landing Clearing flags on attachment: 225654 Committed r164988: <http://trac.webkit.org/changeset/164988>

All reviewed patches have been landed. Closing bug.