| Summary: | wrong calculation of overflow size for flexbox and table | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Shinichiro Hamaji <hamaji> | ||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | eric | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
Created attachment 34258 [details]
Patch v1
---
9 files changed, 117 insertions(+), 62 deletions(-)
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. :)
Created attachment 34440 [details]
Patch v2
---
9 files changed, 130 insertions(+), 62 deletions(-)
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! Comment on attachment 34440 [details]
Patch v2
Fantastic!
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
transitions/transition-end-event-transform.html -> failed Not sure if it was related to your patch or not. (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. 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.
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 All reviewed patches have been landed. Closing bug. |
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.