1362758 - Flexbox layout does not update properly after content changes

Status: RESOLVED WORKSFORME

(Core :: Layout: Flexbox, defect, P2)

Description

[f1041701]

1. go to (NSFW) https://meguca.org/a/2050972 ¹
2. wait for page to load
3. scroll to the end of the page
3. press alt+g (some posts should be hidden now)
4. press alt+g again

This leaves some flexbox items (<article> nodes) in an incorrectly rendered state.

Forcing a relayout, e.g. by running |for(let art of Array.from(document.querySelectorAll("article"))) {art.style.top = 0};| on the console fixes it until the layout is changed again.

I suspect this is the result of some recent flexbox optimizations under bug 1304473


¹ The thread will expire in a few days. Any long thread (>1000 posts) on that site should work.

Comment 1

[Timothy Nikkel (:tnikkel)]

If it's a regression you can use mozregression to find out what caused the problem  http://mozilla.github.io/mozregression/

Comment 2

[Daniel Holbert [:dholbert]]

I can reproduce.  This is indeed a regression, and it seems to be caused by bug 1209697. (a flex optimization, as suspected in comment 0)  So, new regression in Firefox 53.

I narrowed to this regression range, in which bug 1209697 is the only commit to mention "flex":
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d025ac534a6333a8170a59a95a8a3673d4028ee&tochange=bf15d4078c2a6db7df37ab466d28a1e075c9eb4d

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug

Here's a screenshot of what the bug looks like, BTW. (Text overflowing from comment boxes, and some text overlapping other text.)

(Screenshot is SFW, though the site has NSFW content as noted in comment 0.)

Comment 4

[Astley Chen (inactive)]

Per investigation in comment 2.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I can take a look, I guess :)

So what pressing alt + g does is calling a toggleGallery function that, as far as I can tell, only toggles a class in the root ("gallery"), which makes some articles to be hidden with a selector that looks like: 

    .gallery:root #threads article[id^="p"]:not(.media):not(.editing)

However, that's not enough to trigger the bug I think, per my experimentation with the console: doing from devtools:

    document.documentElement.classList.toggle("gallery")

Twice, doesn't produce the garbled content.

If the reporter knew off-hand about what is going on apart from toggling that class (if anything, this could just be an artifact of the devtools), that'd save me a bit of time :). Otherwise I'll keep digging today when I come back from class.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix for 53, likely to miss 54 as well. But if you land a patch and can verify the fix we could still uplift to 54, potentially.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

A reduced test-case would help... I guess a potential fix is just using the DOM generation id as key for the reflow input too as I had planned initially for bug 1209697, but understanding what's making us hit the cache incorrectly would help a lot.

I'm with exams these weeks, but I could prepare the easy fix, wdyt Daniel?

Comment 8

[Astley Chen (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
Thanks for working on this bug.

Comment 9

[f1041701]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> If the reporter knew off-hand about what is going on apart from toggling
> that class (if anything, this could just be an artifact of the devtools),
> that'd save me a bit of time :). Otherwise I'll keep digging today when I
> come back from class.

Toggling the class is all it should be doing.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (nsfw language) - click blank space twice and scroll to bottom

The original thread has disappeared, but here's a standalone (not reduced) testcase taken from a snapshot of another thread.

In this version, you can just click some blank space to toggle the "gallery" class.  STR: click blank space, wait for re-render, click again, wait for re-render, scroll to end & look for content that overlaps text.

Comment 11

[Daniel Holbert [:dholbert]]

(For the record, my attached testcase is based on a snapshot from https://meguca.org/a/2105712 [NSFW])

Comment 12

[Daniel Holbert [:dholbert]]

I suspect this is related to interruptible reflow (and specifically to a measurement from an interruptible reflow being mistakenly cached or reused or something).

My "testcase 1" isn't 100% reliable, but I can generally get it to repro the issue after a few atttempts at clicking twice + scrolling to end (no reloading needed BTW).  Specifically: the first part of the wide comment about "dig things up from history if needed" will often be covered up by the comment-box that follows it.

However: if I toggle the about:config pref "layout.interruptible-reflow.enabled" to false, then I can't (yet) make the bug reproduce.

(I'd thought we instantiated a nsPresContext::InterruptPreventer to avoid interruptible-reflow interrupts during flexbox reflow, but I'm not finding that now, so it might've been removed, or I might be misremembering.)

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 56 as well.
We could still uplift a fix to 55, but I don't think release management needs to track this.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I don't feel the need to track this for 55. If a fix is ready and deemed low risk, please nominate for uplift to Beta55.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Assuming this is still an issue but I am not sure. In any case, too late for a fix in 56.

Comment 16

[Daniel Holbert [:dholbert]]

(Yeah, it is indeed still an issue -- just reproduced with attached testcase in latest Nightly.)

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Discussed this with Emilio and he is not working on it. I will see if I can find a new owner.

Comment 18

[Daniel Holbert [:dholbert]]

I think this is fixed -- possibly by bug 1495169 (which emilio just landed a fix for), or possibly by something earlier.

The attached testcase is a little tricky because if you interact with it too much, it makes the layout explode and take forever, which I'll spin off another bug for (though it might already be covered somewhere).  So I used these STR, which reliably trigger the bug in older builds without exploding the layout:

 1. Start with a blank tab.
 2. Visit https://bugzilla.mozilla.org/attachment.cgi?id=8873229
 3. As soon as you see some page content appear, press the "End" key on your keyboard.
 4. Wait for the page to finish loading (as reported by the throbber in the tab)
 5. Press "End" key again.

In old builds, e.g. Nightly 2017-09-26, I reliably see overlapping text/posts when I perform those STR.

In today's Nightly (2018-10-05), I do not see overlapping text/posts when I perform those STR.  Nor do I see any overlap in yesterday's Nightly (2018-10-04) -- hence, something else may have helped here before bug 1495169 was merged last night.  So, resolving as WORKSFORME rather than a dupe, since I'm not sure precisely when this became fixed.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> The attached testcase is a little tricky because if you interact with it too
> much, it makes the layout explode and take forever, which I'll spin off
> another bug for

BTW: I filed bug 1496817 for this part.