Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(218)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 578723003: Don't allow negative width for positioned elements (Closed)

Created:
6 years, 3 months ago by Kyungtae Kim
Modified:
6 years, 3 months ago
CC:
blink-reviews, blink-reviews-rendering, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., rune+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Don't allow minus width for positioned objects If left and right is not auto and left+right is bigger than the containing block width, the width of positioned element can be calculated as negative value. But, negative values for width are illegal (by CSS2.1 10.2) If the width is negative, the ClientRect and the boundingClientRect also have incorrect values. TEST=fast/css/absolute-position-with-negative-width.html BUG=383936 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182292

Patch Set 1 #

Patch Set 2 : Add a test #

Total comments: 12

Patch Set 3 : Simplify the test & Move the code to few lines up #

Total comments: 2

Patch Set 4 : fix nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -1 line) Patch
A LayoutTests/fast/css/absolute-position-with-negative-width.html View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/absolute-position-with-negative-width-expected.txt View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBox.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 13 (3 generated)
Kyungtae Kim
PTAL When calculating the 'width' of the positioned elements, if 'left' and 'right' is not ...
6 years, 3 months ago (2014-09-17 08:49:04 UTC) #1
rune
You need to add tests.
6 years, 3 months ago (2014-09-17 08:53:06 UTC) #3
Kyungtae Kim
On 2014/09/17 08:53:06, rune wrote: > You need to add tests. I add a test. ...
6 years, 3 months ago (2014-09-17 10:43:44 UTC) #4
rune
https://codereview.chromium.org/578723003/diff/20001/LayoutTests/fast/css/absolute-position-with-negative-width.html File LayoutTests/fast/css/absolute-position-with-negative-width.html (right): https://codereview.chromium.org/578723003/diff/20001/LayoutTests/fast/css/absolute-position-with-negative-width.html#newcode2 LayoutTests/fast/css/absolute-position-with-negative-width.html:2: <html> No need to include html, head or body ...
6 years, 3 months ago (2014-09-17 11:32:57 UTC) #6
mstensho (USE GERRIT)
https://codereview.chromium.org/578723003/diff/20001/Source/core/rendering/RenderBox.cpp File Source/core/rendering/RenderBox.cpp (right): https://codereview.chromium.org/578723003/diff/20001/Source/core/rendering/RenderBox.cpp#newcode3305 Source/core/rendering/RenderBox.cpp:3305: if (computedValues.m_extent < 0) Might consider moving this to ...
6 years, 3 months ago (2014-09-17 11:48:22 UTC) #7
Kyungtae Kim
https://codereview.chromium.org/578723003/diff/20001/LayoutTests/fast/css/absolute-position-with-negative-width.html File LayoutTests/fast/css/absolute-position-with-negative-width.html (right): https://codereview.chromium.org/578723003/diff/20001/LayoutTests/fast/css/absolute-position-with-negative-width.html#newcode2 LayoutTests/fast/css/absolute-position-with-negative-width.html:2: <html> On 2014/09/17 11:32:57, rune wrote: > No need ...
6 years, 3 months ago (2014-09-18 12:50:07 UTC) #8
rune
lgtm with nit. https://codereview.chromium.org/578723003/diff/40001/LayoutTests/fast/css/absolute-position-with-negative-width.html File LayoutTests/fast/css/absolute-position-with-negative-width.html (right): https://codereview.chromium.org/578723003/diff/40001/LayoutTests/fast/css/absolute-position-with-negative-width.html#newcode12 LayoutTests/fast/css/absolute-position-with-negative-width.html:12: shouldBe("absolute_with_left_right.offsetWidth","0"); Most tests don't indent script ...
6 years, 3 months ago (2014-09-18 13:07:32 UTC) #9
Kyungtae Kim
https://codereview.chromium.org/578723003/diff/40001/LayoutTests/fast/css/absolute-position-with-negative-width.html File LayoutTests/fast/css/absolute-position-with-negative-width.html (right): https://codereview.chromium.org/578723003/diff/40001/LayoutTests/fast/css/absolute-position-with-negative-width.html#newcode12 LayoutTests/fast/css/absolute-position-with-negative-width.html:12: shouldBe("absolute_with_left_right.offsetWidth","0"); On 2014/09/18 13:07:32, rune wrote: > Most tests ...
6 years, 3 months ago (2014-09-18 14:00:43 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/578723003/60001
6 years, 3 months ago (2014-09-19 00:41:23 UTC) #12
commit-bot: I haz the power
6 years, 3 months ago (2014-09-19 01:45:01 UTC) #13
Message was sent while issue was closed. 
Committed patchset #4 (id:60001) as 182292

Powered by Google App Engine
This is Rietveld 408576698