The CSSOM spec only lists the following properties which needs to return used value as resolved value [1]: line-height height margin margin-bottom margin-left margin-right margin-top padding padding-bottom padding-left padding-right padding-top width bottom left right top However, we currently have much more properties return used value as resolve value, including {max,min}-{width,height}, border-widths, etc, which is not only a web-compat issue, but also makes us less performant when script tries to query them, because we need to flush layout. [1] https://drafts.csswg.org/cssom/#resolved-values

I think we also want to have more detailed check around whether a querying of resolved value actually needs flushing layout. e.g. if the dimension does not contain any percentage, we don't need flush layout at all; if line-height is not the special internal-only value, we don't need flush layout either.

This pages shows all properties for which we use used value while Blink uses computed value as the resolved value.

(The test page crashes Edge, and I don't know why...)

In addition to that list in comment 0, there are other properties which should use used value as resolved value from other specs: CSS Grid Layout [1] * grid-template-rows * grid-template-columns CSS Transforms * transform-origin [2] * perspective-origin [3] * transform, which has a special serialization rule that is basically just the used value [4] Also, Blink and us both agree on border-*-width should be using used value for resolved value, so I've proposed that the spec should be changed to match impls [5]. [1] https://drafts.csswg.org/css-grid/#resolved-track-list [2] https://drafts.csswg.org/css-transforms/#transform-origin-property [3] https://drafts.csswg.org/css-transforms/#perspective-origin-property [4] https://drafts.csswg.org/css-transforms/#serialization-of-the-computed-value [5] https://github.com/w3c/csswg-drafts/issues/393

How does this change match what other engines do? (This probably needs to be evaluated separately for each property being changed.)

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #10) > How does this change match what other engines do? (This probably needs to > be evaluated separately for each property being changed.) See comment 2.

Comment on attachment 8782836 [details] Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value. https://reviewboard.mozilla.org/r/72864/#review72280 r=me with the following nits addressed. ::: layout/style/nsComputedDOMStyle.cpp:4849 (Diff revision 2) > > already_AddRefed<CSSValue> > nsComputedDOMStyle::DoGetMaxHeight() > { > RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue; > - SetValueToCoord(val, StylePosition()->mMaxHeight, true, > + SetValueToCoord(val, StylePosition()->mMaxHeight, true, nullptr, nsCSSProps::kWidthKTable); This line exceeds 80 chars. ::: layout/style/nsComputedDOMStyle.cpp:4857 (Diff revision 2) > > already_AddRefed<CSSValue> > nsComputedDOMStyle::DoGetMaxWidth() > { > RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue; > - SetValueToCoord(val, StylePosition()->mMaxWidth, true, > + SetValueToCoord(val, StylePosition()->mMaxWidth, true, nullptr, nsCSSProps::kWidthKTable); This line exceeds 80 chars. ::: layout/style/nsComputedDOMStyle.cpp:4901 (Diff revision 2) > if (static_cast<nsFlexContainerFrame*>(flexContainer)->IsHorizontal()) { > minWidth.SetIntValue(NS_STYLE_WIDTH_MIN_CONTENT, eStyleUnit_Enumerated); > } > } > } > - SetValueToCoord(val, minWidth, true, > + Please remove this line.

Comment on attachment 8785137 [details] Bug 1292447: part 2 - Get border-*-radius and outline-radius-* props value resolved to computed value. https://reviewboard.mozilla.org/r/74440/#review72282 ::: layout/style/nsComputedDOMStyle.cpp:3314 (Diff revision 1) > nsStyleCoord radiusX, radiusY; > - if (mInnerFrame && aIsBorder) { > + > - nscoord radii[8]; > - mInnerFrame->GetBorderRadii(radii); > - radiusX.SetCoordValue(radii[NS_FULL_TO_HALF_CORNER(aFullCorner, false)]); > - radiusY.SetCoordValue(radii[NS_FULL_TO_HALF_CORNER(aFullCorner, true)]); > - } else { > - radiusX = aRadius.Get(NS_FULL_TO_HALF_CORNER(aFullCorner, false)); > + radiusX = aRadius.Get(NS_FULL_TO_HALF_CORNER(aFullCorner, false)); > - radiusY = aRadius.Get(NS_FULL_TO_HALF_CORNER(aFullCorner, true)); > + radiusY = aRadius.Get(NS_FULL_TO_HALF_CORNER(aFullCorner, true)); Please merge the declaration into the lines below. ::: layout/style/nsComputedDOMStyle.cpp (Diff revision 1) > - if (mInnerFrame) { > - // We need to convert to absolute coordinates before doing the > - // equality check below. > - nscoord v; > - > - v = StyleCoordToNSCoord(radiusX, > - &nsComputedDOMStyle::GetFrameBorderRectWidth, > - 0, true); > - radiusX.SetCoordValue(v); > - > - v = StyleCoordToNSCoord(radiusY, > - &nsComputedDOMStyle::GetFrameBorderRectHeight, > - 0, true); > - radiusY.SetCoordValue(v); > - } > - } This is probably wrong. If (mInnerFrame & aIsBorder) is false, but mInnerFrame is not nullptr, then aIsBorder is false, so this code is actually for outline radius. Outline radius is not something standardized, so it is probably fine making it resolved to computed value as well. But that needs corresponding changes in nsCSSPropList.h in addition. (You can do that in the same patch.) Also, if we resolve both border-radius and outline-radius to computed value, "aIsBorder" would become useless. Please remove that parameter at the same time in that case.

Comment on attachment 8785138 [details] Bug 1292447: part 3 - Get text-indent prop resolved to computed value. https://reviewboard.mozilla.org/r/74442/#review72284

Comment on attachment 8785139 [details] Bug 1292447: part 4 - Get vertical-align prop resolved to computed value. https://reviewboard.mozilla.org/r/74444/#review72286

Comment on attachment 8782836 [details] Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value. https://reviewboard.mozilla.org/r/72864/#review72288 r=me with Xidorn's comments addressed.

Comment on attachment 8782836 [details] Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value. https://reviewboard.mozilla.org/r/72864/#review72290

Comment on attachment 8782836 [details] Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value. https://reviewboard.mozilla.org/r/72864/#review72292

Comment on attachment 8785137 [details] Bug 1292447: part 2 - Get border-*-radius and outline-radius-* props value resolved to computed value. https://reviewboard.mozilla.org/r/74440/#review72294

Comment on attachment 8785138 [details] Bug 1292447: part 3 - Get text-indent prop resolved to computed value. https://reviewboard.mozilla.org/r/74442/#review72296

Comment on attachment 8785139 [details] Bug 1292447: part 4 - Get vertical-align prop resolved to computed value. https://reviewboard.mozilla.org/r/74444/#review72298

Comment on attachment 8782836 [details] Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value. https://reviewboard.mozilla.org/r/72864/#review72280 Addressed and patch updated.

Comment on attachment 8785137 [details] Bug 1292447: part 2 - Get border-*-radius and outline-radius-* props value resolved to computed value. https://reviewboard.mozilla.org/r/74440/#review72282 Addressed and patch updated. Thanks for review. > This is probably wrong. If (mInnerFrame & aIsBorder) is false, but mInnerFrame is not nullptr, then aIsBorder is false, so this code is actually for outline radius. > > Outline radius is not something standardized, so it is probably fine making it resolved to computed value as well. But that needs corresponding changes in nsCSSPropList.h in addition. (You can do that in the same patch.) > > Also, if we resolve both border-radius and outline-radius to computed value, "aIsBorder" would become useless. Please remove that parameter at the same time in that case. Review comments addressed. Keep outline-radius prefixed with -moz and removed flush layout flag in nsCSSPropList.h.

Comment on attachment 8785137 [details] Bug 1292447: part 2 - Get border-*-radius and outline-radius-* props value resolved to computed value. https://reviewboard.mozilla.org/r/74440/#review72328 The commit message should mention outline-radius as well. r=me with the commit message updated.

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ef785c3d13b&selectedJob=26392120 Need to update a few mochitest tests(assumed resolve value is used value) to pass through try. Will append a new patch to address try failures.

Comment on attachment 8789243 [details] Bug 1292447: part 5 - Update property_database.js. https://reviewboard.mozilla.org/r/77522/#review75856 ::: layout/style/test/property_database.js:1152 (Diff revision 1) > domProp: "borderRadius", > inherited: false, > type: CSS_TYPE_TRUE_SHORTHAND, > prerequisites: { "width": "200px", "height": "100px", "display": "inline-block"}, > subproperties: [ "border-bottom-left-radius", "border-bottom-right-radius", "border-top-left-radius", "border-top-right-radius" ], > - initial_values: [ "0", "0px", "0%", "0px 0 0 0px", "calc(-2px)", "calc(-1%)", "calc(0px) calc(0pt) calc(0%) calc(0em)" ], > + initial_values: [ "0", "0px", "0px 0 0 0px", "calc(-2px)", "calc(0px) calc(0pt)", "calc(0px) calc(0em)" ], Any reason to divide "calc(0px) calc(0pt) calc(0%) calc(0em)" into two tests? Anyway, borders-radius takes two-value form, so I guess it's OK.

Comment on attachment 8789244 [details] Bug 1292447: part 6 - Update test_computed_style.html. https://reviewboard.mozilla.org/r/77524/#review75860 ::: layout/style/test/test_computed_style.html:77 (Diff revision 1) > > p.parentNode.removeChild(p); > })(); > > -(function test_bug_595651() { > - // Test that clamping of border-radius is reflected in computed style. > +(function test_bug_1292447() { > + // Was for bug 595651 which test that clamping of border-radius "which tests". This test seems losing its original purpose. xidorn, do you think it's worth keeping it?

Comment on attachment 8789244 [details] Bug 1292447: part 6 - Update test_computed_style.html. https://reviewboard.mozilla.org/r/77524/#review75862

Update try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=163c863494cee634987828e36bf9976bb14c10b8 Looks like there is a new failure found as below: 130 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/attributes/test_obj_css.html | Attribute 'text-indent' has wrong value for 'text-indent_percent' - got "47.1px", expected "10%" Keep fixing.

Comment on attachment 8789244 [details] Bug 1292447: part 6 - Update test_computed_style.html. https://reviewboard.mozilla.org/r/77524/#review75860 > "which tests". > > This test seems losing its original purpose. xidorn, do you think it's worth keeping it? Hmmm, I think it's probably worth, for ensuring that we do not clamp it by accident in the future. (I don't know how it could be accidently clampped, though).

Comment on attachment 8789244 [details] Bug 1292447: part 6 - Update test_computed_style.html. https://reviewboard.mozilla.org/r/77524/#review76094 r=me with the comment issue addressed.

Comment on attachment 8789245 [details] Bug 1292447: part 7 - Update test_bug1292447.html. https://reviewboard.mozilla.org/r/77526/#review76120 I assume your refactor is to allow property test of elements $(id)-2 diverged so that those resolved value becomes computed value after your patches go to the percentValueTest(). ::: layout/style/test/test_bug1292447.html:317 (Diff revision 1) > + is(round(decl.getPropertyCSSValue(cssProp) > + .getFloatValue(CSSPrimitiveValue.CSS_PERCENTAGE), > + 3), percentVal, prettyName + " as float value"); > +} > + > +function doATest(propName, idBase, coordVal, percentVal, resolvedToUsed = false) { Nit: Might be better called 'resolveToUsedVal' ::: layout/style/test/test_bug1292447.html:371 (Diff revision 1) > - is(decl[cssPropertiesPropName], coordVal+"px", prettyName); > - is(decl.getPropertyCSSValue(propName).cssValueType, > - CSSValue.CSS_PRIMITIVE_VALUE, prettyName + " is primitive value"); > - is(decl.getPropertyCSSValue(propName).primitiveType, > - CSSPrimitiveValue.CSS_PX, prettyName + " is px"); > - is(decl.getPropertyCSSValue(propName).cssText, coordVal + "px", > + propName + " of " + idBase + "3"); > + > + /* Test $(id)-4 */ > + percentValueTest(cssCamelPropName, propName, > + style(idBase + "4"), percentVal, > + propName + " of " + idBase + "4"); Nit: While you're here, please remove the trailing whitespaces.

Comment on attachment 8789246 [details] Bug 1292447: part 8 - Update test_transitions_per_property.html. https://reviewboard.mozilla.org/r/77528/#review76136

Comment on attachment 8789247 [details] Bug 1292447: part 9 - Update test_value_computation.html. https://reviewboard.mozilla.org/r/77530/#review76138

Comment on attachment 8782836 [details] Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value. https://reviewboard.mozilla.org/r/72864/#review77466 ::: layout/style/nsComputedDOMStyle.cpp:4884 (Diff revision 4) > if (eStyleUnit_Auto == minHeight.GetUnit()) { > // In non-flexbox contexts, "min-height: auto" means "min-height: 0" > // XXXdholbert For flex items, we should set |minHeight| to the > // -moz-min-content keyword, instead of 0, once we support -moz-min-content > // as a height value. > minHeight.SetCoordValue(0); > } We probably also want to remove this code, as "auto" itself is computed value. ::: layout/style/nsComputedDOMStyle.cpp:4903 (Diff revision 4) > if (eStyleUnit_Auto == minWidth.GetUnit()) { > // "min-width: auto" means "0", unless we're a flex item in a horizontal > // flex container, in which case it means "min-content" > minWidth.SetCoordValue(0); > if (mOuterFrame && mOuterFrame->IsFlexItem()) { > nsIFrame* flexContainer = mOuterFrame->GetParent(); > MOZ_ASSERT(flexContainer && > flexContainer->GetType() == nsGkAtoms::flexContainerFrame, > "IsFlexItem() lied...?"); > > if (static_cast<nsFlexContainerFrame*>(flexContainer)->IsHorizontal()) { > minWidth.SetIntValue(NS_STYLE_WIDTH_MIN_CONTENT, eStyleUnit_Enumerated); > } > } > } Here as well.

Comment on attachment 8789243 [details] Bug 1292447: part 5 - Update property_database.js. https://reviewboard.mozilla.org/r/77522/#review75856 > Any reason to divide "calc(0px) calc(0pt) calc(0%) calc(0em)" into two tests? Anyway, borders-radius takes two-value form, so I guess it's OK. I'll leave the 4-values as-is but change % to abs value, ex, px. 2-values initial value test will be added as well.

Comment on attachment 8796557 [details] Bug 1292447: part 10 - Get a11y StyleInfo::TextIndent value resolved correctly. https://reviewboard.mozilla.org/r/82378/#review81024 r=me, it looks like the change is just to keep the things exactly same as they were before. However, I was surprised a bit that text-indent, given in percents, is returned in percents too to AT, since I bet AT would prefer to deal with one unit. Anyway I dind't find text-indent attribute on IA2 spec (https://wiki.linuxfoundation.org/accessibility/iaccessible2/textattributes), so let's not wake a sleeping dog.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2521ec649f2f

Comment on attachment 8796557 [details] Bug 1292447: part 10 - Get a11y StyleInfo::TextIndent value resolved correctly. https://reviewboard.mozilla.org/r/82378/#review81024 Thanks for your review and let's deal with it after the dog is awoke. :)

Comment on attachment 8796557 [details] Bug 1292447: part 10 - Get a11y StyleInfo::TextIndent value resolved correctly. https://reviewboard.mozilla.org/r/82378/#review81118 Carry r+

Comment on attachment 8796557 [details] Bug 1292447: part 10 - Get a11y StyleInfo::TextIndent value resolved correctly. https://reviewboard.mozilla.org/r/82378/#review81350 mozreview doesn't recognize r+ from surkov, so I add a r+.

Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/07e1d8a21134 part 1 - Get {min,max}-{width,height} prop resolved to computed value. r=TYLin,xidorn https://hg.mozilla.org/integration/autoland/rev/925bf6f0413c part 2 - Get border-*-radius and outline-radius-* props value resolved to computed value. r=xidorn https://hg.mozilla.org/integration/autoland/rev/849fe06602a9 part 3 - Get text-indent prop resolved to computed value. r=TYLin,xidorn https://hg.mozilla.org/integration/autoland/rev/4c1287558974 part 4 - Get vertical-align prop resolved to computed value. r=TYLin,xidorn https://hg.mozilla.org/integration/autoland/rev/bd34ad7c9624 part 5 - Update property_database.js. r=TYLin https://hg.mozilla.org/integration/autoland/rev/73f05d1f7db5 part 6 - Update test_computed_style.html. r=TYLin,xidorn https://hg.mozilla.org/integration/autoland/rev/d1b7c10168bc part 7 - Update test_bug1292447.html. r=TYLin https://hg.mozilla.org/integration/autoland/rev/08c21952f50d part 8 - Update test_transitions_per_property.html. r=TYLin https://hg.mozilla.org/integration/autoland/rev/42225a3fea65 part 9 - Update test_value_computation.html. r=TYLin https://hg.mozilla.org/integration/autoland/rev/7b9b82c81d19 part 10 - Get a11y StyleInfo::TextIndent value resolved correctly. r=surkov,TYLin

https://hg.mozilla.org/mozilla-central/rev/07e1d8a21134 https://hg.mozilla.org/mozilla-central/rev/925bf6f0413c https://hg.mozilla.org/mozilla-central/rev/849fe06602a9 https://hg.mozilla.org/mozilla-central/rev/4c1287558974 https://hg.mozilla.org/mozilla-central/rev/bd34ad7c9624 https://hg.mozilla.org/mozilla-central/rev/73f05d1f7db5 https://hg.mozilla.org/mozilla-central/rev/d1b7c10168bc https://hg.mozilla.org/mozilla-central/rev/08c21952f50d https://hg.mozilla.org/mozilla-central/rev/42225a3fea65 https://hg.mozilla.org/mozilla-central/rev/7b9b82c81d19

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.