Bug 17918 - -webkit-box-align stretch does not work on replaced content
Summary: -webkit-box-align stretch does not work on replaced content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-03-18 11:53 PDT by Erik Arvidsson
Modified: 2010-08-11 00:18 PDT (History)
6 users (show)

See Also:


Attachments
Tests -webkit-box-align: stretch (752 bytes, text/html)
2008-03-18 11:54 PDT, Erik Arvidsson
no flags Details
Updated test case that tests, button, textarea, input and spans (1.33 KB, text/html)
2008-03-18 13:16 PDT, Erik Arvidsson
no flags Details
Proposed Patch (6.67 KB, patch)
2010-05-10 02:30 PDT, Yoshiki Hayashi
no flags Details | Formatted Diff | Diff
Proposed Patch 2 (7.79 KB, patch)
2010-05-11 01:52 PDT, Yoshiki Hayashi
no flags Details | Formatted Diff | Diff
Proposed Patch 3 (4.62 KB, patch)
2010-05-13 00:54 PDT, Yoshiki Hayashi
no flags Details | Formatted Diff | Diff
Proposed Patch 4 (7.86 KB, patch)
2010-05-13 02:43 PDT, Yoshiki Hayashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2008-03-18 11:53:13 PDT
I have -webkit-box-align set to stretch but it does not seem to work. start, end and center seems to work fine though. Test case coming
Comment 1 Erik Arvidsson 2008-03-18 11:54:10 PDT
Created attachment 19867 [details]
Tests -webkit-box-align: stretch

The buttons should be 100% of the width of the div
Comment 2 Dave Hyatt 2008-03-18 12:02:35 PDT
I suspect this is a bug with <button> rather than box stretch not working in general.

Comment 3 Erik Arvidsson 2008-03-18 13:15:54 PDT
I suspect it is a bug with any replaced content since it happens for textareas and inputs as well.
Comment 4 Erik Arvidsson 2008-03-18 13:16:45 PDT
Created attachment 19868 [details]
Updated test case that tests, button, textarea, input and spans
Comment 5 Yoshiki Hayashi 2010-05-10 02:29:41 PDT
I investigated this bug and found that the root cause seems to be that CSSStyleSelector::adjustRenderStyle in WebCore/css/CSSStyleSelector.cpp changes width: auto to mean width: intrinsic for these tags (button, input, select and textarea).  Changing those to be kept as width: auto fixes this bug and doesn't cause any regression as far as I can see so I'm going to attach that patch to this issue.

It looks like the change was made in http://trac.webkit.org/changeset/10662 by David, so I believe he can shoot my patch down quickly if I'm doing something completely stupid.

Alternative solution I could see is to treat width: intrinsic of button, input, select or textarea in flexbox as width: auto.  That looks uglier so I didn't implement it that way but if it's the desired way or there's some other way I should formulate my patch, I could implement it.
Comment 6 Yoshiki Hayashi 2010-05-10 02:30:47 PDT
Created attachment 55530 [details]
Proposed Patch
Comment 7 Yoshiki Hayashi 2010-05-11 01:51:03 PDT
Shinichiro (hamaji@chromium.org) pointed it out to me that treating width: auto as is for form controls for display: block isn't in sync with other browsers like Firefox, Opera and Internet Explorer and as far as I can see, the spec doesn't demand it either way, so I created a new patch that keeps the current width: auto -> width: intrinsic behavior outside of flexbox and only stretches vertically when text controls are in the vertical flex box with stretch specified as box-align.

Since I couldn't come up with any better way, the new patch moves width: auto -> width: intrinsic conversion from CSSStyleSelector::adjustRenderStyle to RenderBox::sizesToIntrinsicWidth.
Comment 8 Yoshiki Hayashi 2010-05-11 01:52:11 PDT
Created attachment 55678 [details]
Proposed Patch 2
Comment 9 Shinichiro Hamaji 2010-05-11 03:43:36 PDT
This change broke the following 3 tests on Mac:

fast/forms/003.html
fast/forms/select-block-background.html
fast/text/drawBidiText.html

I guess some code for <select> needs to check sizesToIntrinsicWidth. I suspect isAuto() in Render(MenuList|ListBox)::calcPrefWidths() .

Also, I guess we need the same change for <datagrid> . I'm not sure about <legend>, but probably it has similar issue when we specify display:inline-block explicitly?

CCing dhyatt.
Comment 10 Shinichiro Hamaji 2010-05-11 03:48:02 PDT
Comment on attachment 55678 [details]
Proposed Patch 2

Putting r- per my comment, but the overall plan looks sane to me. Thanks for tackling this issue!
Comment 11 Yoshiki Hayashi 2010-05-13 00:49:38 PDT
(In reply to comment #9)
> This change broke the following 3 tests on Mac:
> 
> fast/forms/003.html
> fast/forms/select-block-background.html
> fast/text/drawBidiText.html
> 
> I guess some code for <select> needs to check sizesToIntrinsicWidth. I suspect isAuto() in Render(MenuList|ListBox)::calcPrefWidths() .

My bad.  I forgot to include <select> in sizesToIntrinsicWidth.

> Also, I guess we need the same change for <datagrid> . I'm not sure about <legend>, but probably it has similar issue when we specify display:inline-block explicitly?

I'm not sure what would be the desired behavior but I've changed the code as suggested.  Now all the tags formerly treated width: auto as width: intrinsic behaves the same way, it is handled as width: auto only when in vertical flexbox with box-align: stretch.  Otherwise those are handled as width: intrinsic as before.
Comment 12 Yoshiki Hayashi 2010-05-13 00:54:19 PDT
Created attachment 55953 [details]
Proposed Patch 3
Comment 13 Shinichiro Hamaji 2010-05-13 02:34:09 PDT
Comment on attachment 55953 [details]
Proposed Patch 3

> +        * fast/flexbox/vertical-box-form-controls-expected.txt: Added.
> +        * fast/flexbox/vertical-box-form-controls.html: Added.

I think you forgot to include the test cases into your patch :)
Comment 14 Yoshiki Hayashi 2010-05-13 02:41:02 PDT
(In reply to comment #13)
> (From update of attachment 55953 [details])
> > +        * fast/flexbox/vertical-box-form-controls-expected.txt: Added.
> > +        * fast/flexbox/vertical-box-form-controls.html: Added.
> 
> I think you forgot to include the test cases into your patch :)

Oops.  Sorry about that!
Comment 15 Yoshiki Hayashi 2010-05-13 02:43:59 PDT
Created attachment 55957 [details]
Proposed Patch 4
Comment 16 Shinichiro Hamaji 2010-08-04 22:14:23 PDT
Comment on attachment 55957 [details]
Proposed Patch 4

Looks good to me. I'd wait a few days to see if someone object.
Comment 17 Adam Barth 2010-08-10 23:01:24 PDT
Comment on attachment 55957 [details]
Proposed Patch 4

The few days have passed.  :)
Comment 18 WebKit Commit Bot 2010-08-11 00:14:16 PDT
Comment on attachment 55957 [details]
Proposed Patch 4

Clearing flags on attachment: 55957

Committed r65131: <http://trac.webkit.org/changeset/65131>
Comment 19 WebKit Commit Bot 2010-08-11 00:14:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Shinichiro Hamaji 2010-08-11 00:18:18 PDT
(In reply to comment #17)
> (From update of attachment 55957 [details])
> The few days have passed.  :)

Ah, thanks for putting cq+ for this!