Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(610)

Issue 2121173002: [css-grid] Fix percentage height resolution for replaced elements (Closed)

Created:
4 years ago by Manuel Rego
Modified:
3 years, 12 months ago
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Fix percentage height resolution for replaced elements The percentage heights for replaced elements were not working as expected in grid items. The patch fixes the different issues to make it work as expected. If a replaced item is a grid item, it needs to resolve the percentage height against its grid area. When the replaced item is a child of a grid item, and the grid item is stretched, it needs to use that size to resolve the percentage height too. BUG=624716 TEST=fast/css-grid-layout/grid-item-with-percent-height-replaced-element.html Committed: https://crrev.com/7886bec673ccae048c0ef5726ceb21c8fcbd86e0 Cr-Commit-Position: refs/heads/master@{#404896}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Make hasAutoHeightOrContainingBlockWithAutoHeight(bool) private #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -7 lines) Patch
A third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-with-percent-height-replaced-element.html View 1 chunk +73 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.cpp View 2 chunks +8 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp View 1 3 chunks +13 lines, -2 lines 0 comments Download

Messages

Total messages: 44 (18 generated)
Manuel Rego
4 years ago (2016-07-05 11:03:44 UTC) #2
svillar
Won't give my OK as I don't really know what this funcion is used for, ...
4 years ago (2016-07-07 11:45:48 UTC) #3
Manuel Rego
On 2016/07/07 11:45:48, svillar wrote: > Won't give my OK as I don't really know ...
4 years ago (2016-07-07 14:40:57 UTC) #5
cbiesinger
https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp#newcode558 third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool checkingContainingBlock) const On 2016/07/07 14:40:57, Manuel Rego ...
4 years ago (2016-07-07 17:34:27 UTC) #6
Manuel Rego
https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp#newcode558 third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool checkingContainingBlock) const On 2016/07/07 17:34:27, cbiesinger wrote: ...
4 years ago (2016-07-08 07:57:04 UTC) #7
cbiesinger
On 2016/07/08 07:57:04, Manuel Rego wrote: > https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): > > https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp#newcode558 ...
3 years, 12 months ago (2016-07-08 20:14:05 UTC) #8
Manuel Rego
Ok, so I've just uploaded a new version making hasAutoHeightOrContainingBlockWithAutoHeight(bool) private. PTAL, thanks!
3 years, 12 months ago (2016-07-11 09:15:06 UTC) #9
cbiesinger
lgtm
3 years, 12 months ago (2016-07-11 15:55:49 UTC) #10
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/2121173002/20001
3 years, 12 months ago (2016-07-12 05:14:36 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/260729)
3 years, 12 months ago (2016-07-12 06:41:23 UTC) #14
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/2121173002/20001
3 years, 12 months ago (2016-07-12 07:29:56 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/253837)
3 years, 12 months ago (2016-07-12 08:40:14 UTC) #18
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/2121173002/20001
3 years, 12 months ago (2016-07-12 08:48:18 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/260835)
3 years, 12 months ago (2016-07-12 09:04:48 UTC) #22
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/2121173002/20001
3 years, 12 months ago (2016-07-12 09:17:46 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/242467)
3 years, 12 months ago (2016-07-12 10:14:26 UTC) #26
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/2121173002/20001
3 years, 12 months ago (2016-07-12 11:20:19 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/254003)
3 years, 12 months ago (2016-07-12 13:43:45 UTC) #30
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/2121173002/20001
3 years, 12 months ago (2016-07-12 13:48:57 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/191260)
3 years, 12 months ago (2016-07-12 15:30:04 UTC) #34
eae
LGTM
3 years, 12 months ago (2016-07-12 16:08:43 UTC) #35
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/2121173002/20001
3 years, 12 months ago (2016-07-12 16:10:24 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/191416)
3 years, 12 months ago (2016-07-12 20:19:57 UTC) #39
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/2121173002/20001
3 years, 12 months ago (2016-07-12 20:41:14 UTC) #41
commit-bot: I haz the power
Committed patchset #2 (id:20001)
3 years, 12 months ago (2016-07-13 02:14:36 UTC) #42
commit-bot: I haz the power
3 years, 12 months ago (2016-07-13 02:17:14 UTC) #44
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/7886bec673ccae048c0ef5726ceb21c8fcbd86e0
Cr-Commit-Position: refs/heads/master@{#404896}

Powered by Google App Engine
This is Rietveld 408576698