Bug 188711

Can you attach an actual test case please?

Created attachment 347507 [details]
testcase

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 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)

(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

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.

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

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 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 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 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 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.

Created attachment 351898 [details]
Patch

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 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 on attachment 351898 [details]
Patch

Clearing flags on attachment: 351898

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

All reviewed patches have been landed.  Closing bug.

<rdar://problem/45137051>