Issue 24360004: Use shrink-to-fit for width for Button, input, select, textarea, and legend treat width value of 'a…

Issue 24360004: Use shrink-to-fit for width for Button, input, select, textarea, and legend treat width value of 'a… (Closed)

Created:
7 years, 3 months ago by pals

Modified:
7 years, 2 months ago

Reviewers:
cbiesinger, ojan

CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Use shrink-to-fit for width for Button, input, select, textarea, and legend treat width value of 'auto' as 'intrinsic'. Per CSS 2.1 section 10.3.8, the width should be the width an input would normally have (rule 1 in 10.3.8), and the 'right' value should be ignored because the values are over-constrained (rule 6 in 10.3.8). BUG=281380 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159860

Patch Set 1 #

Total comments: 7

Patch Set 2 : Layout tests #

Patch Set 3 : Added expected.txt and box-sizing tests #

Patch Set 4 : Added NeedsRebaseLine for win and mac #

Patch Set 5 : Reftests added #

Total comments: 6

Patch Set 6 : Fixed review comments #

Total comments: 6

Patch Set 7 : Fixed review comments #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+299 lines, -8 lines)			Patch
A	LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width.html	View	1 2 3 4 5	1 chunk	+69 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-expected.html	View	1 2 3 4 5	1 chunk	+68 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-quirks.html	View	1 2 3 4 5	1 chunk	+68 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-quirks-expected.html	View	1 2 3 4 5	1 chunk	+67 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBox.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2 3 4 5 6	5 chunks	+24 lines, -8 lines	0 comments	Download

Messages

Total messages: 23 (0 generated)

Expand Messages | Collapse Messages

tony

Ojan or Christian would be a better reviewer for this patch. This patch is missing ...

7 years, 3 months ago (2013-09-23 16:51:08 UTC) #2

commit-bot: I haz the power

No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...

7 years, 3 months ago (2013-09-24 06:22:08 UTC) #3

pals

I shall upload layout tests soon. I want to try this patch on try bots ...

7 years, 3 months ago (2013-09-24 06:36:25 UTC) #4

ojan

On 2013/09/24 06:36:25, sanjoyp wrote: > I shall upload layout tests soon. I want to ...

7 years, 2 months ago (2013-09-24 23:53:54 UTC) #5

ojan

https://codereview.chromium.org/24360004/diff/1/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/24360004/diff/1/Source/core/rendering/RenderBox.cpp#newcode3427 Source/core/rendering/RenderBox.cpp:3427: if (!isStretchingColumnFlexItem(this) && node() && (node()->hasTagName(inputTag) || node()->hasTagName(selectTag) This ...

7 years, 2 months ago (2013-09-24 23:58:44 UTC) #6

cbiesinger

7 years, 2 months ago (2013-09-26 18:03:14 UTC) #7

pals

Added tests. PTAL https://codereview.chromium.org/24360004/diff/1/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/24360004/diff/1/Source/core/rendering/RenderBox.cpp#newcode3427 Source/core/rendering/RenderBox.cpp:3427: if (!isStretchingColumnFlexItem(this) && node() && (node()->hasTagName(inputTag) ...

7 years, 2 months ago (2013-10-03 11:00:04 UTC) #8

cbiesinger

https://codereview.chromium.org/24360004/diff/1/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/24360004/diff/1/Source/core/rendering/RenderBox.cpp#newcode3430 Source/core/rendering/RenderBox.cpp:3430: LayoutUnit preferredMinWidth = minPreferredLogicalWidth() - bordersPlusPadding; Thanks for the ...

7 years, 2 months ago (2013-10-03 21:43:30 UTC) #9

pals

Updated with expected result file and box-sizing tests. PTAL.

7 years, 2 months ago (2013-10-04 07:32:38 UTC) #10

pals

On 2013/10/04 07:32:38, sanjoyp wrote: > Updated with expected result file and box-sizing tests. PTAL. ...

7 years, 2 months ago (2013-10-07 05:43:59 UTC) #11

commit-bot: I haz the power

No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...

7 years, 2 months ago (2013-10-07 05:44:11 UTC) #12

cbiesinger

On 2013/10/07 05:43:59, sanjoyp wrote: > On 2013/10/04 07:32:38, sanjoyp wrote: > > Updated with ...

7 years, 2 months ago (2013-10-07 18:54:25 UTC) #13

pals

On 2013/10/07 18:54:25, cbiesinger wrote: > On 2013/10/07 05:43:59, sanjoyp wrote: > > On 2013/10/04 ...

7 years, 2 months ago (2013-10-08 11:18:38 UTC) #14

cbiesinger

Looks good to me now with the changes below. But I'm not an official reviewer, ...

7 years, 2 months ago (2013-10-09 17:13:38 UTC) #15

pals

ojan, PTAL https://codereview.chromium.org/24360004/diff/36001/LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-quirks.html File LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-quirks.html (right): https://codereview.chromium.org/24360004/diff/36001/LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-quirks.html#newcode4 LayoutTests/fast/replaced/absolute-position-auto-width-and-left-and-right-and-intrinsic-width-quirks.html:4: .container { position: relative; height: 50px; background-color: ...

7 years, 2 months ago (2013-10-10 05:30:54 UTC) #16

ojan

https://codereview.chromium.org/24360004/diff/47001/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/24360004/diff/47001/Source/core/rendering/RenderBox.cpp#newcode2359 Source/core/rendering/RenderBox.cpp:2359: bool RenderBox::isElementWithIntrinsicWidth() const This name is too general. There ...

7 years, 2 months ago (2013-10-16 01:44:43 UTC) #17

pals

Fixed. Please review. https://codereview.chromium.org/24360004/diff/47001/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/24360004/diff/47001/Source/core/rendering/RenderBox.cpp#newcode2359 Source/core/rendering/RenderBox.cpp:2359: bool RenderBox::isElementWithIntrinsicWidth() const On 2013/10/16 01:44:43, ...

7 years, 2 months ago (2013-10-16 07:10:05 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/24360004/59001

7 years, 2 months ago (2013-10-16 23:37:51 UTC) #20

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/24360004/59001

7 years, 2 months ago (2013-10-17 00:08:41 UTC) #21

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/24360004/59001

7 years, 2 months ago (2013-10-17 01:26:56 UTC) #22

Message was sent while issue was closed.

Change committed as 159860

Expand Messages | Collapse Messages