RESOLVED FIXED 129032
[CSS Regions] Overset computation is incorrect in some cases
https://bugs.webkit.org/show_bug.cgi?id=129032
Summary [CSS Regions] Overset computation is incorrect in some cases
Andrei Bucur
Reported 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.
Attachments
Repro (903 bytes, text/html)
2014-02-19 00:32 PST, Andrei Bucur
no flags
Patch (32.42 KB, patch)
2014-02-26 06:54 PST, Andrei Bucur
no flags
Patch v2 (32.97 KB, patch)
2014-03-03 07:53 PST, Andrei Bucur
no flags
Patch for landing (32.92 KB, patch)
2014-03-03 08:26 PST, Andrei Bucur
no flags
Andrei Bucur
Comment 1 2014-02-19 00:32:42 PST
Andrei Bucur
Comment 2 2014-02-26 06:54:14 PST
Mihnea Ovidenie
Comment 3 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.
Andrei Bucur
Comment 4 2014-03-03 07:53:31 PST
Created attachment 225651 [details] Patch v2
Mihnea Ovidenie
Comment 5 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.
Andrei Bucur
Comment 6 2014-03-03 08:26:33 PST
Created attachment 225654 [details] Patch for landing
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2014-03-03 09:10:34 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.