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

Issue 2716583002: Avoid negative content box sizes. (Closed)

Created:
3 years, 10 months ago by mstensho (USE GERRIT)
Modified:
3 years, 10 months ago
Reviewers:
eae
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, mstensho (USE GERRIT)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid negative content box sizes. Negative values had two possible causes: 1. Subtracting the scrollbar size from the border box size. Scrollbars do not affect the border box size, but they occupy space between some border edge and padding edge. This means that the presence of a scrollbar on an object reduces the size of the content box and the containing block established by said object. This means that if e.g. the specified width of an object is 10px and it has a vertical scrollbar that takes up more than that, e.g. 15px, the content box width should become 0, not -5px. 2. Subtracting two huge (or even saturated) LayoutUnit values from one LayoutUnit value. When we during layout convert a specified content-box width to border-box (via padding-box), which is what LayoutBox::m_frameRect uses, we may end up with saturated LayoutUnit values, so that: LayoutUnit specified_content_box_width = <whatever, something nice, maybe>; LayoutUnit left_padding = LayoutUnit::max() /*(or something huge, at least)*/; LayoutUnit right_padding = LayoutUnit::max() /*(or something huge, at least);*/ LayoutUnit padding_box_width = content_box_width + left_padding + right_padding; LayoutUnit content_box_width = padding_box_width - left_padding - right_padding; Here, content_box_width won't be the same as specified_content_box_width, because padding_box_width got saturated. That's kind of inevitable, with saturated arithmetic and all, but what's worse is that we used to end up with a negative value in content_box_width, which is illegal. So just clamp negative values to 0 to avoid that. Negative box sizes have various kinds of ill effects, such as inline-axis misalignment and unwanted negative block direction progression. It was possible to get negative padding (which is illegal) resolved from percentage values. This in turn caused unnecessary assertion failures in multicol. Attempted to come up with sensible layout tests that don't make assumptions about how the engine deals with extreme values internally. BUG=683554, 683090 Review-Url: https://codereview.chromium.org/2716583002 Cr-Commit-Position: refs/heads/master@{#452578} Committed: https://chromium.googlesource.com/chromium/src/+/47305a33360149240ab99c57c6c65ff5f05fa68e

Patch Set 1 #

Total comments: 5

Patch Set 2 : code review #

Messages

Total messages: 14 (9 generated)
mstensho (USE GERRIT)
https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html File third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html (right): https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html#newcode4 third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html:4: <div style="position:absolute; left:100%; border-width:1234567890px; border-style:solid;"> This test is valid, ...
3 years, 10 months ago (2017-02-23 13:35:10 UTC) #6
eae
LGTM w/minor suggestions https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html File third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html (right): https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html#newcode4 third_party/WebKit/LayoutTests/fast/block/large-border-abspos.html:4: <div style="position:absolute; left:100%; border-width:1234567890px; border-style:solid;"> On ...
3 years, 10 months ago (2017-02-23 15:54:37 UTC) #7
mstensho (USE GERRIT)
https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2716583002/diff/1/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode513 third_party/WebKit/Source/core/layout/LayoutBox.cpp:513: // of the two border sides may be larger ...
3 years, 10 months ago (2017-02-23 17:42:09 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2716583002/20001
3 years, 10 months ago (2017-02-23 17:42:54 UTC) #11
commit-bot: I haz the power
3 years, 10 months ago (2017-02-23 19:19:54 UTC) #14
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/47305a33360149240ab99c57c6c6...

Powered by Google App Engine
This is Rietveld 408576698