Summary: | Remove overflow: scroll handling in block flow layout methods | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | eric, ojan, simon.fraser, tony, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
Description
Julien Chaffraix
2012-07-30 15:48:10 PDT
Created attachment 155391 [details]
Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code.
I like the idea of this change. I don't know much about RenderLayer. Is there one per RenderBlock? If not, don't we need to update child RenderBlocks? Also, does updateScrollbarsAfterStyleChange run before layout? Comment on attachment 155391 [details] Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code. View in context: https://bugs.webkit.org/attachment.cgi?id=155391&action=review > Source/WebCore/rendering/RenderLayer.cpp:2530 > + if (UNLIKELY(box->style()->appearance() == ListboxPart)) I don't think UNLIKELY is useful here. > Source/WebCore/rendering/RenderLayer.cpp:4847 > + if (UNLIKELY(box->style()->appearance() == ListboxPart)) Ditto. > Source/WebCore/rendering/RenderLayer.cpp:4856 > + if (!overflowCanHaveAScrollbar(overflowX)) > setHasHorizontalScrollbar(false); > - if (hasVerticalScrollbar() && !overflowCanHaveAScrollbar(overflowY)) > + else if (overflowRequiresAScrollbar(overflowX)) > + setHasHorizontalScrollbar(true); Having this if/else is confusing, since overflowCanHaveAScrollbar and overflowRequiresAScrollbar both consult the same EOverflow value. > I don't know much about RenderLayer. Is there one per RenderBlock? Not necessarily. First it's at most one per RenderBoxModelObject and it is allocated on style change if needed. If you have some overflow clip, you will have a RenderLayer (see RenderBox::requiresLayer()). > If not, don't we need to update child RenderBlocks? No need for that. > Also, does updateScrollbarsAfterStyleChange run before layout? updateScrollbarsAfterStyleChange is called during style change after the layer was created. If the style change required a layout, it will occur later (which should be the case for some 'overflow' transition as we need to relayout). Comment on attachment 155391 [details] Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code. View in context: https://bugs.webkit.org/attachment.cgi?id=155391&action=review >> Source/WebCore/rendering/RenderLayer.cpp:4856 >> + setHasHorizontalScrollbar(true); > > Having this if/else is confusing, since overflowCanHaveAScrollbar and overflowRequiresAScrollbar both consult the same EOverflow value. Thinking further, we can probably get away with: setHasHorizontalScrollbar(overflowRequiresAScrollbar(overflowX)); setHasVerticalScrollbar(overflowRequiresAScrollbar(overflowY)); This has the big downside of destroying the automatic scrollbar(s) during any style change - regardless of whether we actually changed 'overflow' and thus would likely force a relayout later. A better fix would be to be more conservative by keeping automatic scrollbars and forcing overflow: scroll ones. Let me think more about this. Created attachment 155409 [details]
Updated updateScrollbarsAfterStyleChange to be less confusing while keeping existing automatic scrollbars.
Comment on attachment 155409 [details] Updated updateScrollbarsAfterStyleChange to be less confusing while keeping existing automatic scrollbars. View in context: https://bugs.webkit.org/attachment.cgi?id=155409&action=review > Source/WebCore/rendering/RenderLayer.cpp:4829 > +static bool overflowRequiresAScrollbar(EOverflow overflow) Please call this overflowRequiresScrollbar(). Created attachment 155421 [details]
Patch for landing
Comment on attachment 155421 [details] Patch for landing Clearing flags on attachment: 155421 Committed r124168: <http://trac.webkit.org/changeset/124168> All reviewed patches have been landed. Closing bug. |