1381083 - Stylo: Percentage based position values reported in different units over time

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

DevTools test "devtools/client/inspector/computed/test/browser_computed_pseudo-element_01.js" is failing because getComputedStyle for pseudo-elements in Stylo appears to flip flop between returning `top` in % vs. px.

I'll attach a reduced reftest for this case.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Example reftest

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It appears that when nsComputedDOMStyle fully resolves the style, we get the expected px value.  However, if it uses the cached style context, we can get % value instead.

I'll try to trace down why the cached value changes to %.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 1382288 comment 1 might be related here.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case that also fails in Gecko.

So it indeed is because of that line I mentioned, but the bug is also present in Gecko, as can be shown on the following test-case, it's just that stylo is more conservative re-resolving pseudo-element styles.

After talking with jryans I'll take this bug :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Pretty sure this is a regression since this was introduced.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1381083: Preliminary alignment and whitespace fixup.

Review commit: https://reviewboard.mozilla.org/r/161736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161736/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1381083: Don't hold the style context if we had a frame and re-resolved the style.

Otherwise we risk losing track of the frame if we hit the cache.

Review commit: https://reviewboard.mozilla.org/r/161738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161738/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1381083: Test.

Review commit: https://reviewboard.mozilla.org/r/161740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161740/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I wonder why do we need to clear the current style sources after each GetPropertyValue call? Shouldn't the restyle generation check take care of that?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> I wonder why do we need to clear the current style sources after each
> GetPropertyValue call? Shouldn't the restyle generation check take care of
> that?

The question is nonsense, frames can go away with DOM mutations apart from restyles, so you really don't want a stale pointer there.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8890584 [details]
Bug 1381083: Preliminary alignment and whitespace fixup.

https://reviewboard.mozilla.org/r/161736/#review167230

::: commit-message-e3375:1
(Diff revision 1)
> +Bug 1381083: Preliminar alignment, whitespace fixup. r?heycam

Preliminary

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8890585 [details]
Bug 1381083: Don't hold the style context if we had a frame and re-resolved the style.

https://reviewboard.mozilla.org/r/161738/#review167674

Nice find!

::: layout/style/nsComputedDOMStyle.cpp:970
(Diff revision 1)
>    // For a style context we resolved, keep it around so that we
>    // can re-use it next time this object is queried.

Can you update the comment here to point out that we need to clear the style context if it was resolved due to being inside a pseudo?

::: layout/style/nsComputedDOMStyle.cpp:972
(Diff revision 1)
> -  if (!mResolvedStyleContext) {
> -    mStyleContext = nullptr;
> +  if (!mResolvedStyleContext || hadFrame) {
> +    ClearStyleContext();
>    }

Feel free to just move this to the top of the function, so we don't need to record `hadFrame`.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8890586 [details]
Bug 1381083: Test.

https://reviewboard.mozilla.org/r/161740/#review167678

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890584 [details]
Bug 1381083: Preliminary alignment and whitespace fixup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161736/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890585 [details]
Bug 1381083: Don't hold the style context if we had a frame and re-resolved the style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161738/diff/1-2/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890586 [details]
Bug 1381083: Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161740/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc839a86967d
Preliminary alignment and whitespace fixup. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b8755e706ea7
Don't hold the style context if we had a frame and re-resolved the style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a20bd8c1a22d
Test. r=heycam

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc839a86967d
https://hg.mozilla.org/mozilla-central/rev/b8755e706ea7
https://hg.mozilla.org/mozilla-central/rev/a20bd8c1a22d