Bug 188711 - Wrong resolved value for inset CSS properties when there is overconstraintment
Summary: Wrong resolved value for inset CSS properties when there is overconstraintment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar, Regression
Depends on:
Blocks: 189441
  Show dependency treegraph
 
Reported: 2018-08-17 16:09 PDT by Oriol Brufau
Modified: 2018-10-09 13:12 PDT (History)
8 users (show)

See Also:


Attachments
testcase (176 bytes, text/html)
2018-08-20 10:37 PDT, Oriol Brufau
no flags Details
Patch (187.20 KB, patch)
2018-09-10 13:29 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (187.32 KB, patch)
2018-09-10 14:56 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (187.25 KB, patch)
2018-09-10 16:14 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (3.04 MB, application/zip)
2018-09-10 18:51 PDT, EWS Watchlist
no flags Details
Patch (187.24 KB, patch)
2018-09-11 13:27 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (3.14 MB, application/zip)
2018-09-11 15:16 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.87 MB, application/zip)
2018-09-11 15:17 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.64 MB, application/zip)
2018-09-11 15:22 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.30 MB, application/zip)
2018-09-11 15:51 PDT, EWS Watchlist
no flags Details
Patch (194.08 KB, patch)
2018-09-12 08:33 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (194.08 KB, patch)
2018-09-21 08:14 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (194.26 KB, patch)
2018-10-05 09:16 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (194.19 KB, patch)
2018-10-09 11:27 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2018-08-17 16:09:52 PDT
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.
Comment 1 Simon Fraser (smfr) 2018-08-20 08:21:31 PDT
Can you attach an actual test case please?
Comment 2 Oriol Brufau 2018-08-20 10:37:54 PDT
Created attachment 347507 [details]
testcase
Comment 3 Oriol Brufau 2018-09-10 13:29:43 PDT Comment hidden (obsolete)
Comment 4 Oriol Brufau 2018-09-10 14:56:39 PDT Comment hidden (obsolete)
Comment 5 Oriol Brufau 2018-09-10 16:14:08 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-09-10 18:51:36 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-09-10 18:51:37 PDT Comment hidden (obsolete)
Comment 8 Manuel Rego Casasnovas 2018-09-11 03:26:45 PDT
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?
Comment 9 Oriol Brufau 2018-09-11 05:15:44 PDT
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)
Comment 10 Oriol Brufau 2018-09-11 13:27:43 PDT Comment hidden (obsolete)
Comment 11 Oriol Brufau 2018-09-11 13:37:58 PDT
(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
Comment 12 EWS Watchlist 2018-09-11 15:16:30 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-09-11 15:16:32 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-09-11 15:17:10 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-09-11 15:17:12 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-09-11 15:22:41 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-09-11 15:22:43 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-09-11 15:51:16 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-09-11 15:51:18 PDT Comment hidden (obsolete)
Comment 20 Oriol Brufau 2018-09-12 08:33:56 PDT Comment hidden (obsolete)
Comment 21 Oriol Brufau 2018-09-12 08:41:38 PDT
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.
Comment 22 Oriol Brufau 2018-09-21 08:14:40 PDT Comment hidden (obsolete)
Comment 23 Oriol Brufau 2018-09-21 08:15:43 PDT
Updated the WPT with changes from https://github.com/web-platform-tests/wpt/pull/13131
Comment 24 Manuel Rego Casasnovas 2018-10-03 03:52:47 PDT
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?
Comment 25 Oriol Brufau 2018-10-03 04:26:56 PDT
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.
Comment 26 Manuel Rego Casasnovas 2018-10-03 05:48:29 PDT
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.
Comment 27 Oriol Brufau 2018-10-05 09:16:18 PDT Comment hidden (obsolete)
Comment 28 Oriol Brufau 2018-10-05 09:24:37 PDT
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.
Comment 29 Manuel Rego Casasnovas 2018-10-08 02:15:12 PDT
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.
Comment 30 Oriol Brufau 2018-10-09 11:27:32 PDT
Created attachment 351898 [details]
Patch
Comment 31 Oriol Brufau 2018-10-09 11:39:42 PDT
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
Comment 32 Manuel Rego Casasnovas 2018-10-09 12:42:50 PDT
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).
Comment 33 WebKit Commit Bot 2018-10-09 13:11:11 PDT
Comment on attachment 351898 [details]
Patch

Clearing flags on attachment: 351898

Committed r236979: <https://trac.webkit.org/changeset/236979>
Comment 34 WebKit Commit Bot 2018-10-09 13:11:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2018-10-09 13:12:45 PDT
<rdar://problem/45137051>