1374997 - Print Preview doesn't return to the previewed tab after closing.

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Bob Owen (:bobowen)]

[Tracking Requested - why for this release]: Regressed in Fx55.

I noticed this while testing for bug 1365601.
After bisection this was the regression range, which pointed to bug 1332386:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=084bebaffd26e5a0e3aa71b0751b95bce6ac356f&tochange=d338de5e37ecb849f80a27b101a53cce917e6bd6

I have a patch.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Make sure changing the tab is allowed, before we switch back after print preview

Comment 2

[:Gijs (he/him)]

Comment on attachment 8879920 [details] [diff] [review]
Make sure changing the tab is allowed, before we switch back after print preview

Review of attachment 8879920 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing, r=me

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Add check to ensure we switch back to original tab after print preview

I realised I could use this test to add a quick check for this.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8879953 [details] [diff] [review]
Part 2: Add check to ensure we switch back to original tab after print preview

Review of attachment 8879953 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js
@@ +2,5 @@
>  const kURL2 = "data:text/html,I shouldn't be here!";
>  
>  /**
>   * Verify that if we open a new tab and try to make it the selected tab while
>   * print preview is up, that doesn't happen.

Nit: please update this comment.

@@ +18,5 @@
>      isnot(gBrowser.selectedTab, tab, "Selected tab should still not be the tab we added");
>      is(gBrowser.selectedTab, PrintPreviewListener._printPreviewTab, "Selected tab should still be the print preview tab");
>      PrintUtils.exitPrintPreview();
>      await BrowserTestUtils.waitForCondition(() => !gInPrintPreviewMode, "should be in print preview mode");
> +    is(gBrowser.selectedTab, originalTab, "Selected tab should be back to the original tab that we print previewed");

I wonder if this should wait for the tab switch to be complete... I guess it doesn't really matter? If it works like this on infra, r=me. Alternatively, use:

let tabSwitched = BrowserTestUtils.switchTab(gBrowser, () => {});

before calling exitPrintPreview(), and then

await tabSwitched

before checking the selected tab.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e313f8c77c
Part 1: Make sure changing the tab is allowed, before we switch back after print preview. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/8724211bb0bb
Part 2: Add check to ensure we switch back to original tab after print preview. r=Gijs

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/68e313f8c77c
https://hg.mozilla.org/mozilla-central/rev/8724211bb0bb

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8879920 [details] [diff] [review]
Make sure changing the tab is allowed, before we switch back after print preview

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1332386 

[User impact if declined]:
On exiting from Print Preview the incorrect tab will be shown.

[Is this code covered by automated tests?]:
Yes, new test for this bug.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
I don't think it desperately needs it, but it's pretty quick and simple to test.

[List of other uplifts needed for the feature/fix]:
Other patch in this bug.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Very simple change to make sure the existing attempt to select the correct tab works.

[String changes made/needed]:
No

Comment 9

[Bob Owen (:bobowen)]

Comment on attachment 8879953 [details] [diff] [review]
Part 2: Add check to ensure we switch back to original tab after print preview

See comment 8.
(Test that landed was an improved version.)

Comment 10

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

Comment on attachment 8879920 [details] [diff] [review]
Make sure changing the tab is allowed, before we switch back after print preview

Fix for regression in 55, includes a test, let's bring it to beta 5.

Comment 11

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

Comment on attachment 8879953 [details] [diff] [review]
Part 2: Add check to ensure we switch back to original tab after print preview

Tests. Yay!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a2de62b17ca8
https://hg.mozilla.org/releases/mozilla-beta/rev/967c6d81513d