Bug 28064 – wrong calculation of overflow size for flexbox and table

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Shinichiro Hamaji]

RenderFlexibleBox and RenderTable has following code

    IntRect reflection(reflectionBox());
    m_overflowTop = min(m_overflowTop, reflection.y());
    m_overflowHeight = max(m_overflowHeight, reflection.bottom());
    m_overflowLeft = min(m_overflowLeft, reflection.x());
    m_overflowHeight = max(m_overflowWidth, reflection.right());

The last line is obviously wrong. I'll send a patch later.

[Shinichiro Hamaji]

Created attachment 34258 [details]
Patch v1


---
 9 files changed, 117 insertions(+), 62 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 34258 [details]
Patch v1

It is not immediately apparent to me what was wrong in the table code.  Please explain in your ChangeLog.

I like how the line spacing was where you copied this from:
+    style()->getBoxShadowExtent(shadowTop, shadowRight, shadowBottom, shadowLeft);
+    m_overflowLeft = min(m_overflowLeft, shadowLeft);

I would prefer there was a line between the shadow lookups and the first m_overflowLeft setting.

r- because your ChangeLog isn't clear enough (or my eyes aren't good enough) to understand easily what this does besides moving code.

This change looks GREAT!  But I need to know (and future people looking at your patch need to know) what is changed for Table/Flexbox.  A simple pointer in the ChangeLog to what the error in the old code was would suffice.

Thanks for doing this!  Such a fantastic cleanup. :)

[Shinichiro Hamaji]

Created attachment 34440 [details]
Patch v2


---
 9 files changed, 130 insertions(+), 62 deletions(-)

[Shinichiro Hamaji]

Done! Thanks for the review. As for RenderBlock style, I slightly preferred no spacing, but it was not strong preference. I'm 100% sure your preference should be closer to webkit's preference :) and I added a vertical space. I also added some descriptions in ChangeLog. Sorry for poor descriptions before. Thanks!

[Eric Seidel (no email)]

Comment on attachment 34440 [details]
Patch v2

Fantastic!

[Eric Seidel (no email)]

Comment on attachment 34440 [details]
Patch v2

Rejecting patch 34440 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/run-webkit-tests --no-launch-safari --quiet failed with exit code 1

[Eric Seidel (no email)]

transitions/transition-end-event-transform.html -> failed
Not sure if it was related to your patch or not.

[Shinichiro Hamaji]

(In reply to comment #7)
> transitions/transition-end-event-transform.html -> failed
> Not sure if it was related to your patch or not.

Hmm... I don't see this failure.

[Eric Seidel (no email)]

Comment on attachment 34440 [details]
Patch v2

It's possible my local test run was simply flakey.  I"ll add it back to the queue.

[Eric Seidel (no email)]

Comment on attachment 34440 [details]
Patch v2

Clearing flags on attachment: 34440

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/reflections/reflection-overflow-scroll-expected.txt
	M	LayoutTests/fast/reflections/reflection-overflow-scroll.html
	M	LayoutTests/fast/reflections/resources/reflection-overflow-scroll.js
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderBlock.h
	M	WebCore/rendering/RenderFlexibleBox.cpp
	M	WebCore/rendering/RenderTable.cpp
Committed r47200
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderFlexibleBox.cpp
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderBlock.h
	M	WebCore/rendering/RenderTable.cpp
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/reflections/reflection-overflow-scroll-expected.txt
	M	LayoutTests/fast/reflections/reflection-overflow-scroll.html
	M	LayoutTests/fast/reflections/resources/reflection-overflow-scroll.js
r47200 = df611cf32e24948c658a71bf104e67f8eaf2971a (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47200

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.