Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1007623003:
Fix incorrect percentage top for positioned elements. (Closed)
|
Created:
5 years, 9 months ago by changseok Modified:
5 years, 8 months ago CC:
blink-reviews, blink-reviews-rendering, blink-reviews-style_chromium.org, zoltan1, blink-reviews-css, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, ed+blinkwatch_opera.com, leviw+renderwatch, Dominik Röttsches, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix incorrect percentage top for positioned elements.
This cl fixes wrong computed top from getComputedStyle('top'). It will not affect layout.
Current implementation for the computed top value has two issues which are not fit
spec nor gecko.
- Gecko returns a percentage top as is for elements positioned static.
- According to the spec, we should use a padding box, not content box
as a containing block of an element where the element is absolutely positioned.
http://www.w3.org/TR/CSS21/visudet.html#containing-block-details
To return correctly computed top, containingBlockLogicalHeightForComputedStyle
is newly added, not reusing containingBlockLogicalHeightForContent.
BUG=465192
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193757
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated patch #
Total comments: 7
Patch Set 3 : New approach by adding a more simple api to get containingBlock's height #
Total comments: 7
Patch Set 4 : Addressed comments #
Total comments: 6
Patch Set 5 : Just rebased #Patch Set 6 : Updated CL #
Total comments: 2
Patch Set 7 : Remove line-height and add font style to the test. #Patch Set 8 : Renamed containingBlockLogicalHeightForGetComputedStyle #Patch Set 9 : Update an expected value of web-animations-api/animations-responsive-to-style-change.html #Messages
Total messages: 36 (9 generated)
shivamidow@gmail.com changed reviewers: + eae@chromium.org, pdr@chromium.org, rune@opera.com, timloh@chromium.org
Please have a look.
This aligns the implementation with spec and Gecko, which is good. I'll only rubber stamp this, modulo comment, since I haven't worked with this code. CL description: - Way too long lines, keep them in the mid 70's at longest. - "calcurated" -> "calculated" https://codereview.chromium.org/1007623003/diff/1/Source/core/layout/LayoutBo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/1/Source/core/layout/LayoutBo... Source/core/layout/LayoutBox.cpp:2720: if (isLayoutBlock() && isOutOfFlowPositioned() && (heightType == IncludePadding)) { What happens if you pass IncludePadding with non-out-of-flow-positioned? It doesn't look like you handle that at all. Should you just check for IncludePadding and assert isLayoutBlock() && isOutOfFlowPositioned() instead?
https://codereview.chromium.org/1007623003/diff/1/Source/core/layout/LayoutBo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/1/Source/core/layout/LayoutBo... Source/core/layout/LayoutBox.cpp:2720: if (isLayoutBlock() && isOutOfFlowPositioned() && (heightType == IncludePadding)) { On 2015/03/13 12:49:13, rune wrote: > What happens if you pass IncludePadding with non-out-of-flow-positioned? > > It doesn't look like you handle that at all. Should you just check for > IncludePadding and assert isLayoutBlock() && isOutOfFlowPositioned() instead? IncludePadding is only for position:absolute. so none-out-of-flow-positioned elements can't have it. But I think you properly pointed out blinded problems. After rethinking the lines and testing gecko, I found this worked weirdly for some cases. Basically below recursive call of containingBlockLogicalHeightForContent need to be changed with a determinate method returning logical height. Otherwise viewport height (or page height, i,e initialContainingBlock in the spec) would be eventually returned for some cases.
rune@opera.com changed reviewers: + mstensho@opera.com
Added mstensho as reviewer as he'd be better at reviewing this. https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (left): https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2720: // FIXME: This is wrong if the containingBlock has a perpendicular writing mode. You removed this FIXME. Is it fixed? https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2723: // FIXME: Margin collapsing hasn't happened yet, so this incorrectly removes collapsed margins. You removed this FIXME. Is it fixed? https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2721: LayoutUnit availableHeight = cb->clientLogicalHeight(); This seems quite different from containingBlockLogicalHeightForContent(). If the need for this change can be observed as bugs, you should add some more testcases. I don't quite follow this. Some layout code owner should take over reviewing, I think.
On 2015/03/16 13:34:03, rune wrote: > Added mstensho as reviewer as he'd be better at reviewing this. > > https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... > File Source/core/layout/LayoutBox.cpp (left): > > https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... > Source/core/layout/LayoutBox.cpp:2720: // FIXME: This is wrong if the > containingBlock has a perpendicular writing mode. > You removed this FIXME. Is it fixed? > > https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... > Source/core/layout/LayoutBox.cpp:2723: // FIXME: Margin collapsing hasn't > happened yet, so this incorrectly removes collapsed margins. > You removed this FIXME. Is it fixed? > > https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... > File Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... > Source/core/layout/LayoutBox.cpp:2721: LayoutUnit availableHeight = > cb->clientLogicalHeight(); > This seems quite different from containingBlockLogicalHeightForContent(). If the > need for this change can be observed as bugs, you should add some more > testcases. > > I don't quite follow this. Some layout code owner should take over reviewing, I > think. Thanks rune. I'm not also convinced by myself yet. :P At least I could not see any special issue when running layout tests in local. (But I'm using mac 10.10 Yosemite which is not an official supported platform, so that's not that trustable.) Would you trigger layout bots for me? It would much help me to understand the perpendicular stuff and to elaborate my patch more. =)
https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2720: const LayoutBlock* cb = isLayoutBlock() ? toLayoutBlock(this) : containingBlock(); Pretending that we're our own containing block? This makes no sense. As far as I can tell, the only thing you want to change is resolving percentage top/bottom values from computedStyle, to increase browser interoperability. Then I don't think this method should be changed at all, since it's also called during layout. The changes here cause quite a few test regressions.
https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2720: const LayoutBlock* cb = isLayoutBlock() ? toLayoutBlock(this) : containingBlock(); On 2015/03/23 14:27:55, mstensho wrote: > Pretending that we're our own containing block? This makes no sense. > > As far as I can tell, the only thing you want to change is resolving percentage > top/bottom values from computedStyle, to increase browser interoperability. Then > I don't think this method should be changed at all, since it's also called > during layout. The changes here cause quite a few test regressions. Agree. I looked into this area more. I don't know the history of containingBlockLogicalHeight , but what I can tell you is it is too much complex over its name. Actually containingBlockLogicalWidth works more simple. containingBlockLogicalWidthForContent -> availableLogicalWidth -> contentLogicalWidth -> contentWidth In the other hand. containingBlockLogicalHeightForContent -> availableLogicalHeight -> availableLogicalHeightUsing -> containingBlockLogicalHeightForContent -> ... -> eventually the height of viewport returned where containingBlock is static or relative or absolute. So I think getting content height from containingBlockLogicalHeightForContent is not proper. It might be originally designed for this purpose, but not anymore. Fixing this method may not be a good idea as you pointed out. It has been used some places and involved in layout. Rather, adding a new method returning immediate containingBlock's content height or padding box height is better for percentage top. https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2721: LayoutUnit availableHeight = cb->clientLogicalHeight(); On 2015/03/16 13:34:03, rune wrote: > This seems quite different from containingBlockLogicalHeightForContent(). If the > need for this change can be observed as bugs, you should add some more > testcases. Yeah.. I think containingBlockLogicalHeightForContent is too complex to get contentHeight of containing block. We might need a more simple method for it. > > I don't quite follow this. Some layout code owner should take over reviewing, I > think. O.K Thanks anyway ;)
We're getting there. :) https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:2720: const LayoutBlock* cb = isLayoutBlock() ? toLayoutBlock(this) : containingBlock(); On 2015/03/26 07:12:04, changseok wrote: > On 2015/03/23 14:27:55, mstensho wrote: > > Pretending that we're our own containing block? This makes no sense. > > > > As far as I can tell, the only thing you want to change is resolving > percentage > > top/bottom values from computedStyle, to increase browser interoperability. > Then > > I don't think this method should be changed at all, since it's also called > > during layout. The changes here cause quite a few test regressions. > > Agree. I looked into this area more. I don't know the history of > containingBlockLogicalHeight , but what I can tell you is it is too much complex > over its name. Actually containingBlockLogicalWidth works more simple. > containingBlockLogicalWidthForContent -> availableLogicalWidth -> > contentLogicalWidth -> contentWidth > > In the other hand. > containingBlockLogicalHeightForContent -> availableLogicalHeight -> > availableLogicalHeightUsing -> containingBlockLogicalHeightForContent -> ... -> > eventually the height of viewport returned where containingBlock is static or > relative or absolute. Yeah, but resolving percentage heights isn't as simple as resolving percentage widths. If the containing block has auto height, for instance... > So I think getting content height from containingBlockLogicalHeightForContent is > not proper. It might be originally designed for this purpose, but not anymore. > Fixing this method may not be a good idea as you pointed out. It has been used > some places and involved in layout. Rather, adding a new method returning > immediate containingBlock's content height or padding box height is better for > percentage top. https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:1529: // http://www.w3.org/TR/CSS21/visudet.html#containing-block-details Better omit this text. This is not completely spec compliant, and it never was (not before your changes, either). Consider: <span style="display:inline; position:relative;"> xxx <div style="position:absolute; top:50%;">xxx</div> </div> It's laid out correctly, but computedStyle.top on the abspos won't use the inline as its containing block. Not before your changes. Not after your changes. https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.h:479: LayoutUnit containingBlockLogicalHeight() const; Need a more descriptive name, so that nobody gets tempted to use it during layout. containingBlockLogicalHeightForComputedStyleResolution(), or something less ugly if you can think of something.
https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:1529: // http://www.w3.org/TR/CSS21/visudet.html#containing-block-details On 2015/03/26 12:53:37, mstensho wrote: > Better omit this text. This is not completely spec compliant, and it never was > (not before your changes, either). > > Consider: > <span style="display:inline; position:relative;"> > xxx > <div style="position:absolute; top:50%;">xxx</div> > </div> > > It's laid out correctly, but computedStyle.top on the abspos won't use the > inline as its containing block. Not before your changes. Not after your changes. Good point. containingBlock never return inline block. We might need an another api to return containing box including inline block as well as layoutBlock e,g containingBox()? https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.h:479: LayoutUnit containingBlockLogicalHeight() const; On 2015/03/26 12:53:37, mstensho wrote: > Need a more descriptive name, so that nobody gets tempted to use it during > layout. > > containingBlockLogicalHeightForComputedStyleResolution(), or something less ugly > if you can think of something. I'm not good at naming anything too. :p How about just containingBlockLogicalheightForComputedStyle()?
https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:1529: // http://www.w3.org/TR/CSS21/visudet.html#containing-block-details On 2015/03/30 07:17:57, changseok wrote: > On 2015/03/26 12:53:37, mstensho wrote: > > Better omit this text. This is not completely spec compliant, and it never was > > (not before your changes, either). > > > > Consider: > > <span style="display:inline; position:relative;"> > > xxx > > <div style="position:absolute; top:50%;">xxx</div> > > </div> > > > > It's laid out correctly, but computedStyle.top on the abspos won't use the > > inline as its containing block. Not before your changes. Not after your > changes. > Good point. containingBlock never return inline block. We might need an another > api to return containing box including inline block as well as layoutBlock e,g > containingBox()? > Well. we don't need to create a new api. container() works similarly.
Comment please?
Just some final nits. Could you also update the description, please? It seems somewhat off now. https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.h:479: LayoutUnit containingBlockLogicalHeight() const; On 2015/03/30 07:17:57, changseok wrote: > On 2015/03/26 12:53:37, mstensho wrote: > > Need a more descriptive name, so that nobody gets tempted to use it during > > layout. > > > > containingBlockLogicalHeightForComputedStyleResolution(), or something less > ugly > > if you can think of something. > > I'm not good at naming anything too. :p How about just > containingBlockLogicalheightForComputedStyle()? Sounds good. https://codereview.chromium.org/1007623003/diff/60001/LayoutTests/fast/css/ge... File LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html (right): https://codereview.chromium.org/1007623003/diff/60001/LayoutTests/fast/css/ge... LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:2: </style> Stray terminator. https://codereview.chromium.org/1007623003/diff/60001/LayoutTests/fast/css/ge... LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:3: <span id='outer' style="display:inline; position:relative; border:10px solid red; margin:20px; padding:30px;"> Could you please add a line-height declaration here (line-height:50px, for instance), so that font or font engine differences won't cause different results? https://codereview.chromium.org/1007623003/diff/60001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/60001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:1536: height = containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding); Blink coding style prefers early returns. You could do this: if (!isPositioned()) return containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding); LayoutUnit height; LayoutBoxModelObject* cb = toLayoutBoxModelObject(container()); ...
Thanks for your review! All comments are addressed. https://codereview.chromium.org/1007623003/diff/60001/LayoutTests/fast/css/ge... File LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html (right): https://codereview.chromium.org/1007623003/diff/60001/LayoutTests/fast/css/ge... LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:2: </style> On 2015/04/13 21:40:04, mstensho wrote: > Stray terminator. Removed. https://codereview.chromium.org/1007623003/diff/60001/LayoutTests/fast/css/ge... LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:3: <span id='outer' style="display:inline; position:relative; border:10px solid red; margin:20px; padding:30px;"> On 2015/04/13 21:40:04, mstensho wrote: > Could you please add a line-height declaration here (line-height:50px, for > instance), so that font or font engine differences won't cause different > results? Done. https://codereview.chromium.org/1007623003/diff/60001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1007623003/diff/60001/Source/core/layout/Layo... Source/core/layout/LayoutBox.cpp:1536: height = containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding); On 2015/04/13 21:40:04, mstensho wrote: > Blink coding style prefers early returns. You could do this: > > if (!isPositioned()) > return containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding); > LayoutUnit height; > LayoutBoxModelObject* cb = toLayoutBoxModelObject(container()); > ... Done.
Could you also mention on the first line in the description that this fix only has to do with getComputedStyle()? https://codereview.chromium.org/1007623003/diff/100001/LayoutTests/fast/css/g... File LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html (right): https://codereview.chromium.org/1007623003/diff/100001/LayoutTests/fast/css/g... LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:2: <span id='outer' style="display:inline; position:relative; line-height:50px; border:10px solid red; margin:20px; padding:30px;"> Sorry for misleading you, but line-height:50px did no good (line-height is irrelevant, since the span is INSIDE the line). But we need something more here to make the test more reliable. On my machine (Linux) computedStyle.getPropertyValue('top') becomes 39.5px, so the test fails (both with line-height:50px and without). Could you add "font:20px Ahem" instead of "line-height:50px"? Then top becomes 40px, which makes sense (30px + 20px * 50 / 100).
> Could you also mention on the first line in the description that this fix only has to do with getComputedStyle()? The description is updated as well. https://codereview.chromium.org/1007623003/diff/100001/LayoutTests/fast/css/g... File LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html (right): https://codereview.chromium.org/1007623003/diff/100001/LayoutTests/fast/css/g... LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:2: <span id='outer' style="display:inline; position:relative; line-height:50px; border:10px solid red; margin:20px; padding:30px;"> On 2015/04/14 09:20:07, mstensho wrote: > Sorry for misleading you, but line-height:50px did no good (line-height is > irrelevant, since the span is INSIDE the line). > > But we need something more here to make the test more reliable. On my machine > (Linux) computedStyle.getPropertyValue('top') becomes 39.5px, so the test fails > (both with line-height:50px and without). > > Could you add "font:20px Ahem" instead of "line-height:50px"? > > Then top becomes 40px, which makes sense (30px + 20px * 50 / 100). No problem. Done.
On 2015/04/14 09:48:28, changseok wrote: > > Could you also mention on the first line in the description that this fix only > has to do with getComputedStyle()? > > The description is updated as well. > > https://codereview.chromium.org/1007623003/diff/100001/LayoutTests/fast/css/g... > File > LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html > (right): > > https://codereview.chromium.org/1007623003/diff/100001/LayoutTests/fast/css/g... > LayoutTests/fast/css/getComputedStyle/computed-style-percentage-top-with-position-inline.html:2: > <span id='outer' style="display:inline; position:relative; line-height:50px; > border:10px solid red; margin:20px; padding:30px;"> > On 2015/04/14 09:20:07, mstensho wrote: > > Sorry for misleading you, but line-height:50px did no good (line-height is > > irrelevant, since the span is INSIDE the line). > > > > But we need something more here to make the test more reliable. On my machine > > (Linux) computedStyle.getPropertyValue('top') becomes 39.5px, so the test > fails > > (both with line-height:50px and without). > > > > Could you add "font:20px Ahem" instead of "line-height:50px"? > > > > Then top becomes 40px, which makes sense (30px + 20px * 50 / 100). > > No problem. Done. I think you forgot to upload a new patch set?
On 2015/04/14 09:51:29, mstensho wrote: > I think you forgot to upload a new patch set? Or I was just being too impatient. :) It's there now. lgtm.
On 2015/04/14 09:52:55, mstensho wrote: > On 2015/04/14 09:51:29, mstensho wrote: > > I think you forgot to upload a new patch set? > > Or I was just being too impatient. :) It's there now. > > lgtm. No surprised. I can't wait for this landing, too. Thank you. ;)
https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBox.h:479: LayoutUnit containingBlockLogicalHeight() const; On 2015/04/13 21:40:04, mstensho wrote: > On 2015/03/30 07:17:57, changseok wrote: > > On 2015/03/26 12:53:37, mstensho wrote: > > > Need a more descriptive name, so that nobody gets tempted to use it during > > > layout. > > > > > > containingBlockLogicalHeightForComputedStyleResolution(), or something less > > ugly > > > if you can think of something. > > > > I'm not good at naming anything too. :p How about just > > containingBlockLogicalheightForComputedStyle()? > > Sounds good. This name is misleading. Maybe [...]ForGetComputedStyle? The ComputedStyle object stores computed values, while getComputedStyle returns what are now called resolved values. This is equal to the computed value for most properties, but in a select few cases (e.g. top) returns something else. http://dev.w3.org/csswg/cssom/#resolved-values
On 2015/04/14 09:57:49, Timothy Loh wrote: > https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... > File Source/core/layout/LayoutBox.h (right): > > https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... > Source/core/layout/LayoutBox.h:479: LayoutUnit containingBlockLogicalHeight() > const; > On 2015/04/13 21:40:04, mstensho wrote: > > On 2015/03/30 07:17:57, changseok wrote: > > > On 2015/03/26 12:53:37, mstensho wrote: > > > > Need a more descriptive name, so that nobody gets tempted to use it during > > > > layout. > > > > > > > > containingBlockLogicalHeightForComputedStyleResolution(), or something > less > > > ugly > > > > if you can think of something. > > > > > > I'm not good at naming anything too. :p How about just > > > containingBlockLogicalheightForComputedStyle()? > > > > Sounds good. > > This name is misleading. Maybe [...]ForGetComputedStyle? The ComputedStyle > object stores computed values, while getComputedStyle returns what are now > called resolved values. This is equal to the computed value for most properties, > but in a select few cases (e.g. top) returns something else. > > http://dev.w3.org/csswg/cssom/#resolved-values O.K.. then does containingBlockLogicalHeightForGetComputedStyle make you all happy? =)
On 2015/04/14 10:02:54, changseok wrote: > On 2015/04/14 09:57:49, Timothy Loh wrote: > > > https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... > > File Source/core/layout/LayoutBox.h (right): > > > > > https://codereview.chromium.org/1007623003/diff/40001/Source/core/layout/Layo... > > Source/core/layout/LayoutBox.h:479: LayoutUnit containingBlockLogicalHeight() > > const; > > On 2015/04/13 21:40:04, mstensho wrote: > > > On 2015/03/30 07:17:57, changseok wrote: > > > > On 2015/03/26 12:53:37, mstensho wrote: > > > > > Need a more descriptive name, so that nobody gets tempted to use it > during > > > > > layout. > > > > > > > > > > containingBlockLogicalHeightForComputedStyleResolution(), or something > > less > > > > ugly > > > > > if you can think of something. > > > > > > > > I'm not good at naming anything too. :p How about just > > > > containingBlockLogicalheightForComputedStyle()? > > > > > > Sounds good. > > > > This name is misleading. Maybe [...]ForGetComputedStyle? The ComputedStyle > > object stores computed values, while getComputedStyle returns what are now > > called resolved values. This is equal to the computed value for most > properties, > > but in a select few cases (e.g. top) returns something else. > > > > http://dev.w3.org/csswg/cssom/#resolved-values > > O.K.. then does containingBlockLogicalHeightForGetComputedStyle make you all > happy? =) Sure, rs-lgtm once that's changed.
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1007623003/#ps140001 (title: "Renamed containingBlockLogicalHeightForGetComputedStyle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007623003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/51747)
On 2015/04/14 19:21:18, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/51747) > FAIL Lengths responsive to style changes assert_equals: expected "350px" but got "calc(150px + 40%)" I don't think this is a regression. Gecko also represent left and top of static positioned element as that kind of form. Try https://jsfiddle.net/axu4uh9o/ and see the console in inspector of Gecko. I'll try to land again if no strong objection to update the expected value for the complaining test.
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1007623003/#ps160001 (title: "Update an expected value of web-animations-api/animations-responsive-to-style-change.html")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007623003/160001
The CQ bit was unchecked by timloh@chromium.org
The CQ bit was checked by timloh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007623003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193757 |
