1306266 - Write automated test for tab-switching using window.open (or external links?) while print preview opens up (bug 1301876)

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[:Gijs (he/him)]

See bug 1301876 for context. We should have an automated test for this case.

Comment 1

[:Gijs (he/him)]

Attachment: Test v0.1

This works as-is against current trunk. I checked that either removing the extra selectedTab = '...' in the print preview code or removing the early return from the "selectedTab" getter in tabbrowser.xml regresses one of the 2 sets of double assertions here.

Comment 2

[:Gijs (he/him)]

Attachment: Additional issues I had with print preview

However... I noticed that the 2nd page still ends up being the thing that's print previewed when I ran the test on my mac (just insert yield new Promise(r => setTimeout(r, 10000)); before exiting print preview). That seems wrong.

getSourceBrowser reads _tabBeforePrintPreview and otherwise just returns gBrowser.selectedBrowser. I noticed it is being called a second time, async, after print preview starts, which produces the bogus result.

I think the callsites in onEnter should just be relying on this._sourceBrowser directly instead of calling this._listener.getSourceBrowser() again.

As I was looking around for _sourceBrowser, I also noticed it gets overwritten with this._listener.getPrintPreviewBrowser(), which is not the source browser but the browser in which we're showing the print preview, which also seemed wrong. So here's a separate patch that fixes both of these issues. Mike, while you're paging all this in, can you doublecheck this is sane? If so, I'll file an orthogonal non-sec-sensitive bug where we can get this reviewed & landed.

Comment 3

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

Comment on attachment 8798385 [details] [diff] [review]
Test v0.1

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

Thanks for the test!

::: browser/base/content/test/tabs/browser_tabSwitchPrintPreview.js
@@ +6,5 @@
> + * print preview is up, that doesn't happen.
> + */
> +add_task(function* () {
> +  yield BrowserTestUtils.withNewTab(kURL1, function* (browser) {
> +    let originalTab = gBrowser.getBrowserForTab(browser);

This is unused - and probably broken (browser is already a <xul:browser>, and getBrowserForTab takes a <xul:tab>)

@@ +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();
> +    yield BrowserTestUtils.waitForCondition(() => !gInPrintPreviewMode, "should be in print preview mode");
> +    gBrowser.removeTab(tab);

yield BrowserTestUtils.removeTab(tab);

Comment 4

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

Comment on attachment 8798387 [details] [diff] [review]
Additional issues I had with print preview

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

Yeah, this is sane.

::: toolkit/components/printing/content/printUtils.js
@@ +204,5 @@
>      } else {
>        // collapse the browser here -- it will be shown in
>        // enterPrintPreview; this forces a reflow which fixes display
>        // issues in bug 267422.
> +      let ppBrowser = this._listener.getPrintPreviewBrowser();

Shouldn't this._sourceBrowser always exist in this case? If so, can't we just use that instead of re-getting it?

Comment 5

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38485e7430ba

(In reply to Mike Conley (:mconley) - (review / needinfo overload) from comment #4)
> Comment on attachment 8798387 [details] [diff] [review]
> Additional issues I had with print preview
> 
> Review of attachment 8798387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, this is sane.
> 
> ::: toolkit/components/printing/content/printUtils.js
> @@ +204,5 @@
> >      } else {
> >        // collapse the browser here -- it will be shown in
> >        // enterPrintPreview; this forces a reflow which fixes display
> >        // issues in bug 267422.
> > +      let ppBrowser = this._listener.getPrintPreviewBrowser();
> 
> Shouldn't this._sourceBrowser always exist in this case? If so, can't we
> just use that instead of re-getting it?

My understanding is that we want to reflow the print preview browser that looks like it has a piece of paper in it - the print preview browser - by collapsing it here. It gets uncollapsed elsewhere in onEnter (IIRC). The source browser has the original page, not the print preview browser. So using _sourceBrowser would collapse the wrong browser. This code just shouldn't be assigning to _sourceBrowser - it's overwriting the value with the wrong thing. This is presumably why the other getters were using getSourceBrowser() - without this change, if they depend on _sourceBrowser being correct, they'd be in for a nasty surprise.

Comment 6

[:Gijs (he/him)]

Florian, there's some background here for bug 1308621 (mconley is not accepting r? requests right now).

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38485e7430ba