Bug 188711 – Wrong resolved value for inset CSS properties when there is overconstraintment

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

Run this code:
  document.body.style.cssText = "position: relative; top: 1px; bottom: 2px";
  getComputedStyle(document.body).bottom

Result:
  "-1px"

Expected:
  "2px"

It works well with Safari 5.1.
I didn't find any way to bisect further in Windows or Linux, and I don't have macOS.

[Simon Fraser (smfr)]

Can you attach an actual test case please?

[Oriol Brufau]

Created attachment 347507 [details]
testcase

[Oriol Brufau]

Created attachment 349324 [details]
Patch

[Oriol Brufau]

Created attachment 349333 [details]
Patch

[Oriol Brufau]

Created attachment 349350 [details]
Patch

[EWS Watchlist]

Comment on attachment 349350 [details]
Patch

Attachment 349350 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9167171

New failing tests:
imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output.html

[EWS Watchlist]

Created attachment 349369 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Manuel Rego Casasnovas]

Comment on attachment 349350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349350&action=review

> Source/WebCore/ChangeLog:11
> +         - In sticky positioning, the resolved value of inset properties is absolutizes

Nit: Typo s/absolutizes/absolutized/

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:789
> +        default:
> +            ASSERT_NOT_REACHED();

Mmm, this is usually at the end of the swtich.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:802
> +            if (!box.isOutOfFlowPositioned())

Isn't this always true after you've checkted box.isPositioned() earlier?

> LayoutTests/imported/w3c/ChangeLog:16
> +        Some tests still have failures due to unrelated issues.

It'd be nice to report these bugs and link them from here.

> LayoutTests/imported/w3c/ChangeLog:35
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-absolute-expected.txt: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-absolute.html: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-fixed-expected.txt: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-fixed.html: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-nobox-expected.txt: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-nobox.html: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-relative-expected.txt: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-relative.html: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-static-expected.txt: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-static.html: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-sticky-expected.txt: Added.
> +        * web-platform-tests/css/cssom/getComputedStyle-insets-sticky.html: Added.
> +        * web-platform-tests/css/cssom/support/getComputedStyle-insets.js: Added.
> +        (serialize):
> +        (wmName):
> +        (checkStyle):
> +        (runTestsWithWM):
> +        (export.runTests):

I guess we need a PR on GitHub to export these tests.
I belive it'd be nice to get it reviewed by other browser vendors too, to avoid misunderstandings.

Do these tests pass in other browsers?

[Oriol Brufau]

Comment on attachment 349350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349350&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:789
>> +            ASSERT_NOT_REACHED();
> 
> Mmm, this is usually at the end of the swtich.

I did that first, but EWS complained about 'potentially uninitialized local variable'.
I guess I will just use a conditional.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:802
>> +            if (!box.isOutOfFlowPositioned())
> 
> Isn't this always true after you've checkted box.isPositioned() earlier?

No, relative and sticky elements are positioned but not out-of-flow positioned.

>> LayoutTests/imported/w3c/ChangeLog:35
>> +        (export.runTests):
> 
> I guess we need a PR on GitHub to export these tests.
> I belive it'd be nice to get it reviewed by other browser vendors too, to avoid misunderstandings.
> 
> Do these tests pass in other browsers?

Tests for no box pass in Firefox, Chromium, WebKit.
Tests for static positioning pass in Firefox, Chromium, WebKit.
Tests for relative positioning pass in Firefox and WebKit, some fail in Chromium (because percentages are resolved with respect to the wrong direction in orthogonal flows).
Some tests for absolute positioning fail in Firefox (because when there is overconstrainment it provides the used value instead of the absolutized computed one), WebKit (because the static position is wrong in vertical writing modes), and Chromium (same problem as WebKit for abspos and Chromium for relpos).
Some tests for fixed positioning fail in Firefox (same problem as Firefox for abspos), WebKit (same problem as WebKit for abspos, and 'auto' value is increased by the padding of the containing block), and Chromium (same problem as Chromium for abspos, and percentages don't take padding into account)

[Oriol Brufau]

Created attachment 349435 [details]
Patch

[Oriol Brufau]

(In reply to Manuel Rego Casasnovas from comment #8)
> I guess we need a PR on GitHub to export these tests.

https://github.com/web-platform-tests/wpt/pull/12956

[EWS Watchlist]

Comment on attachment 349435 [details]
Patch

Attachment 349435 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9178258

New failing tests:
imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output.html

[EWS Watchlist]

Created attachment 349471 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349435 [details]
Patch

Attachment 349435 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9178810

New failing tests:
imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output.html

[EWS Watchlist]

Created attachment 349472 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349435 [details]
Patch

Attachment 349435 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9178193

New failing tests:
imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output.html

[EWS Watchlist]

Created attachment 349474 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 349435 [details]
Patch

Attachment 349435 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9179707

New failing tests:
imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output.html

[EWS Watchlist]

Created attachment 349482 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Oriol Brufau]

Created attachment 349549 [details]
Patch

[Oriol Brufau]

It seems Blink doesn't resolve percentages correctly in sticky positioning. So instead of aligning WebKit with Blink, I have changed the patch so that they continue resolving to the computed value without absolutizing percentages. Filed https://bugs.webkit.org/show_bug.cgi?id=189549

The test failures for cubic-bezier-timing-functions-output.html seemed caused by small difference between the computed and the resolved values due to floating precision issues. I have updated the expectations, the test was already failing anyways so it doesn't seem much important.

[Oriol Brufau]

Created attachment 350362 [details]
Patch

[Oriol Brufau]

Updated the WPT with changes from https://github.com/web-platform-tests/wpt/pull/13131

[Manuel Rego Casasnovas]

Comment on attachment 350362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350362&action=review

> LayoutTests/imported/w3c/ChangeLog:20
> +        This patch can slighlty alter the resolved value if it's a long decimal number,
> +        that's why test expectations for the timing functions test changed.

Why is this happening?

[Oriol Brufau]

Comment on attachment 350362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350362&action=review

>> LayoutTests/imported/w3c/ChangeLog:20
>> +        that's why test expectations for the timing functions test changed.
> 
> Why is this happening?

Previously it was the used value, which has a precision of 1/64. Now it's the absolutized computed value, which can have more precision.
```
Math.trunc(98.89859008789062*64)/64 === 98.890625
Math.trunc(106.3595962524414*64)/64 === 106.359375
Math.trunc(-17.50836753845215*64)/64 === -17.5
Math.trunc(512.0774536132812*64)/64 === 512.0625
```
I can reduce the precision manually if you think it's necessary.

[Manuel Rego Casasnovas]

Comment on attachment 350362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350362&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:802
> +                containingBlockSize -= containingBlock->paddingLogicalWidth();

I'm wondring if you could use containingBlockLogicalWidthForContent() for non positioned elements.
It's strange we're using the containingBlockLogicalWidthForPositioned() for both positioned and not positioned,
is this returning the proper value in both cases?

>>> LayoutTests/imported/w3c/ChangeLog:20
>>> +        that's why test expectations for the timing functions test changed.
>> 
>> Why is this happening?
> 
> Previously it was the used value, which has a precision of 1/64. Now it's the absolutized computed value, which can have more precision.
> ```
> Math.trunc(98.89859008789062*64)/64 === 98.890625
> Math.trunc(106.3595962524414*64)/64 === 106.359375
> Math.trunc(-17.50836753845215*64)/64 === -17.5
> Math.trunc(512.0774536132812*64)/64 === 512.0625
> ```
> I can reduce the precision manually if you think it's necessary.

No I don't think it's needed, thanks for the explanation.

[Oriol Brufau]

Created attachment 351676 [details]
Patch

[Oriol Brufau]

Comment on attachment 350362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350362&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:802
>> +                containingBlockSize -= containingBlock->paddingLogicalWidth();
> 
> I'm wondring if you could use containingBlockLogicalWidthForContent() for non positioned elements.
> It's strange we're using the containingBlockLogicalWidthForPositioned() for both positioned and not positioned,
> is this returning the proper value in both cases?

This was wrong in cases with overrideContainingBlockContentLogicalWidth. Fixed.

[Manuel Rego Casasnovas]

Comment on attachment 351676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351676&action=review

LGTM with nits.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:791
> +            if (box.isOutOfFlowPositioned())
> +                containingBlockSize = box.containingBlockLogicalHeightForPositioned(*containingBlock, false);
> +            else
> +                containingBlockSize = box.containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);

I'd use a ternary operator here:
  containingBlockSize = box.isOutOfFlowPositioned() ?
      box.containingBlockLogicalHeightForPositioned(*containingBlock, false) :
      box.containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:796
> +            if (box.isOutOfFlowPositioned())
> +                containingBlockSize = box.containingBlockLogicalWidthForPositioned(*containingBlock, nullptr, false);
> +            else
> +                containingBlockSize = box.containingBlockLogicalWidthForContent();

Ditto.

[Oriol Brufau]

Created attachment 351898 [details]
Patch

[Oriol Brufau]

Comment on attachment 351676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351676&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:791
>> +                containingBlockSize = box.containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);
> 
> I'd use a ternary operator here:
>   containingBlockSize = box.isOutOfFlowPositioned() ?
>       box.containingBlockLogicalHeightForPositioned(*containingBlock, false) :
>       box.containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);

check-webkit-style complained about this syntax, I have moved "?" and ":" to the next line

[Manuel Rego Casasnovas]

Comment on attachment 351898 [details]
Patch

Thanks.

BTW, you don't need to ask for review "r?" if it was already approved and just minor changes on the patch are made (like in this case).

[WebKit Commit Bot]

Comment on attachment 351898 [details]
Patch

Clearing flags on attachment: 351898

Committed r236979: <https://trac.webkit.org/changeset/236979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45137051>