Closed Bug 1381083 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jryans, Assigned: emilio)

References

Details

Attachments

(5 files)

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.
Attached patch Example reftestSplinter Review
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 %.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Bug 1382288 comment 1 might be related here.
See Also: → 1382288
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 :)
Assignee: jryans → emilio+bugs
Pretty sure this is a regression since this was introduced.
Blocks: 505515
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?
Flags: needinfo?(cam)
(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.
Flags: needinfo?(cam)
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
Attachment #8890584 - Flags: review?(cam) → review+
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`.
Attachment #8890585 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
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
Blocks: 1382288
See Also: 1382288
You need to log in before you can comment on or make changes to this bug.