1367797 - Write reflow tests for tab opening and closing when the tab strip is overflowed

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Similar to bug 1363505, but blocked until we can make the tab strip cause reflows more deterministically (instead of via rAF).

Specific cases to be tested:
* Tab opening (tab strip switches to overflow)
* Tab opening (in overflow state)
* Tab closing (in overflow state)
* Tab closing (tab strip underflows)
* Tab switching (selected tab is scrolled out of view)

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Finally coming back around to this.

What we've already got:

* Tab opening (tab strip switches to overflow)
* Tab closing (tab strip underflows)

What we still need:

* Tab opening (in overflow state)
* Tab closing (in overflow state)
* Tab switching (selected tab is scrolled out of view)

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1367797 - Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed.

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

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8930620 [details]
Bug 1367797 - Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed.

https://reviewboard.mozilla.org/r/201380/#review210210

Looks good, thanks.

I wonder if there would be any point in testing tab closing without animation.

Also, would it be useful to test closing several tabs at once? ie. should we put a reflow observer around the removeAllButFirstTab call? And if so should we do it both in an overflow and a non-overflow case?

Should we check that no reflow happens when closing a window? (a window with one tab / a window with multiple tabs)

(just giving food for thoughts here, none of these new cases is required for landing)

::: browser/base/content/test/performance/browser_tabstrip_overflow_underflow_reflows.js:57
(Diff revision 1)
>  
> -  await withReflowObserver(async function() {
> +  Assert.ok(gBrowser.tabContainer.hasAttribute("overflow"),
> +            "Tabs should now be overflowed.");
> +
> +  // Now test that opening and closing a tab while overflowed doesn't cause
> +  // us to overflow.

s/overflow/reflow/

::: browser/base/content/test/performance/browser_tabstrip_overflow_underflow_reflows.js:88
(Diff revision 1)
> +            "First tab should be scrolled out of view.");
> +
> +  // Now switch to the first tab. We shouldn't flush layout at all.
> +  await withReflowObserver(async function(dirtyFrame) {
> +    let switchDone = BrowserTestUtils.waitForEvent(window, "TabSwitchDone");
> +    await BrowserTestUtils.switchTab(gBrowser, firstTab);

BrowserTestUtils.switchTab returns a promise that waits for the TabSwitchDone event already.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8930620 [details]
Bug 1367797 - Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfa04fd0f319
Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed. r=florian

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8930620 [details]
Bug 1367797 - Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed.

https://reviewboard.mozilla.org/r/201380/#review210210

Good ideas! I've filed bug 1424377 for them.

Comment 7

[Dorel Luca [:dluca]]

Backed out changeset cfa04fd0f319 (bug 1367797) for causing browser-chrome failures on browser_tabstrip_overflow_underflow_reflows.js r=backout on a CLOSED TREE

https://treeherder.mozilla.org/logviewer.html#?job_id=150914547&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/7d3a7c5c694253c8d0382f368067b2c26a6ec192

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cfa04fd0f3198bdefc3dbd3293756e0f1e101b96

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8930620 [details]
Bug 1367797 - Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed.

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

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Made some minor adjustments to make the test more resilient across different display sizes.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1389acf296a
Add reflow tests for adding, removing and switching tabs while the tab strip is overflowed. r=florian

Comment 11

[Tiberius Oros[:tiberius_oros]]

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