Bug 25406

Summary: -webkit-box-orient:horizontal doesn't work on <button> tag
Product: WebKit Reporter: Aaron Boodman <aa>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hamaji, hyatt
Priority: P2    
Version: 525.x (Safari 3.1)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3
none
Patch v4 none

Description Aaron Boodman 2009-04-25 23:09:12 PDT
<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.
Comment 1 Shinichiro Hamaji 2009-06-25 23:46:21 PDT
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(-)
Comment 2 Shinichiro Hamaji 2009-06-26 00:08:58 PDT
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.
Comment 3 Shinichiro Hamaji 2009-07-09 09:52:13 PDT
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.
Comment 4 Eric Seidel (no email) 2009-07-15 16:20:33 PDT
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.
Comment 5 Aaron Boodman 2009-07-15 16:24:07 PDT
Does Mozilla implement any of this? I thought not.
Comment 6 Shinichiro Hamaji 2009-07-15 23:05:02 PDT
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.
Comment 7 Eric Seidel (no email) 2009-08-07 09:32:00 PDT
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! :)
Comment 8 Shinichiro Hamaji 2009-08-10 03:57:07 PDT
Created attachment 34445 [details]
Patch v3


---
 11 files changed, 193 insertions(+), 5 deletions(-)
Comment 9 Shinichiro Hamaji 2009-08-10 04:07:35 PDT
> 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.
Comment 10 Eric Seidel (no email) 2009-08-17 18:19:20 PDT
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.
Comment 11 Eric Seidel (no email) 2009-08-17 18:22:19 PDT
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.
Comment 12 Eric Seidel (no email) 2009-08-17 18:23:24 PDT
make-js-test-wrappers has been renamed, invalidating this patch. :(
Comment 13 Shinichiro Hamaji 2009-08-17 19:25:07 PDT
Created attachment 35010 [details]
Patch v4


---
 11 files changed, 189 insertions(+), 5 deletions(-)
Comment 14 Shinichiro Hamaji 2009-08-17 19:29:51 PDT
(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']).
Comment 15 Eric Seidel (no email) 2009-08-17 22:39:55 PDT
Comment on attachment 35010 [details]
Patch v4

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

Looks fine though.
Comment 16 Eric Seidel (no email) 2009-08-17 23:27:03 PDT
Comment on attachment 35010 [details]
Patch v4

Clearing flags on attachment: 35010

Committed r47418: <http://trac.webkit.org/changeset/47418>
Comment 17 Eric Seidel (no email) 2009-08-17 23:27:08 PDT
All reviewed patches have been landed.  Closing bug.