Closed Bug 1384065 Opened 7 years ago Closed 7 years ago

Stylo: vk.com lightbox gallery is broken

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Hinatori, Assigned: emilio)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Summary: Stylo: → Stylo:vk.com lightbox gallery is broken
Priority: -- → P2
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Thanks for reporting this bug, Hinatori. The screenshots are helpful!
Summary: Stylo:vk.com lightbox gallery is broken → Stylo: vk.com lightbox gallery is broken
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)...
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.
Which ends up boiling down to what offsetHeight returns.
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.
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.
Assignee: nobody → emilio+bugs
(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).
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.
Attached file testcase
Here's a test-case that passes with Gecko (and any other browser), but not with Stylo.
Comment on attachment 8890134 [details]
style: Don't skip computation of pseudo-elements of display: none elements.

https://reviewboard.mozilla.org/r/161216/#review166660
Attachment #8890134 - Flags: review?(cam) → review+
Attachment #8890134 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/fdca0b7a7058
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1384602
Can't reproduce anymore. Nightly 56 x64 20170727100240 @ Debian Testing, Stylo: true (enabled by user)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.