Bug 109994 – Convert buttons from DeprecatedFlexBox to nondeprecated FlexibleBox

• Component: Forms
• Status: RESOLVED FIXED

[Christian Biesinger]

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).

[Christian Biesinger]

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

[Christian Biesinger]

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

[Christian Biesinger]

Created attachment 188685 [details]
Patch

[Ojan Vafai]

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.

[Christian Biesinger]

Created attachment 189169 [details]
Patch

[WebKit Review Bot]

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.

[Christian Biesinger]

I'm still looking into the 2px difference (looks like it's a positioning difference, not size). I fixed the other issues.

[Christian Biesinger]

Created attachment 189206 [details]
Patch

[Christian Biesinger]

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.

[WebKit Review Bot]

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

[Christian Biesinger]

Created attachment 189213 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Christian Biesinger]

Comment on attachment 189213 [details]
Patch

Errors seem unrelated

[WebKit Review Bot]

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

[Ojan Vafai]

(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.

[Christian Biesinger]

Hmm. http/tests/security/isolatedWorld/userGestureEvents is a timeout...

[Christian Biesinger]

bug 110389 helps with table-percent-height.html, but only partially...

[Christian Biesinger]

Oh I see. The remaining difference is because the button is empty, and that's a known difference with the patch.

[Christian Biesinger]

Created attachment 189409 [details]
Patch

[Christian Biesinger]

Comment on attachment 189409 [details]
Patch

Will request commit-queue once the dependent bugs are checked in.

[WebKit Review Bot]

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

[Christian Biesinger]

Created attachment 189566 [details]
Patch

[Christian Biesinger]

Comment on attachment 189566 [details]
Patch

Rebased. Should apply now.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 189566 [details]
Patch

Clearing flags on attachment: 189566

Committed r143643: <http://trac.webkit.org/changeset/143643>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Christian Biesinger]

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.