1384065 - Stylo: vk.com lightbox gallery is broken

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Hinatori]

Attachment: screenshots.zip

vk.com lightbox gallery is broken when layout.css.servo.enabled is set to true.
Tested on windows 8.1 x64 with firefox nightly 56.0 2017-07-24 x64.

Step to reproduce: 
1) Open https://vk.com/durov?z=album1_136592355
2) Click on any picture.

I attached two screenshots, one with servo.enabled set to false and another one with servo.enabled set to true.

Comment 1

[Chris Peterson [:cpeterson]]

Thanks for reporting this bug, Hinatori. The screenshots are helpful!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Ugh, this is going to be tricky to hunt down, they set the margin-left property to a high value, but how they compute it is not clear to me (it's in a minified script call photoview.js, and it's huge)...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ok, so I looked a bit more into this, and the reason is that the size computation for the container differs when Stylo is enabled or not. Still I haven't dug into what that function does, but typing:

  getSize(cur.pvCont)

when a photo is open, returns [ 1194, 631 ] without stylo, and [ 1194, 0 ] with stylo.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Which ends up boiling down to what offsetHeight returns.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

And that's different because there's a .clear_fix element that we end up not generating a pseudo-element for, even though we should.

So, basically, to "fix" it you can type in your console:

cur.pvPhotoWrap.classList.remove('clear_fix')
cur.pvPhotoWrap.classList.add('clear_fix')
Photoview.updateVerticalPosition()

I'm trying to figure out why are we missing the pseudo-element.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Oh, nasty, I think I see it.

So (simplified from the unminified script):

    var o = Photoview.isPhotosList();
    var t = "display: none";
    var v = o ? t : "";

    layer.innerHTML = '<div class="clear_fix pv_photo_wrap" style="' + v + '">';

So I think I know why we're failing to do this, which is because of this piece of code:

    https://github.com/servo/servo/blob/9d21f9e/components/style/style_resolver.rs#L149

It doesn't make a lot of sense anyway, I think, but I'm pretty sure that's the reason, and that's incorrect on its own way anyway.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> It doesn't make a lot of sense anyway, I think, but I'm pretty sure that's
> the reason, and that's incorrect on its own way anyway.

(I mean the brokenness in this page doesn't make a lot of sense, since I'm pretty sure we shouldn't be poking at display: none pseudo-elements, but anyway, it's easy to make a test-case fail with getComputedStyle).

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Ouch, actually, of course it makes sense... When we toggle the style attribute, we don't do a full selector-match, so we don't get the new pseudos.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: testcase

Here's a test-case that passes with Gecko (and any other browser), but not with Stylo.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Don't skip computation of pseudo-elements of display: none elements.

We have optimizations to avoid doing selector-matching when the style attribute
changes, so, given you can toggle the display property and the pseudo-elements
will suddenly become effective, we can't really skip them.

Furthermore, we assume that if an element has an ElementStyles, they're
up-to-date and we can use them for getComputedStyle, so it's pretty easy to
prove that we do the wrong thing when calling getComputedStyle with a
pseudo-element on a display: none root.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1384065: Tests.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890135 [details]
Bug 1384065: Tests.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890134 [details]
style: Don't skip computation of pseudo-elements of display: none elements.

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890135 [details]
Bug 1384065: Tests.

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

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8890134 [details]
style: Don't skip computation of pseudo-elements of display: none elements.

https://reviewboard.mozilla.org/r/161216/#review166660

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8890135 [details]
Bug 1384065: Tests.

https://reviewboard.mozilla.org/r/161218/#review166664

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8890135 [details]
Bug 1384065: Tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161218/diff/3-4/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Servo bits @ https://github.com/servo/servo/pull/17867.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdca0b7a7058
Tests. r=heycam

Comment 20

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

This was also noticed on Reddit via DevTools:

https://www.reddit.com/r/rust/comments/6p9s24/psa_you_can_try_out_stylo_in_firefox_nightly_now/dkq8ggh/

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/fdca0b7a7058

Comment 22

[Darkspirit]

Can't reproduce anymore. Nightly 56 x64 20170727100240 @ Debian Testing, Stylo: true (enabled by user)