Closed Bug 1292447 Opened 8 years ago Closed 8 years ago

Shrink the number of properties which return used value as resolved value for getComputedStyle

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: bmo)

References

Details

Attachments

(11 files)

test page
2.36 KB, text/html
Details
Bug 1292447: part 1 - Get {min,max}-{width,height} prop resolved to computed value.
58 bytes, text/x-review-board-request
xidorn
: review+
TYLin
: review+
Details
Bug 1292447: part 2 - Get border-*-radius and outline-radius-* props value resolved to computed value.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1292447: part 3 - Get text-indent prop resolved to computed value.
58 bytes, text/x-review-board-request
TYLin
: review+
xidorn
: review+
Details
Bug 1292447: part 4 - Get vertical-align prop resolved to computed value.
58 bytes, text/x-review-board-request
TYLin
: review+
xidorn
: review+
Details
Bug 1292447: part 5 - Update property_database.js.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1292447: part 6 - Update test_computed_style.html.
58 bytes, text/x-review-board-request
TYLin
: review+
xidorn
: review+
Details
Bug 1292447: part 7 - Update test_bug1292447.html.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1292447: part 8 - Update test_transitions_per_property.html.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1292447: part 9 - Update test_value_computation.html.
58 bytes, text/x-review-board-request
TYLin
: review+
Details
Bug 1292447: part 10 - Get a11y StyleInfo::TextIndent value resolved correctly.
58 bytes, text/x-review-board-request
surkov
: review+
Details 
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.
Attached file test page — 
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
Flags: needinfo?(bugs)
Assignee: nobody → aschen
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
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.
Attachment #8782836 - Flags: review?(xidorn+moz)
Attachment #8782836 - Flags: review?(tlin)
Attachment #8785137 - Flags: review?(xidorn+moz)
Attachment #8785137 - Flags: review?(tlin)
Attachment #8785138 - Flags: review?(xidorn+moz)
Attachment #8785138 - Flags: review?(tlin)
Attachment #8785139 - Flags: review?(xidorn+moz)
Attachment #8785139 - Flags: review?(tlin)
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.
Attachment #8782836 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8785137 - Flags: review?(xidorn+moz) → review-
Comment on attachment 8785138 [details]
Bug 1292447: part 3 - Get text-indent prop resolved to computed value.

https://reviewboard.mozilla.org/r/74442/#review72284
Attachment #8785138 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8785139 [details]
Bug 1292447: part 4 - Get vertical-align prop resolved to computed value.

https://reviewboard.mozilla.org/r/74444/#review72286
Attachment #8785139 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8782836 - Flags: review?(tlin) → review+
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
Attachment #8782836 - Flags: review+
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
Attachment #8782836 - Flags: review+
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
Attachment #8785137 - Flags: review?(tlin)
Comment on attachment 8785138 [details]
Bug 1292447: part 3 - Get text-indent prop resolved to computed value.

https://reviewboard.mozilla.org/r/74442/#review72296
Attachment #8785138 - Flags: review?(tlin) → review+
Comment on attachment 8785139 [details]
Bug 1292447: part 4 - Get vertical-align prop resolved to computed value.

https://reviewboard.mozilla.org/r/74444/#review72298
Attachment #8785139 - Flags: review?(tlin) → review+
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.
Attachment #8785137 - Flags: review?(xidorn+moz) → review+
Attachment #8785137 - Flags: review?(tlin)
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.
Depends on: 365932
Attachment #8789243 - Flags: review?(tlin)
Attachment #8789244 - Flags: review?(tlin)
Attachment #8789245 - Flags: review?(tlin)
Attachment #8789246 - Flags: review?(tlin)
Attachment #8789247 - Flags: review?(tlin)
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.
Attachment #8789243 - Flags: review?(tlin) → review+
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?
Attachment #8789244 - Flags: review?(xidorn+moz)
Comment on attachment 8789244 [details]
Bug 1292447: part 6 - Update test_computed_style.html.

https://reviewboard.mozilla.org/r/77524/#review75862
Attachment #8789244 - Flags: review?(tlin) → review+
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.
Attachment #8789244 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8789245 - Flags: review?(tlin) → review+
Comment on attachment 8789246 [details]
Bug 1292447: part 8 - Update test_transitions_per_property.html.

https://reviewboard.mozilla.org/r/77528/#review76136
Attachment #8789246 - Flags: review?(tlin) → review+
Comment on attachment 8789247 [details]
Bug 1292447: part 9 - Update test_value_computation.html.

https://reviewboard.mozilla.org/r/77530/#review76138
Attachment #8789247 - Flags: review?(tlin) → review+
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.
Blocks: 1304636
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.
Attachment #8796557 - Flags: review?(surkov.alexander)
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.
Attachment #8796557 - Flags: review?(surkov.alexander) → review+
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+
Attachment #8796557 - Flags: review+
Attachment #8796557 - Flags: review?(tlin)
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+.
Attachment #8796557 - Flags: review?(tlin) → review+
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
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: