1576946 - Implement <stack> using CSS Grid and remove nsStackFrame

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Brian Grinstead [:bgrins]]

This seems similar to nsDeckFrame (Bug 1559192) except a stack shows all children on top of each other. Stacks could potentially be easier since we wouldn't have to worry about the height of the node changing as the visible child changes (https://phabricator.services.mozilla.com/D35467#1084549), since they are always visible.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1576946 - Implement <stack> using CSS Grid.

Comment 2

[Brian Grinstead [:bgrins]]

There's also -moz-stack-sizing which should be removable: https://searchfox.org/mozilla-central/search?q=moz-stack-sizing&path=

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1576946 - Rewrite and remove tests relying on `display: -moz-stack;`.

Depends on D46226

Comment 4

[Tim Nguyen :ntim]

Attachment: Bug 1576946 - Remove nsStackFrame platform code.

Depends on D49485

Comment 5

[Tim Nguyen :ntim]

Artifact without last part: https://treeherder.mozilla.org/#/jobs?repo=try&revision=751a42cb569384ebdd9efb1040000018b46b84d4
Non artifact with last part: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3891297ea2d175b1b8147446b00f322215746edb

Comment 6

[Tim Nguyen :ntim]

Talos without last part: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=24ef95f3b2f72caea780243a9b0cfea03bce191d&framework=1&showOnlyImportant=1

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #6)

Talos without last part: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=24ef95f3b2f72caea780243a9b0cfea03bce191d&framework=1&showOnlyImportant=1

FYI that is comparing an artifact push (new) with a non-artifact push (mozilla-central over last two days). I haven't gotten reliable results doing that in the past. In general I do: ./mach try fuzzy --preset perf-chrome both with any without changes applied to get a more direct comparison (artifact is fine, as long as they both are).

Comment 8

[Tim Nguyen :ntim]

Non-artifact: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb225f5837af2af07feac3ce0fd2dde33620ca9d
Artifact: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51bdf4dec43aadb856226a0ccba6ace71a7cf219

Comment 9

[Mats Palmgren (inactive)]

(In reply to Tim Nguyen :ntim from comment #8)

Artifact: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51bdf4dec43aadb856226a0ccba6ace71a7cf219

The browser_urlbar_keyed_search.js failures are probably worth looking in to?

Comment 10

[Tim Nguyen :ntim]

(In reply to Mats Palmgren (:mats) from comment #9)

(In reply to Tim Nguyen :ntim from comment #8)

Artifact: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51bdf4dec43aadb856226a0ccba6ace71a7cf219

The browser_urlbar_keyed_search.js failures are probably worth looking in to?

Mats, do you have any idea how to fix the flicker that happens when a tab is opening ? I believe this is caused by the "Implement <stack> using CSS Grid." patch, but I'm not sure how to fix it. Fixing the flicker would fix the browser_urlbar_keyed_search.js and browser_windowopen.js perf tests.

For reference, the tab markup uses <stack> and the CSS for stack is now:

stack {
  display: grid;
  position: relative;
}

stack > *|* {
  grid-area: 1 / 1;
  z-index: 0;
}

Comment 11

[Tim Nguyen :ntim]

Attachment: Flicker start

Comment 12

[Tim Nguyen :ntim]

Attachment: Flicker end

Comment 13

[Mats Palmgren (inactive)]

I have no idea what that might come from. Would it possible to make a standalone testcase?

Comment 14

[Tim Nguyen :ntim]

(In reply to Mats Palmgren (:mats) from comment #13)

I have no idea what that might come from. Would it possible to make a standalone testcase?

Those flickers were only detected by the tests I mentioned above, it’s very hard/impossible to notice when starting Firefox manually.

The screenshots are generated by the test as well: if it detects a flicker, the test will generate 2 data URIs containing the screenshots.

That unfortunately means it’s quite tricky to create a standalone test case here, since things are very tied to the test.

You can run one of the two tests locally (browser_urlbar_keyed_search.js on Windows/Linux or browser_windowopen.js on MacOS) with the first patch applied.

Comment 15

[Tim Nguyen :ntim]

Hi Kohei, why was the site-compat keyword added here ? None of the features removed here are web-exposed.

Comment 16

[Kohei Yoshino]

Sorry, was misreading the intent post.

Comment 17

[Mats Palmgren (inactive)]

I debugged the browser_urlbar_keyed_search.js failure under rr on Linux and I think I understand why that issue occurs now.
The problem is that GetXULMinSize/GetXULPrefSize etc provides the frame's intrinsic size also in the block-axis, which for non-XUL boxes means that we'll have to reflow it to find out:
https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/layout/generic/nsFrame.cpp#10144-10145
This changes the size of the frame to be its intrinsic size. Normally, there is a XULLayout call after that which resizes it again but it appears that's not guaranteed. In this case, we get these calls:
https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/layout/base/PresShell.cpp#4185
viewManager->UpdateWidgetGeometry()
presShell->SyncWindowProperties()
nsContainerFrame::SyncWindowProperties, which calls GetXULMinSize here:
https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/layout/generic/nsContainerFrame.cpp#617
with no XULLayout after that and then we paint it at that size.

Comment 18

[Mats Palmgren (inactive)]

Attachment: workaround wip

I'm not sure how to fix the issue mentioned above yet, but one way to workaround it is to set a definite height and make nsFrame::RefreshSizeCache skip the reflow in that case.
Seems to work, except maybe for the browser_toolbox_toolbar_overflow.js failures?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8adcda635cdf142503213892d654a4ad6c4106&selectedJob=273207256

Comment 19

[Tim Nguyen :ntim]

After discussing this a bit more with dholbert, the flicker seems caused by RefreshSizeCache (which is what comment 18 suggests too), which is only called for documents with XUL root elements. I confirmed locally that applying patches from bug 1492582 along with the ones here fixes the flicker.

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #19)

After discussing this a bit more with dholbert, the flicker seems caused by RefreshSizeCache (which is what comment 18 suggests too), which is only called for documents with XUL root elements. I confirmed locally that applying patches from bug 1492582 along with the ones here fixes the flicker.

Nice! So is this green with the html root node in browser.xhtml?

Comment 21

[Brian Grinstead [:bgrins]]

Tim, is this green now that Bug 1492582 has stuck?

Comment 22

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8019c98635058192f94d4adf9dfb8aae9296eafc

Comment 23

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #22)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8019c98635058192f94d4adf9dfb8aae9296eafc

I've fixed the toolkit/content/tests/browser/browser_label_textlink.js failures.

However, I still need to fix the reftest failures that happen only on particular platforms. Everything else looks green though.

Comment 24

[Tim Nguyen :ntim]

I fixed the reftest failures locally with mstange's help, let's see what try says:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a11c1f4024dcea83bbc94f7c252433536e13cd

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7477b284a8a3
Implement <stack> using CSS Grid. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/68cc349414ca
Rewrite and remove tests relying on `display: -moz-stack;`. r=mats
https://hg.mozilla.org/integration/autoland/rev/1a85c571a464
Remove nsStackFrame platform code. r=mats

Comment 26

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7477b284a8a3
https://hg.mozilla.org/mozilla-central/rev/68cc349414ca
https://hg.mozilla.org/mozilla-central/rev/1a85c571a464

Comment 27

[:Gijs (he/him)]

Are we still in a position where we can back this out of beta given that there's several open regressions here?

Comment 28

[Tim Nguyen :ntim]

(In reply to :Gijs (he/him) from comment #27)

Are we still in a position where we can back this out of beta given that there's several open regressions here?

It would require backing out a lot of bugs, but yes:

This bug, bug 1596966, bug 1598034, bug 1597844, bug 1596176 and bug 1596047.

Comment 29

[Tim Nguyen :ntim]

Alternatively, another possible route is to just back out https://hg.mozilla.org/mozilla-central/rev/1a85c571a464 and use display: -moz-stack on the usages that are currently broken.

Comment 30

[Julien Cristau [:jcristau]]

(In reply to Tim Nguyen :ntim from comment #29)

Alternatively, another possible route is to just back out https://hg.mozilla.org/mozilla-central/rev/1a85c571a464 and use display: -moz-stack on the usages that are currently broken.

Is this something you can pursue, by say next week? (Presumably in a new bug?)

Comment 31

[Tim Nguyen :ntim]

(In reply to Julien Cristau [:jcristau] from comment #30)

(In reply to Tim Nguyen :ntim from comment #29)

Alternatively, another possible route is to just back out https://hg.mozilla.org/mozilla-central/rev/1a85c571a464 and use display: -moz-stack on the usages that are currently broken.

Is this something you can pursue, by say next week? (Presumably in a new bug?)

I'm already looking into it in bug 1599783.

Comment 32

[Julien Cristau [:jcristau]]

Awesome, thank you.

Comment 33

[Tim Nguyen :ntim]

The platform code removal was backed out from Firefox Beta 72 in bug 1599783: https://hg.mozilla.org/releases/mozilla-beta/rev/588538e4ad3d