Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577363002:
Implement CSS Logical Properties: inline/block size (Closed)
|
Created:
4 years ago by xing.xu Modified:
3 years, 11 months ago CC:
darktears, apavlov+blink_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement CSS Logical Properties: inline/block size
These properties provide the author with the ability to control layout through logical,
rather than physical, direction and dimension mappings.
Intent to Implement and Ship: CSS Logical Properties, Inline/Block size,
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent
is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}",
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc.
When entire css-logical-props are implemented, will review these
-webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them.
Spec:
https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties
https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical
BUG=538475
Review-Url: https://codereview.chromium.org/2577363002
Cr-Commit-Position: refs/heads/master@{#441979}
Committed: https://chromium.googlesource.com/chromium/src/+/27fa89644b9c8ea68e0445ff6f170acdc3eb0933
Patch Set 1 #Patch Set 2 : Fix webexposed test #
Total comments: 2
Patch Set 3 : Fix alphabetical order #Patch Set 4 : Refactor test case, add {inline-,block-}size overrides width height #
Total comments: 4
Patch Set 5 : Refactor reftest to testharness.js #Patch Set 6 : Refactor test case, add more table case #Patch Set 7 : Refine case, add quirky length case #Patch Set 8 : Refactor test case #Messages
Total messages: 76 (49 generated)
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ==========
xing.xu@intel.com changed reviewers: + cbiesinger@chromium.org, foolip@chromium.org, jochen@chromium.org, timloh@chromium.org
PTAL. Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs This is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc). From this Intent, agreed to implement {inline-,block-}size and {min-,max-}{inline-,block-}size first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'd happy to leave the review to the style/layout experts, but let me know if you need a rubberstamp at the end. Will take a look at the intent now.
A few comments below, but I'll leave final review to a style person. https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp (right): https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:3376: case CSSPropertyInlineSize: Please put these in alphabetical order (ie. above the Webkit ones) https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right): https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:41: case CSSPropertyInlineSize: these too
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/16 18:51:50, cbiesinger wrote: > A few comments below, but I'll leave final review to a style person. > > https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp > (right): > > https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:3376: case > CSSPropertyInlineSize: > Please put these in alphabetical order (ie. above the Webkit ones) > > https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right): > > https://codereview.chromium.org/2577363002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:41: case > CSSPropertyInlineSize: > these too order is done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
please link to the intent to ship from the CL description.
UseCounter.cpp / histograms.xml lgtm
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ==========
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ==========
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ==========
On 2016/12/19 11:53:39, jochen wrote: > please link to the intent to ship from the CL description. Done
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ==========
PTAL, thanks
cbiesinger@chromium.org changed reviewers: + dstockwell@chromium.org, rune@opera.com
Adding some more style reviewers who are hopefully not OOO. Also see nit below. https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right): https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:28: case CSSPropertyMinBlockSize: This is not actually sorted?
I am not sure if I understand your idea clearly.
In CSSParserFastPaths.cpp file, it seems items are sorted by the first item in a
group, and a group includes items which are logical related.
Such as this:
/*Font group, 1st is CSSPropertyF*/
case CSSPropertyFontSize:
/*Grid group, 1st is CSSPropertyG*/
case CSSPropertyGridColumnGap:
case CSSPropertyGridRowGap:
/*Width/Height group,1st is CSSPropertyH*/
case CSSPropertyHeight:
case CSSPropertyWidth:
/*MinWidth/MinHeight group*/
case CSSPropertyMinHeight:
case CSSPropertyMinWidth:
/*Padding group,1st is CSSPropertyP*/
case CSSPropertyPaddingBottom:
case CSSPropertyPaddingLeft:
case CSSPropertyPaddingRight:
case CSSPropertyPaddingTop:
/*WebKit group,1st is CSSPropertyW*/
case CSSPropertyWebkitLogicalWidth:
case CSSPropertyWebkitLogicalHeight:
case CSSPropertyWebkitMinLogicalWidth:
case CSSPropertyWebkitMinLogicalHeight:
case CSSPropertyWebkitPaddingAfter:
case CSSPropertyWebkitPaddingBefore:
case CSSPropertyWebkitPaddingEnd:
case CSSPropertyWebkitPaddingStart:
If I add the Inline-Block size group, first item is CSSPropertyB*, that's why I
put this at the start:
/*Inline-Block size group,1st is CSSPropertyB*/
case CSSPropertyBlockSize:
case CSSPropertyInlineSize:
case CSSPropertyMinBlockSize:
case CSSPropertyMinInlineSize:
On 2016/12/21 23:15:00, cbiesinger wrote:
> Adding some more style reviewers who are hopefully not OOO.
>
> Also see nit below.
>
>
https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):
>
>
https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:28: case
> CSSPropertyMinBlockSize:
> This is not actually sorted?
And the following two group:
/*Width/Height group,1st is CSSPropertyH*/
case CSSPropertyHeight:
case CSSPropertyWidth:
/*MinWidth/MinHeight group*/
case CSSPropertyMinHeight:
case CSSPropertyMinWidth:
may be merged into one, because they are logical related:
/*Width/Height/MinWidth/MinHeight group,1st is CSSPropertyH*/
case CSSPropertyHeight:
case CSSPropertyWidth:
case CSSPropertyMinHeight:
case CSSPropertyMinWidth:
On 2016/12/22 01:31:34, xing.xu wrote:
> I am not sure if I understand your idea clearly.
> In CSSParserFastPaths.cpp file, it seems items are sorted by the first item in
a
> group, and a group includes items which are logical related.
> Such as this:
> /*Font group, 1st is CSSPropertyF*/
> case CSSPropertyFontSize:
> /*Grid group, 1st is CSSPropertyG*/
> case CSSPropertyGridColumnGap:
> case CSSPropertyGridRowGap:
> /*Width/Height group,1st is CSSPropertyH*/
> case CSSPropertyHeight:
> case CSSPropertyWidth:
> /*MinWidth/MinHeight group*/
> case CSSPropertyMinHeight:
> case CSSPropertyMinWidth:
> /*Padding group,1st is CSSPropertyP*/
> case CSSPropertyPaddingBottom:
> case CSSPropertyPaddingLeft:
> case CSSPropertyPaddingRight:
> case CSSPropertyPaddingTop:
> /*WebKit group,1st is CSSPropertyW*/
> case CSSPropertyWebkitLogicalWidth:
> case CSSPropertyWebkitLogicalHeight:
> case CSSPropertyWebkitMinLogicalWidth:
> case CSSPropertyWebkitMinLogicalHeight:
> case CSSPropertyWebkitPaddingAfter:
> case CSSPropertyWebkitPaddingBefore:
> case CSSPropertyWebkitPaddingEnd:
> case CSSPropertyWebkitPaddingStart:
>
>
> If I add the Inline-Block size group, first item is CSSPropertyB*, that's why
I
> put this at the start:
> /*Inline-Block size group,1st is CSSPropertyB*/
> case CSSPropertyBlockSize:
> case CSSPropertyInlineSize:
> case CSSPropertyMinBlockSize:
> case CSSPropertyMinInlineSize:
>
>
>
> On 2016/12/21 23:15:00, cbiesinger wrote:
> > Adding some more style reviewers who are hopefully not OOO.
> >
> > Also see nit below.
> >
> >
>
https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp
(right):
> >
> >
>
https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:28: case
> > CSSPropertyMinBlockSize:
> > This is not actually sorted?
Oh, it seems Width/Height group and MinWidth/MinHeight group not merged in
current implementation.
So, do you means, I should split below
/*Inline-Block size group,1st is CSSPropertyB*/
case CSSPropertyBlockSize:
case CSSPropertyInlineSize:
case CSSPropertyMinBlockSize:
case CSSPropertyMinInlineSize:
into two groups, and like this?
case CSSPropertyBlockSize:
case CSSPropertyInlineSize:
case CSSPropertyFontSize:
case CSSPropertyGridColumnGap:
case CSSPropertyGridRowGap:
case CSSPropertyHeight:
case CSSPropertyWidth:
/*Move to here:*/
case CSSPropertyMinBlockSize:
case CSSPropertyMinInlineSize:
case CSSPropertyMinHeight:
case CSSPropertyMinWidth:
case CSSPropertyPaddingBottom:
case CSSPropertyPaddingLeft:
case CSSPropertyPaddingRight:
case CSSPropertyPaddingTop:
case CSSPropertyWebkitLogicalWidth:
case CSSPropertyWebkitLogicalHeight:
case CSSPropertyWebkitMinLogicalWidth:
case CSSPropertyWebkitMinLogicalHeight:
case CSSPropertyWebkitPaddingAfter:
case CSSPropertyWebkitPaddingBefore:
case CSSPropertyWebkitPaddingEnd:
case CSSPropertyWebkitPaddingStart:
I guess I'll let a style owner comment on that
Source/core/css lgtm. We don't seem to be strict in the switch case ordering in that part of the code (seems "logically" ordered like width comes before height), but feel free to fix the places you modify. You should upstream tests to csswg (no directory css-logical-props yet, so I'm assuming there's no coverage) and remove these tests when importing back. I saw foolip mentioned there's a process in the works for pushing such test from the chromium repo to github, but I don't know if that's the preferred way yet. https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/logical-props/logicalprops-block-size-vlr.html (right): https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/logical-props/logicalprops-block-size-vlr.html:3: <title>CSS Logical Properties: {max-,min-}block-size vertical-lr</title> We normally avoid unnecessary elements like <title> for internal tests, but this looks like it's prepared for csswg test upstreaming. Do you have a plan to do so? https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right): https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:28: case CSSPropertyMinBlockSize: On 2016/12/21 23:15:00, cbiesinger wrote: > This is not actually sorted? If you put these into order, it would make sense to fix the order for the rest of the properties too.
I have no plan to upstream to csswg. But if necessary, I would like to do it. So, it it OK to click the commit button now? As to the alpha order, as you suggested, maybe we should work out the "preferred way" first. If the "preferred way" is clear, I will upload a CL to fix this. On 2016/12/22 11:29:34, rune wrote: > Source/core/css lgtm. > > We don't seem to be strict in the switch case ordering in that part of the code > (seems "logically" ordered like width comes before height), but feel free to fix > the places you modify. > > You should upstream tests to csswg (no directory css-logical-props yet, so I'm > assuming there's no coverage) and remove these tests when importing back. I saw > foolip mentioned there's a process in the works for pushing such test from the > chromium repo to github, but I don't know if that's the preferred way yet. > > https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/css3/logical-props/logicalprops-block-size-vlr.html > (right): > > https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/css3/logical-props/logicalprops-block-size-vlr.html:3: > <title>CSS Logical Properties: {max-,min-}block-size vertical-lr</title> > We normally avoid unnecessary elements like <title> for internal tests, but this > looks like it's prepared for csswg test upstreaming. Do you have a plan to do > so? > > https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right): > > https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:28: case > CSSPropertyMinBlockSize: > On 2016/12/21 23:15:00, cbiesinger wrote: > > This is not actually sorted? > > If you put these into order, it would make sense to fix the order for the rest > of the properties too.
On 2016/12/22 11:57:47, xing.xu wrote: > I have no plan to upstream to csswg. But if necessary, I would like to do it. For new features without test coverage in csswg test or wpt, we should. It's important for interoperability and the platform predictability project. > So, it it OK to click the commit button now? I didn't look too close at the tests, I think you should use testharness for the tests as mentioned inline. > As to the alpha order, as you suggested, maybe we should work out the "preferred > way" first. > If the "preferred way" is clear, I will upload a CL to fix this. You can keep it as is.
https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/logical-props/logicalprops-block-size.html (right): https://codereview.chromium.org/2577363002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/logical-props/logicalprops-block-size.html:42: <div id="div3">Hello, this is a test</div> I think using testharness.js and check computed width and height would be preferred here. Currently there's no clear pass condition without checking the reference.
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, tune@, all cases were re-factored to testharness.js, PTAL.
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/23 05:11:19, xing.xu wrote: > Hi, tune@, all cases were re-factored to testharness.js, PTAL. I've reviewed tests upstream now.
lgtm but a couple of things for you to do first. Could you test that the logical properties do not accept quirky lengths? The physical properties accept these but the logical properties should not (the implementation already does this correctly). For example, have a div with styling like "block-size: 10;" and check that the value is not set (or "block-size: 20px; block-size: 10" and check it's 20px). Note you can't check these with CSS.supports because that function never supports quirky lengths, and the document must be in quirks mode for this to work. https://quirks.spec.whatwg.org/#the-unitless-length-quirk Could you also, in the patch description, mention the intended schedule for removing the prefixed properties?
dstockwell@chromium.org changed reviewers: - dstockwell@chromium.org
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When logical properties are done, will review these webkit-{min-,max-}logical-{width,height}. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ==========
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When logical properties are done, will review these webkit-{min-,max-}logical-{width,height}. Spec: https://drafts.csswg.org/css-logical-props/ https://drafts.csswg.org/css-writing-modes/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When logical properties are done, will review these webkit-{min-,max-}logical-{width,height}. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ==========
Add the quirky related length case.
Add "When logical properties are done, will review these
webkit-{min-,max-}logical-{width,height}."
in the patch description, is it OK?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/04 06:51:23, xing.xu wrote:
> Add the quirky related length case.
> Add "When logical properties are done, will review these
> webkit-{min-,max-}logical-{width,height}."
> in the patch description, is it OK?
I was hoping for something more concrete. I can't tell for example whether you
want to remove it immediately after this lands, or wait a few months for the
unprefixed version to ship to stable and then remove it, or wait for the entire
css-logical-props to be implemented first.
I think it's best to wait for the entire css-logical-props to be implemented.
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When logical properties are done, will review these webkit-{min-,max-}logical-{width,height}. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ==========
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ==========
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ==========
The CQ bit was checked by xing.xu@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Refactor test case base on https://github.com/w3c/csswg-test/pull/1165. PTAL. BTW, now that there are two lgtm, and csswg-test gets 1 lgtm. is it ok to click the commit button?
On 2017/01/06 13:25:06, xing.xu wrote: > Refactor test case base on https://github.com/w3c/csswg-test/pull/1165. PTAL. > > BTW, now that there are two lgtm, and csswg-test gets 1 lgtm. is it ok to click > the commit button? Yes, once your csswg tests land they should be imported and the tests here removed. See if you can make sense of https://www.chromium.org/blink/importing-the-w3c-tests Thanks for upstreaming the tests!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by xing.xu@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, rune@opera.com, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2577363002/#ps140001 (title: "Refactor test case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483719734756020,
"parent_rev": "00d50f2e63ac31615026ad0f09e3a7b196fde53d", "commit_rev":
"27fa89644b9c8ea68e0445ff6f170acdc3eb0933"}
Message was sent while issue was closed.
Description was changed from ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 ========== to ========== Implement CSS Logical Properties: inline/block size These properties provide the author with the ability to control layout through logical, rather than physical, direction and dimension mappings. Intent to Implement and Ship: CSS Logical Properties, Inline/Block size, https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4qEXWptfVHs. This intent is a followup for "Intent to Deprecate and Remove: -webkit-{min-,max-}logical-{width,height}", https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VbPI65jBMIc. When entire css-logical-props are implemented, will review these -webkit-{min-,max-}logical-{width,height} and see if it is necessary to remove them. Spec: https://drafts.csswg.org/css-logical-props-1/#logical-dimension-properties https://drafts.csswg.org/css-writing-modes-3/#logical-to-physical BUG=538475 Review-Url: https://codereview.chromium.org/2577363002 Cr-Commit-Position: refs/heads/master@{#441979} Committed: https://chromium.googlesource.com/chromium/src/+/27fa89644b9c8ea68e0445ff6f17... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/27fa89644b9c8ea68e0445ff6f17... |
