Bug 25406 – -webkit-box-orient:horizontal doesn't work on <button> tag

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

[Aaron Boodman]

<button> tag seems to always use -webkit-box-orient:vertical, no matter what the style rule is.

With this code:

<style type="text/css">
#a {
  display:-webkit-box;
  -webkit-box-orient:horizontal;
}
span {
  display:-webkit-box;
}
</style>
<button id="a"><span>hello</span><span>world</span></button>


In Safari 3.1 on PC, the two spans are stacked vertically, but they should be aligned horizontally. Changing the <button> tag to a <div> and updating the CSS fixes the problem.

[Shinichiro Hamaji]

Created attachment 31913 [details]
Patch v1

 LayoutTests/ChangeLog                              |   15 +++++++
 LayoutTests/fast/flexbox/box-orient-button.html    |   26 ++++++++++++
 .../flexbox/box-orient-button-expected.checksum    |    1 +
 .../fast/flexbox/box-orient-button-expected.png    |  Bin 0 -> 21141 bytes
 .../fast/flexbox/box-orient-button-expected.txt    |   44 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   20 +++++++++
 WebCore/rendering/RenderBlock.cpp                  |   14 +++++-
 WebCore/rendering/RenderBlock.h                    |    2 +-
 WebCore/rendering/RenderButton.cpp                 |    3 +-
 WebCore/rendering/RenderFlexibleBox.cpp            |    2 +
 10 files changed, 122 insertions(+), 5 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31913 [details]
Patch v1

This patch would fix this bug. The problem was that RenderButton's anonymous children were always RenderBlock, but I think it should be RenderFlexibleBox when the RenderButton is flexible.

We may need (at least) one change to make this patch perfect. As box-orient is not inherited with current WebKit (the current spec says box-orinet should be inherited), the anonymous flexible box doesn't use RenderButton's box-orient. So, with this patch, even if we specifies box-orient:vertical for <button>, it doesn't layout the inner blocks vertically. I filed another bug and sent a patch for this issue: https://bugs.webkit.org/show_bug.cgi?id=26717

So, I wait the progress of Bug 26717 and I don't mark this patch as '?' for now.

[Shinichiro Hamaji]

Created attachment 32520 [details]
Patch v2

As it turned out that box-orient should not be inherited, I modified the first patch so that the flexible button passes its box-orient to its anonymous child.

[Eric Seidel (no email)]

Hyatt is your ideal reviewer here.

Have you compared your test with Firefox's behavior by using -moz- instead of WebKit for the CSS?  I don't really know enough about flexbox at this moment to say whether your change is right or wrong, but Hyatt would.

[Aaron Boodman]

Does Mozilla implement any of this? I thought not.

[Shinichiro Hamaji]

Mozilla is also incorrect in opposite way. It always use -webkit-box-orient:horizontal. I've created an HTML which contains -(webkit |moz)-box-orient:(horizontal|vertical) .

http://tinyurl.com/n2nnyy

And thanks Eric for suggestion. I'll ask Hyatt to check my patch.

[Eric Seidel (no email)]

Comment on attachment 32520 [details]
Patch v2

In general this looks fine.

Is it possible to make a dumpAsText test for this instead?

Should "isBox" be isFlexibleBox?

Why shouldn't this check be moved inside createAnonymousBlock?
 55         bool isBox = style()->display() == BOX || style()->display() == INLINE_BOX;

r- for now, pending comments.  Feel free to set it back to r=? when you answer! :)

[Shinichiro Hamaji]

Created attachment 34445 [details]
Patch v3


---
 11 files changed, 193 insertions(+), 5 deletions(-)

[Shinichiro Hamaji]

> Is it possible to make a dumpAsText test for this instead?

Yes, it's possible. Thanks for the good catch!

> Should "isBox" be isFlexibleBox?

Agreed.

> Why shouldn't this check be moved inside createAnonymousBlock?
>  55         bool isBox = style()->display() == BOX || style()->display() ==
> INLINE_BOX;

RenderBlock::addChild() creates an anonymous child and call addChild() again using the anonymous child when m_childrenInline is true. As RenderFlexibleBox's m_childrenInline is always true, it means that creating an anonymous flexible child causes infinite recursion (when addChild is called for a flexible box, it creates an anonymous box, which is flexible, and it calls addChild for the anonymous child. Then, the anonymous flexible child creates another anonymous child and calls addChild again...). So, I think we need to control flexible-ness of anonymous children in caller.

Thanks for the review. I hope my change looks better.

[Eric Seidel (no email)]

Comment on attachment 34445 [details]
Patch v3

It's kinda evil... but really useful: webkit supports the window['id'] quirk that IE implements.  So you could remove all of these lines:
8 var referenceHorizontalButton = document.getElementById("reference_horizontal");
 9 var referenceVerticalButton = document.getElementById("reference_vertical");
 10 var defaultButton = document.getElementById("default");
 11 var horizontalButton = document.getElementById("horizontal");
 12 var verticalButton = document.getElementById("vertical");
 13 var toVerticalButton = document.getElementById("toVertical");
 14 var toHorizontalButton = document.getElementById("toHorizontal");

basically:
// <span id='foo'>
foo == getElementById('foo')
evaluates true in WebKIt and IE.  I'm nto sure about firefox.

Sometimes peopel also define a:
gebi() function to make things shorter:

function gebi(id) {
   return document.getElementById(id);
}

Technically a style violation:
 if (window.layoutTestController) {
 29     document.getElementById("main").innerHTML = "";
 30 }

Looks OK.

[Eric Seidel (no email)]

Comment on attachment 34445 [details]
Patch v3

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

Patch https://bugs.webkit.org/attachment.cgi?id=34445 from bug 25406 failed to download and apply.

[Eric Seidel (no email)]

make-js-test-wrappers has been renamed, invalidating this patch. :(

[Shinichiro Hamaji]

Created attachment 35010 [details]
Patch v4


---
 11 files changed, 189 insertions(+), 5 deletions(-)

[Shinichiro Hamaji]

(In reply to comment #12)
> make-js-test-wrappers has been renamed, invalidating this patch. :(

Oops. I updated my patch so that there should be no conflict. I also fixed style issues in my layout test. I prefer gebi() solution as it will run even on Firefox (I confirmed that Firefox doesn't support the evil window['id']).

[Eric Seidel (no email)]

Comment on attachment 35010 [details]
Patch v4

Technically a style violation:
 8 function gebi(id) {

Looks fine though.

[Eric Seidel (no email)]

Comment on attachment 35010 [details]
Patch v4

Clearing flags on attachment: 35010

Committed r47418: <http://trac.webkit.org/changeset/47418>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.