Bug 15135 – REGRESSION (r19843-r19850): Changing a flexbox's contents makes its container scroll to the top

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

To reproduce: open the URL and click the Test button. The overflow area should not scroll to the top.

<http://trac.webkit.org/projects/webkit/changeset/19848> is the likely suspect for this.

[Shinichiro Hamaji]

Created attachment 31156 [details]
Patch v1

 LayoutTests/ChangeLog                         |   15 ++++
 LayoutTests/fast/flexbox/repaint-expected.txt |   20 ++++++
 LayoutTests/fast/flexbox/repaint.html         |   18 +++++
 LayoutTests/fast/flexbox/resources/repaint.js |    9 +++
 WebCore/ChangeLog                             |   24 +++++++
 WebCore/rendering/RenderBlock.cpp             |   92 ++++++++++++++-----------
 WebCore/rendering/RenderBlock.h               |   14 ++++
 WebCore/rendering/RenderFlexibleBox.cpp       |   40 ++++++++++-
 8 files changed, 190 insertions(+), 42 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31156 [details]
Patch v1

I'm not sure if this is the best fix for this issue. I think code around flexible box may need to be refactored. If reviewer considers we should fix this bug after some refactoring, please let me know. If so, I'll withdraw this patch and try refactoring if I can. Anyway, any kind of comments are really appreciated.

Thanks!

[Dave Hyatt]

Comment on attachment 31156 [details]
Patch v1

You cannot add any member variables to RenderBlock, as this will significantly bloat the size of the render tree for real world HTML.  Doing so just to fix flexbox bugs is not a good real-world tradeoff.

I believe you can maybe just set a flag to avoid doing the work you refactored into repaintBlockAfterLayout in the case where an object is going to do double layouts because of flexing.

[Dave Hyatt]

Note that in the case of a single flexing object, I imagine you could eliminate the double layouts completely.

[Shinichiro Hamaji]

Created attachment 31283 [details]
Patch v2

 LayoutTests/ChangeLog                              |   15 +++
 .../fast/flexbox/repaint-scrollbar-expected.html   |   20 ++++
 LayoutTests/fast/flexbox/repaint-scrollbar.html    |   18 +++
 .../fast/flexbox/resources/repaint-scrollbar.js    |    9 ++
 WebCore/ChangeLog                                  |   24 ++++
 WebCore/rendering/RenderBlock.cpp                  |  114 +++++++++++++-------
 WebCore/rendering/RenderBlock.h                    |   25 +++++
 WebCore/rendering/RenderFlexibleBox.cpp            |   40 +++++++-
 8 files changed, 223 insertions(+), 42 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31283 [details]
Patch v2

Thanks for the review!

I understood that we cannot add members into RenderBlock. As the delayed repaint needs some contexts of layout (repainter, repaintBottom and repaintTop), I created a hash map which holds the context information and is used only when a flexbox is layouting its children. This hack looks not so good, but it seems that this kind of technique is often used for flexible box support (e.g., gOverrideSizeMap in RenderBox.cpp) and this won't consume extra RAM for HTMLs which have no flexboxes.

Could you take a look again?

Thanks!

[Shinichiro Hamaji]

Created attachment 31284 [details]
Patch v3

 LayoutTests/ChangeLog                              |   15 +++
 .../fast/flexbox/repaint-scrollbar-expected.html   |   20 ++++
 LayoutTests/fast/flexbox/repaint-scrollbar.html    |   18 +++
 .../fast/flexbox/resources/repaint-scrollbar.js    |    9 ++
 WebCore/ChangeLog                                  |   24 ++++
 WebCore/rendering/RenderBlock.cpp                  |  114 +++++++++++++-------
 WebCore/rendering/RenderBlock.h                    |   25 +++++
 WebCore/rendering/RenderFlexibleBox.cpp            |   40 +++++++-
 8 files changed, 223 insertions(+), 42 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31284 [details]
Patch v3

Sorry, I changed the name of layout tests but I forgot to update ChangeLog as well.

[Shinichiro Hamaji]

Also, for now, I'd like not to eliminate double layouts for the case of a single flexing object as this change may introduce additional complexity to my patch.

[Dave Hyatt]

Comment on attachment 31284 [details]
Patch v3

You've picked a very challenging bug to work on here.

I'm not sure I'd worry so much about repaint being wrong.  If the flexing block has kids that move during the first layout, your patch isn't going to help that case.  They are still going to repaint too much.  I think you should just concentrate on the scrolling issue and ignore repaint for now.

As for scrolling, trying to stop just the immediate child block from updating its scroll info during the first layout doesn't really generally solve the problem.  What about a scrollable object inside a flexing block that incorrectly adjusts its scroll position in response to the first layout?

I think a simpler fix for this bug is to just make the scroll position updating code immune to flex box layout thrashing.

[Shinichiro Hamaji]

Created attachment 31536 [details]
Patch v4

 LayoutTests/ChangeLog                              |   15 ++
 .../fast/flexbox/repaint-scrollbar-expected.txt    |   44 ++++++
 LayoutTests/fast/flexbox/repaint-scrollbar.html    |   37 +++++
 .../fast/flexbox/resources/repaint-scrollbar.js    |   25 ++++
 WebCore/ChangeLog                                  |   24 +++
 WebCore/rendering/RenderBlock.cpp                  |  152 +++++++++++++++-----
 WebCore/rendering/RenderBlock.h                    |   11 ++
 WebCore/rendering/RenderFlexibleBox.cpp            |   12 ++-
 WebCore/rendering/bidi.cpp                         |    4 +-
 9 files changed, 281 insertions(+), 43 deletions(-)

[Shinichiro Hamaji]

(In reply to comment #10)
> (From update of attachment 31284 [details] [review])
> You've picked a very challenging bug to work on here.

Yeah, it was more difficult than I expected :( Sorry and thanks for your time to check my wrong patches.

> I'm not sure I'd worry so much about repaint being wrong.  If the flexing block
> has kids that move during the first layout, your patch isn't going to help that
> case.  They are still going to repaint too much.  I think you should just
> concentrate on the scrolling issue and ignore repaint for now.
> 
> As for scrolling, trying to stop just the immediate child block from updating
> its scroll info during the first layout doesn't really generally solve the
> problem.  What about a scrollable object inside a flexing block that
> incorrectly adjusts its scroll position in response to the first layout?

Ah yes, I didn't care about grandchild nodes at all.

> I think a simpler fix for this bug is to just make the scroll position updating
> code immune to flex box layout thrashing.

I'm not sure if I understand what you are suggesting here correctly. I tried two ways to fix only scrollbar, but both attempts failed so far :( I tried

- Just disable RenderLayer::updateScrollInfoAfterLayout() while layouting descendants of flexible box: Scrollbars are never painted. Maybe updateScrollInfoAfterLayout() is painting as well as updating?
- Disable updateScrollInfoAfterLayout(), and after layout descendants of the flexible box, call updateScrollInfoAfterLayout() for delayed objects: This means that we update scrollbar after repainting. This change seems to break some existing layout tests.

I also tried to create a patch which fixes both update of scrollbar and repainting (the uploaded patch), and it seems to be working. I also added layout tests for nested flexible boxes, which my previous patch didn't handle properly.

In this change, we skip all update of scrollbar and repaint during layout of flexible boxes, and keep the skipped objects and contexts into global-static hashmap (so that we don't increase the size of RenderBlock). Then, after the layout of descendants of a flexible box finishes, we process all delayed repainting. When we are layouting nested flexible boxes, we don't process the delayed repainting until the first flexible finishes to layout its children.

If there are something I'm still missing, and/or you still think I should concentrate on the scrolling, I'm happy to do so, of course. Please let me know in this case.

Thanks!

[Shinichiro Hamaji]

Created attachment 31777 [details]
Patch v5

 .../fast/flexbox/repaint-scrollbar-expected.txt    |   44 +++++++++++++++++++
 LayoutTests/fast/flexbox/repaint-scrollbar.html    |   37 ++++++++++++++++
 .../fast/flexbox/resources/repaint-scrollbar.js    |   25 +++++++++++
 WebCore/rendering/RenderBlock.cpp                  |   46 ++++++++++++++++++--
 WebCore/rendering/RenderBlock.h                    |   10 ++++
 WebCore/rendering/RenderFlexibleBox.cpp            |   12 ++++-
 6 files changed, 168 insertions(+), 6 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31777 [details]
Patch v5

Sorry, delaying RenderLayer::updateScrollInfoAfterLayout() was working, actually. I introduced a bug when I tried first. I also noticed that my previous patch won't work for some nested cases as it may change the order of painting. So, I uploaded another patch, which only fixes the scrollbar issue as you suggested.

[Shinichiro Hamaji]

Created attachment 31834 [details]
Patch v6

 LayoutTests/ChangeLog                              |   17 +++++++
 .../fast/flexbox/repaint-scrollbar-expected.txt    |   44 +++++++++++++++++++
 LayoutTests/fast/flexbox/repaint-scrollbar.html    |   37 ++++++++++++++++
 .../fast/flexbox/resources/repaint-scrollbar.js    |   25 +++++++++++
 WebCore/ChangeLog                                  |   26 +++++++++++
 WebCore/rendering/RenderBlock.cpp                  |   46 ++++++++++++++++++--
 WebCore/rendering/RenderBlock.h                    |   10 ++++
 WebCore/rendering/RenderFlexibleBox.cpp            |   12 ++++-
 8 files changed, 211 insertions(+), 6 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31834 [details]
Patch v6

I forgot to add ChangeLogs in the previous patch.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/flexbox/repaint-scrollbar-expected.txt
	A	LayoutTests/fast/flexbox/repaint-scrollbar.html
	A	LayoutTests/fast/flexbox/resources/repaint-scrollbar.js
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderBlock.h
	M	WebCore/rendering/RenderFlexibleBox.cpp
Committed r45568
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderFlexibleBox.cpp
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderBlock.h
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/flexbox/repaint-scrollbar-expected.txt
	A	LayoutTests/fast/flexbox/repaint-scrollbar.html
	A	LayoutTests/fast/flexbox/resources/repaint-scrollbar.js
r45568 = 52339b5a0ebb8366f59f26c4b326f14f225fb789 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/45568