Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
385583005:
For flex items, percent paddings should resolve against their respective dimension
|
Created:
6 years, 5 months ago by harpreet.sk Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFor flex items, percent paddings should resolve against their respective dimension
Normally, padding top and padding bottom resolve against the width
of the containing block, but for the flex items, we should resolve
against the height of the containing block.
http://dev.w3.org/csswg/css-flexbox/#item-margins
This patch tries to resolve this bug by performing computation
of percentage padding-top , padding-bottom , padding-before and
padding-after against the height of the flexbox.
BUG=229568
Patch Set 1 #
Total comments: 16
Patch Set 2 : WIP Patch 2 #
Total comments: 8
Patch Set 3 : WIP Patch 3 #
Total comments: 17
Patch Set 4 : WIP Patch 4 #Patch Set 5 : WIP Patch 5 #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : WIP Patch 6 #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 22 (0 generated)
This is a WIP patch trying to solve the problem for padding. please take a look..
The change description should link to the spec instead of an email of meeting notes. http://dev.w3.org/csswg/css-flexbox/#item-margins Also, the change description mentions margins and padding, but it looks like it only changes the behavior of padding. For reference, the code for fixing margins is in RenderFlexibleBox::computeChildMarginValue(). What about Grid Layout? BTW, you can test your changes against Firefox which has already implemented this behavior. Here's an example: http://jsfiddle.net/PLya6/ https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... File LayoutTests/css3/flexbox/percent-margins-padding.html (right): https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:15: shouldBeEqualToString('window.getComputedStyle(document.getElementById("flexItem1"),null).getPropertyValue("padding-top")', '10px'); I think it would a bit clearer to put a div inside the flex children and use data-* attributes to check the padding rather than using getComputedStyle. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:19: shouldBeEqualToString('window.getComputedStyle(document.getElementById("flexItem2"),null).getPropertyValue("padding-top")', '8px'); How does this get a value of 8px? The percent height doesn't have a fixed value to resolve against. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:26: shouldBeEqualToString('window.getComputedStyle(document.getElementById("flexItem3"),null).getPropertyValue("padding-right")', '235.1875px'); Why no tests for before/after? You make code changes to before/after. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:35: <div class="flexbox" style="height:30%"> Did you mean to put a percent height here? It doesn't do anything does it? https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:36: <div id="flexItem2" data-expected-height="20" style="padding:40%; background:yellow">Pasta batman</div> Is the height based on the font? That seems unreliable across platforms. https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBoxModelObject.cpp:317: w = (containingBlock()->isFlexibleBox() && isTopOrBottomOrAfterOrBeforePadding)?containingBlockLogicalHeightForContent():containingBlockLogicalWidthForContent(); There should be spaces around the ? and :. https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBoxModelObject.cpp:2512: return containingBlock()->logicalHeight() - containingBlock()->borderAndPaddingLogicalHeight(); Does this always work? We might not have computed the height yet for the containing block, but I guess in that case we should ignore the percentage height (since it can't be resolved). https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBoxModelObject.h (right): https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBoxModelObject.h:84: LayoutUnit computedCSSPaddingTop() const { return computedCSSPadding(style()->paddingTop(), true); } This should be an enum, not a bool.
On 2014/07/10 17:10:06, tony wrote: > The change description should link to the spec instead of an email of meeting > notes. > http://dev.w3.org/csswg/css-flexbox/#item-margins > Sorry that link came by mistake. Updated the description containing correct link. > Also, the change description mentions margins and padding, but it looks like it > only changes the behavior of padding. > I have updated change description. In this CL i am only resolving padding issue. For margins i will upload a different CL. > > What about Grid Layout? > I am not sure that right now we want to resolve this issue for grid also. Currently i think none of the browser is resolving percentage padding against the height for grid. Can you please confirm. > BTW, you can test your changes against Firefox which has already implemented > this behavior. > I cross checked the value for the test cases i have added in layout test with Firefox. It's same as Firefox.
Uploaded new patch... ptal.... https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... File LayoutTests/css3/flexbox/percent-margins-padding.html (right): https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:15: shouldBeEqualToString('window.getComputedStyle(document.getElementById("flexItem1"),null).getPropertyValue("padding-top")', '10px'); On 2014/07/10 17:10:06, tony wrote: > I think it would a bit clearer to put a div inside the flex children and use > data-* attributes to check the padding rather than using getComputedStyle. Done. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:19: shouldBeEqualToString('window.getComputedStyle(document.getElementById("flexItem2"),null).getPropertyValue("padding-top")', '8px'); On 2014/07/10 17:10:06, tony wrote: > How does this get a value of 8px? The percent height doesn't have a fixed value > to resolve against. There was a mistake in previous solution. Correct the mistake with new solution. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:26: shouldBeEqualToString('window.getComputedStyle(document.getElementById("flexItem3"),null).getPropertyValue("padding-right")', '235.1875px'); On 2014/07/10 17:10:06, tony wrote: > Why no tests for before/after? You make code changes to before/after. Done. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:35: <div class="flexbox" style="height:30%"> On 2014/07/10 17:10:06, tony wrote: > Did you mean to put a percent height here? It doesn't do anything does it? Just wanted add the case when containing block has height in % which cannot be resolved. In this case then padding top and bottom will be 0. https://codereview.chromium.org/385583005/diff/1/LayoutTests/css3/flexbox/per... LayoutTests/css3/flexbox/percent-margins-padding.html:36: <div id="flexItem2" data-expected-height="20" style="padding:40%; background:yellow">Pasta batman</div> On 2014/07/10 17:10:06, tony wrote: > Is the height based on the font? That seems unreliable across platforms. Acknowledged. https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBoxModelObject.cpp:317: w = (containingBlock()->isFlexibleBox() && isTopOrBottomOrAfterOrBeforePadding)?containingBlockLogicalHeightForContent():containingBlockLogicalWidthForContent(); On 2014/07/10 17:10:06, tony wrote: > There should be spaces around the ? and :. Done. https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBoxModelObject.cpp:2512: return containingBlock()->logicalHeight() - containingBlock()->borderAndPaddingLogicalHeight(); On 2014/07/10 17:10:06, tony wrote: > Does this always work? We might not have computed the height yet for the > containing block, but I guess in that case we should ignore the percentage > height (since it can't be resolved). This will not work always. Uploaded new solution. https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBoxModelObject.h (right): https://codereview.chromium.org/385583005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBoxModelObject.h:84: LayoutUnit computedCSSPaddingTop() const { return computedCSSPadding(style()->paddingTop(), true); } On 2014/07/10 17:10:06, tony wrote: > This should be an enum, not a bool. Done.
https://codereview.chromium.org/385583005/diff/20001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding.html (right): https://codereview.chromium.org/385583005/diff/20001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding.html:13: <div id="flexItem1" data-expected-padding-top="10" data-expected-padding-bottom="10" data-expected-padding-before="10" data-expected-padding-after="10" style="background:yellow; padding:10%;">Pasta batman</div> data-expected-padding-before and data-expected-padding-after don't do anything. See LayoutTests/resources/check-layout.js. https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2525: LayoutUnit RenderBoxModelObject::computePercentLogicalHeight() const I'm not sure this function is correct. For example, see RenderBox::computePercentageLogicalHeight which is much more complicated and has lots of quirks for certain elements like tables. This might not be the right design. Maybe Ojan has some insight in the right way to resolve percent heights while we're still laying out a child. https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2530: if (cb->style()->logicalHeight().isAuto()) This should probably be !isSpecified(). Please add some test cases with other values for height like a calculated value or min-content or fit-content. https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2534: percentHeightFactor *= cb->style()->logicalHeight().value() / 100; I think you want to verify that b->style()->logicalHeight().isPercent().
I tried to go through the implementation of mozilla (although only some part of it i understand) and by testing various scenario in mozilla i found that it only resolves the percentage padding in the cases in which height is finite (ie height do not depend on layout of content ie cases of fixed height or calculated height). In other cases like percent height which is not resolvable or intrinsic height (min-content, max-content etc) or auto height it simply makes the padding-top and bottom as 0. I tried to incorporate most of the test cases which are added in mozilla too and all work fine. https://codereview.chromium.org/385583005/diff/20001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding.html (right): https://codereview.chromium.org/385583005/diff/20001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding.html:13: <div id="flexItem1" data-expected-padding-top="10" data-expected-padding-bottom="10" data-expected-padding-before="10" data-expected-padding-after="10" style="background:yellow; padding:10%;">Pasta batman</div> On 2014/07/14 16:34:38, tony wrote: > data-expected-padding-before and data-expected-padding-after don't do anything. > See LayoutTests/resources/check-layout.js. Removed padding-after and padding-before https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2525: LayoutUnit RenderBoxModelObject::computePercentLogicalHeight() const On 2014/07/14 16:34:38, tony wrote: > I'm not sure this function is correct. For example, see > RenderBox::computePercentageLogicalHeight which is much more complicated and has > lots of quirks for certain elements like tables. This might not be the right > design. Maybe Ojan has some insight in the right way to resolve percent heights > while we're still laying out a child. I tried to go through the implementation of mozilla (although only some part of it i understand) and by testing various scenario in mozilla i found that it only resolves the percentage padding in the cases in which height is finite (ie height do not depend on layout of content ie cases of fixed height or calculated height). In other cases like percent height which is not resolvable or intrinsic height (min-content, max-content etc) or auto height it simply makes the padding-top and bottom as 0. I tried to incorporate most of the test cases which are added in mozilla too and all work fine. https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2530: if (cb->style()->logicalHeight().isAuto()) On 2014/07/14 16:34:38, tony wrote: > This should probably be !isSpecified(). > > Please add some test cases with other values for height like a calculated value > or min-content or fit-content. Done. https://codereview.chromium.org/385583005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2534: percentHeightFactor *= cb->style()->logicalHeight().value() / 100; On 2014/07/14 16:34:38, tony wrote: > I think you want to verify that b->style()->logicalHeight().isPercent(). Modifeid the solution to include check for all types of height.
@ojan, christian, tony - please have a look
This approach seems reasonable to me, but I might be missing something. Since I'm not a reviewer, Levi or Ojan will need to take a look. Ojan is busy with the infra code yellow, so it may be a while. https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding-expected.txt (right): https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding-expected.txt:1: Pasta batman Nit: I find the text here detract from the results. I would replace the text with or not have text or something.
https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding-expected.txt (right): https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding-expected.txt:1: Pasta batman +1, though I know Pasta Batman is the best name for a dog ever. https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding.html (right): https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding.html:12: <div class="flexbox" style="height:100px; padding: 10px; border:2px solid red;"> Nit: please promote the repeated inline styles to the above sheet. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2514: LayoutUnit RenderBoxModelObject::containingBlockLogicalHeightForContent() const This is different than RenderBox::containingBlockLogicalHeightForContent... It should either be unified or renamed to make its different functionality apparent.
https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:317: if (containingBlock()->isFlexibleBox() && (type == TopPadding || type == BottomPadding || type == BeforePadding || type == AfterPadding)) This isn't correct for vertical writing mode. TopPadding/BottomPadding are not computed with respect to the logical height in vertical writing mode. Please add some vertical writing mode tests. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2522: return containingBlock()->style()->logicalHeight().value(); Please ASSERT(containingBlock()->style()->logicalHeight().type() == Fixed). Even better, just make this a switch statement over the type. That way you save a branch and the compiler will yell at you if you miss any cases. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2525: LayoutUnit RenderBoxModelObject::computePercentLogicalHeight() const This also is duplicating a method on RenderBox but doing something different. I don't think this is right for a bunch of cases. Why can't you use RenderBox::computePercentageLogicalHeight? https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2527: float percentHeightFactor = style()->logicalHeight().value() / 100; ASSERT(style()->logicalHeight().isPercent()); https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2532: if (cb->style()->logicalHeight().isFixed() || cb->style()->logicalHeight().isCalculated()) This should use skipContainingBlockForPercentHeightCalculation.
Please add a runtime flag for this. I don't want us to ship this until we have both padding and margin fixed.
Please have a look.... https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding-expected.txt (right): https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding-expected.txt:1: Pasta batman On 2014/07/15 18:29:26, tony wrote: > Nit: I find the text here detract from the results. I would replace the text > with or not have text or something. Done. https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... File LayoutTests/css3/flexbox/percent-margins-padding.html (right): https://codereview.chromium.org/385583005/diff/40001/LayoutTests/css3/flexbox... LayoutTests/css3/flexbox/percent-margins-padding.html:12: <div class="flexbox" style="height:100px; padding: 10px; border:2px solid red;"> On 2014/07/15 20:08:29, leviw wrote: > Nit: please promote the repeated inline styles to the above sheet. Done. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:317: if (containingBlock()->isFlexibleBox() && (type == TopPadding || type == BottomPadding || type == BeforePadding || type == AfterPadding)) On 2014/07/15 20:33:25, ojan-only-code-yellow-reviews wrote: > This isn't correct for vertical writing mode. TopPadding/BottomPadding are not > computed with respect to the logical height in vertical writing mode. Please add > some vertical writing mode tests. Done. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2514: LayoutUnit RenderBoxModelObject::containingBlockLogicalHeightForContent() const On 2014/07/15 20:08:29, leviw wrote: > This is different than RenderBox::containingBlockLogicalHeightForContent... It > should either be unified or renamed to make its different functionality > apparent. Renamed. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2522: return containingBlock()->style()->logicalHeight().value(); On 2014/07/15 20:33:25, ojan-only-code-yellow-reviews wrote: > Please ASSERT(containingBlock()->style()->logicalHeight().type() == Fixed). Even > better, just make this a switch statement over the type. That way you save a > branch and the compiler will yell at you if you miss any cases. Replaced if's with switch https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2525: LayoutUnit RenderBoxModelObject::computePercentLogicalHeight() const On 2014/07/15 20:33:25, ojan-only-code-yellow-reviews wrote: > This also is duplicating a method on RenderBox but doing something different. I > don't think this is right for a bunch of cases. Why can't you use > RenderBox::computePercentageLogicalHeight? Renamed the function. The reason why i am not using RenderBox::computePercentageLogicalHeight because say for example <div style="height:fit-content;"> <div class= "flexbox" "style="height=10%"> <div style="padding: 10%"> </div> </div> </div> If suppose i use RenderBox::computePercentageLogicalHeight to get percent height of flexbox for computation of padding in that case at that time of computation of padding since the layout has not been done and outer div has layout dependent height so say it will return 0 for height of flexbox and padding-top and bottom becomes 0. Once layout has been done with padding-top/bottom as 0 and say we now query for the values of padding-top and padding-bottom using getCOmputedStyles it will again call this api RenderBox::computePercentageLogicalHeight and since this time layout has been done so we will have some finite height for outer div so this time height of flexbox will be coming different and hence padding values coming will be different. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2527: float percentHeightFactor = style()->logicalHeight().value() / 100; On 2014/07/15 20:33:25, ojan-only-code-yellow-reviews wrote: > ASSERT(style()->logicalHeight().isPercent()); Done. https://codereview.chromium.org/385583005/diff/40001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2532: if (cb->style()->logicalHeight().isFixed() || cb->style()->logicalHeight().isCalculated()) On 2014/07/15 20:33:25, ojan-only-code-yellow-reviews wrote: > This should use skipContainingBlockForPercentHeightCalculation. Done.
On 2014/07/15 20:34:19, ojan-only-code-yellow-reviews wrote: > Please add a runtime flag for this. I don't want us to ship this until we have > both padding and margin fixed. Added runtime flag for this
https://codereview.chromium.org/385583005/diff/90001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/90001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2532: default: One of the reasons to use a switch statement is so you can enumerate all the values and get a compiler error if someone adds a new type. Please replace the |default| label with the other values and add an ASSERT_NOT_REACHED() if appropriate-- you'll need to see if the other LengthTypes can be set to a height or width.
ptal... https://codereview.chromium.org/385583005/diff/90001/Source/core/rendering/Re... File Source/core/rendering/RenderBoxModelObject.cpp (right): https://codereview.chromium.org/385583005/diff/90001/Source/core/rendering/Re... Source/core/rendering/RenderBoxModelObject.cpp:2532: default: On 2014/07/16 16:57:23, tony wrote: > One of the reasons to use a switch statement is so you can enumerate all the > values and get a compiler error if someone adds a new type. Please replace the > |default| label with the other values and add an ASSERT_NOT_REACHED() if > appropriate-- you'll need to see if the other LengthTypes can be set to a height > or width. Done.
Levi or Ojan can you please take a look...
@Ojan, Christian - please take a look
Tab - can you confirm which edge margin-before refers to in case of vertical writing modes? In particular should percentage paddings for flex items always resolve against the height for padding-before? Correspondingly for padding-start and width.
On 2014/08/12 01:37:10, cbiesinger wrote: > Tab - can you confirm which edge margin-before refers to in case of vertical > writing modes? In particular should percentage paddings for flex items always > resolve against the height for padding-before? > > Correspondingly for padding-start and width. The -before properties refer to the start of the block axis, which depends on whether your'e vertical-lr or vertical-rl. In the former, "before" is right; in the latter, it's left. In terms of width/height, vertical writing modes map before/after to width, and start/end to height.
On 2014/08/12 01:43:33, Tab Atkins wrote: > On 2014/08/12 01:37:10, cbiesinger wrote: > > Tab - can you confirm which edge margin-before refers to in case of vertical > > writing modes? In particular should percentage paddings for flex items always > > resolve against the height for padding-before? > > > > Correspondingly for padding-start and width. > > The -before properties refer to the start of the block axis, which depends on > whether your'e vertical-lr or vertical-rl. In the former, "before" is right; in > the latter, it's left. In terms of width/height, vertical writing modes map > before/after to width, and start/end to height. Thanks, Tab! Unfortunately that means this patch is not correct :(
On 2014/08/12 01:37:10, cbiesinger wrote:
> Tab - can you confirm which edge margin-before refers to in case of vertical
> writing modes? In particular should percentage paddings for flex items always
> resolve against the height for padding-before?
@Chris : I am not always resolving the percentages padding-before against the
height. In my sloution i am taking containingBlockLogicalHeight which takes care
of vertical-writing modes and return width for vertical writing mode.
Even i have added 2 test cases at end of the layout test above for vertical
writing modes in which padding-top/bottom is resolved against width.
I might be missing something or might be wrong somewhere. Can you tell where i
am wrong in my solution or can tell some test cases where my patch will fail.
One of the test case which i have added for vertical writing mode:
<div class="flexbox verticalWritingMode" style="width:200px;" >
<div data-expected-padding-top="40" data-expected-padding-bottom="40"
style="padding: 20%"></div>
</div>
If vertical writing mode is there then it is taking width of containing block
otherwise it will take height.
On 2014/08/20 13:58:45, harpreet.sk wrote: > On 2014/08/12 01:37:10, cbiesinger wrote: > > Tab - can you confirm which edge margin-before refers to in case of vertical > > writing modes? In particular should percentage paddings for flex items always > > resolve against the height for padding-before? > > @Chris : I am not always resolving the percentages padding-before against the > height. In my sloution i am taking containingBlockLogicalHeight which takes care > of vertical-writing modes and return width for vertical writing mode. > > Even i have added 2 test cases at end of the layout test above for vertical > writing modes in which padding-top/bottom is resolved against width. > > I might be missing something or might be wrong somewhere. Can you tell where i > am wrong in my solution or can tell some test cases where my patch will fail. Sorry, I got it wrong, it is margin-top/margin-bottom which will not do the right thing (they should resolve to top/bottom even in vertical writing mode). Only margin-before/after should resolve to left/right. I am also really concerned about the performance implications of this additional check *every time* we are accessing a percentage padding value anywhere |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
