Bug 17918 – -webkit-box-align stretch does not work on replaced content

• Component: CSS
• Status: RESOLVED FIXED

[Erik Arvidsson]

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

[Erik Arvidsson]

Created attachment 19867 [details]
Tests -webkit-box-align: stretch

The buttons should be 100% of the width of the div

[Dave Hyatt]

I suspect this is a bug with <button> rather than box stretch not working in general.

[Erik Arvidsson]

I suspect it is a bug with any replaced content since it happens for textareas and inputs as well.

[Erik Arvidsson]

Created attachment 19868 [details]
Updated test case that tests, button, textarea, input and spans

[Yoshiki Hayashi]

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.

[Yoshiki Hayashi]

Created attachment 55530 [details]
Proposed Patch

[Yoshiki Hayashi]

Shinichiro ([e-mail]) 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.

[Yoshiki Hayashi]

Created attachment 55678 [details]
Proposed Patch 2

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

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!

[Yoshiki Hayashi]

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

[Yoshiki Hayashi]

Created attachment 55953 [details]
Proposed Patch 3

[Shinichiro Hamaji]

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

[Yoshiki Hayashi]

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

[Yoshiki Hayashi]

Created attachment 55957 [details]
Proposed Patch 4

[Shinichiro Hamaji]

Comment on attachment 55957 [details]
Proposed Patch 4

Looks good to me. I'd wait a few days to see if someone object.

[Adam Barth]

Comment on attachment 55957 [details]
Proposed Patch 4

The few days have passed.  :)

[WebKit Commit Bot]

Comment on attachment 55957 [details]
Proposed Patch 4

Clearing flags on attachment: 55957

Committed r65131: <http://trac.webkit.org/changeset/65131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

(In reply to comment #17)
> (From update of attachment 55957 [details])
> The few days have passed.  :)

Ah, thanks for putting cq+ for this!