I have a patch mostly done. Working through tests now and using this bug to note where the patch changes behaviour and why that is OK (I will also copy that into the ChangeLog).
Created attachment 188679 [details] Testcase: Current Webkit: Inconsistent behaviour The two buttons should be the same size. They currently aren't (tested on Chromium/Linux). With my patch, they are both small (6px high), which matches Firefox behaviour and is self-consistent. LayoutTests/css2.1/20110323/replaced-elements-001.htm LayoutTests/fast/forms/button-generated-content.html LayoutTests/fast/forms/select-baseline.html LayoutTests/svg/custom/foreign-object-skew.svg
same reason for: LayoutTests/tables/mozilla/bugs/bug92647-2.html LayoutTests/fast/invalid/residual-style.html will be fixed by bug 109981 (button should be invisible). this change slightly moves the positioning of the button, so that test fails with my patch
Created attachment 188685 [details] Patch
Comment on attachment 188685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188685&action=review Looks great overall. Just a few trivial changes to the code. The new test baselines need a bit of explanation though (i.e. it's not obvious to my why some of the new results are correct). The collapsed button cases are fine since that matches Gecko, but the others are less clear to me. I can believe they're correct, it just needs an explanation of why they're changing in the ChangeLog. > Source/WebCore/rendering/RenderBlock.cpp:7896 > if (display == BOX || display == INLINE_BOX) { > newBox = RenderDeprecatedFlexibleBox::createAnonymous(parent->document()); > newDisplay = BOX; Can you add a FIXME to get rid of this part once we get rid of all internal RenderDeprecatedFlexibleBox uses? There should never be a future case where we want to create an anonymous deprecated flexbox. > Source/WebCore/rendering/RenderButton.cpp:-81 > // RenderBlock::setStyle is going to apply a new style to the inner block, which > - // will have the initial box flex value, 0. The current value is 1, because we set > + // will have the initial flex value, 0. The current value is 1, because we set > // it right below. Here we change it back to 0 to avoid getting a spurious layout hint > - // because of the difference. > - m_inner->style()->setBoxFlex(0); Ugh. Not your fault, but we really should fix this. This is a gross hack. Maybe add a FIXME while you're here? > Source/WebCore/rendering/RenderButton.cpp:84 > + m_inner->style()->setFlexGrow(0); > + m_inner->style()->setMinWidth(Length()); > + m_inner->style()->setMarginTop(Length(0, Fixed)); > + m_inner->style()->setMarginBottom(Length(0, Fixed)); Better to use initialValue() off of m_inner->style() or newStyle to avoid code duplication, not that the initialValue of these could realistically change. It almost makes it more clear what's going on. > Source/WebCore/rendering/RenderButton.cpp:116 > + // min-width: 0; is needed for correct shrinking > + innerStyle->setMinWidth(Length(0, Fixed)); Maybe add a FIXME to remove this (and the corresponding line in styleWillChange) once we change the min-width of flex-items back to 0 since the spec seems like it's going to be changing. > Source/WebCore/rendering/RenderButton.cpp:118 > + // we use margin: auto for correct centering. align-items: center > + // does not produce correct results when the content overflows. Instead of "correct", which is kind of vague, can you describe the actual behavior, e.g., // Use margin:auto instead of align-items:center to get safe centering, i.e. // when the content overflows, treat it the same as align-items: flex-start. > LayoutTests/ChangeLog:26 > + * fast/forms/button-generated-content-expected.txt: > + * platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.png: > + * platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.txt: > + * platform/chromium-linux/fast/forms/button-generated-content-expected.png: > + * platform/chromium-linux/fast/forms/button-generated-content-expected.txt: > + * platform/chromium-linux/fast/forms/select-baseline-expected.png: > + * platform/chromium-linux/fast/forms/select-baseline-expected.txt: > + * platform/chromium-linux/svg/custom/foreign-object-skew-expected.png: > + * platform/chromium-linux/svg/custom/foreign-object-skew-expected.txt: > + * platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.png: > + * platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.txt: > + * platform/mac/css2.1/20110323/replaced-elements-001-expected.txt: > + * platform/mac/fast/forms/select-baseline-expected.txt: > + * platform/mac/svg/custom/foreign-object-skew-expected.txt: Please add comments explaining why the results are different. For example, for button-generated-content, the collapsed buttons are no longer centered, but this matches Firefox behavior. Also, all the buttons seem to be 2px shorter. Is that correct? > LayoutTests/ChangeLog:29 > + have the height of the line. See the testcase in the bug. "the bug" is a bit confusing. Best to just link to the bug even though it'a already in the description title. Or, even better, just link to the test case itself. Also, please add that the new rendering matches Firefox.
Created attachment 189169 [details] Patch
Attachment 189169 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/flexbox/box-orient-button.html', u'LayoutTests/fast/flexbox/resources/box-orient-button.js', u'LayoutTests/fast/forms/button-generated-content-expected.txt', u'LayoutTests/platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.png', u'LayoutTests/platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.txt', u'LayoutTests/platform/chromium-linux/fast/forms/button-generated-content-expected.png', u'LayoutTests/platform/chromium-linux/fast/forms/button-generated-content-expected.txt', u'LayoutTests/platform/chromium-linux/fast/forms/select-baseline-expected.png', u'LayoutTests/platform/chromium-linux/fast/forms/select-baseline-expected.txt', u'LayoutTests/platform/chromium-linux/svg/custom/foreign-object-skew-expected.png', u'LayoutTests/platform/chromium-linux/svg/custom/foreign-object-skew-expected.txt', u'LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.png', u'LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.txt', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/css2.1/20110323/replaced-elements-001-expected.txt', u'LayoutTests/platform/mac/fast/forms/select-baseline-expected.txt', u'LayoutTests/platform/mac/svg/custom/foreign-object-skew-expected.txt', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/html.css', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderButton.cpp', u'Source/WebCore/rendering/RenderButton.h', u'Source/WebCore/rendering/RenderFlexibleBox.cpp', u'Source/WebCore/rendering/RenderFlexibleBox.h']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/forms/select-baseline-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
I'm still looking into the 2px difference (looks like it's a positioning difference, not size). I fixed the other issues.
Created attachment 189206 [details] Patch
Comment on attachment 189206 [details] Patch We believe the 2px difference is because I ran the mac tests on mountain lion. So, in this version of the patch, I just added the tests to mac/TestExpectations so they can be rebaselined on the correct mac version.
Comment on attachment 189206 [details] Patch Rejecting attachment 189206 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 189206, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: youtTests/platform/gtk/TestExpectations Hunk #1 succeeded at 1465 (offset 6 lines). patching file LayoutTests/platform/mac/TestExpectations Hunk #1 succeeded at 1430 (offset 6 lines). patching file LayoutTests/platform/qt/TestExpectations Hunk #1 succeeded at 2653 (offset 6 lines). patching file LayoutTests/platform/win/TestExpectations Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Ojan Vafai']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16637128
Created attachment 189213 [details] Patch
Attachment 189213 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/flexbox/box-orient-button.html', u'LayoutTests/fast/flexbox/resources/box-orient-button.js', u'LayoutTests/platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.png', u'LayoutTests/platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.txt', u'LayoutTests/platform/chromium-linux/fast/forms/button-generated-content-expected.png', u'LayoutTests/platform/chromium-linux/fast/forms/button-generated-content-expected.txt', u'LayoutTests/platform/chromium-linux/fast/forms/select-baseline-expected.png', u'LayoutTests/platform/chromium-linux/fast/forms/select-baseline-expected.txt', u'LayoutTests/platform/chromium-linux/svg/custom/foreign-object-skew-expected.png', u'LayoutTests/platform/chromium-linux/svg/custom/foreign-object-skew-expected.txt', u'LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.png', u'LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.txt', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/html.css', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderButton.cpp', u'Source/WebCore/rendering/RenderButton.h', u'Source/WebCore/rendering/RenderFlexibleBox.cpp', u'Source/WebCore/rendering/RenderFlexibleBox.h']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/forms/select-baseline-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 189213 [details] Patch Attachment 189213 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16638141 New failing tests: http/tests/security/isolatedWorld/userGestureEvents.html fast/replaced/table-percent-height.html
Comment on attachment 189213 [details] Patch Errors seem unrelated
Comment on attachment 189213 [details] Patch Rejecting attachment 189213 [details] from commit-queue. New failing tests: http/tests/security/isolatedWorld/userGestureEvents.html fast/replaced/table-percent-height.html Full output: http://queues.webkit.org/results/16658013
(In reply to comment #13) > New failing tests: > http/tests/security/isolatedWorld/userGestureEvents.html > fast/replaced/table-percent-height.html These tests both do have <button>s, so, these might be real failures.
Hmm. http/tests/security/isolatedWorld/userGestureEvents is a timeout...
bug 110389 helps with table-percent-height.html, but only partially...
Oh I see. The remaining difference is because the button is empty, and that's a known difference with the patch.
Created attachment 189409 [details] Patch
Comment on attachment 189409 [details] Patch Will request commit-queue once the dependent bugs are checked in.
Comment on attachment 189409 [details] Patch Rejecting attachment 189409 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 189409, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: xpectations Hunk #1 succeeded at 1469 with fuzz 2 (offset 4 lines). patching file LayoutTests/platform/mac/TestExpectations Hunk #1 succeeded at 1428 (offset -2 lines). patching file LayoutTests/platform/qt/TestExpectations Hunk #1 succeeded at 2654 (offset 1 line). patching file LayoutTests/platform/win/TestExpectations Hunk #1 succeeded at 2642 (offset 3 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16647798
Created attachment 189566 [details] Patch
Comment on attachment 189566 [details] Patch Rebased. Should apply now.
Comment on attachment 189566 [details] Patch Rejecting attachment 189566 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189566, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Full output: http://queues.webkit.org/results/16696252
Comment on attachment 189566 [details] Patch Clearing flags on attachment: 189566 Committed r143643: <http://trac.webkit.org/changeset/143643>
All reviewed patches have been landed. Closing bug.
Adding WebExposed keyword, because old-flexbox CSS properties (like -webkit-box-orient) will no longer work on <button>. Instead, new-flexbox properties like -webkit-flex-direction will have to be used.