|Summary:||[CSS Regions] Overset computation is incorrect in some cases|
|Product:||WebKit||Reporter:||Andrei Bucur <abucur>|
|Component:||WebCore Misc.||Assignee:||Andrei Bucur <abucur>|
|Severity:||Normal||CC:||cmarcelo, commit-queue, esprehn+autocc, glenn, kangil.han, kondapallykalyan, WebkitBugTracker|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Andrei Bucur 2014-02-19 00:30:31 PST
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.