Closed Bug 1329220 Opened 6 years ago Closed 6 years ago

The print preview document is live to changes or navigation by content script (it should be frozen at the time of the print request)

Categories

(Core :: Print Preview, defect)

Product:
Core
Component:
Print Preview
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 - wontfix
firefox51 + wontfix
firefox52 + fixed
firefox53 + fixed

People

(Reporter: smaug, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

alternative testcase showing problems when switching between landscape/portrate in print preview
1.32 KB, text/html
Details
Bug 1329220 - fix print preview source browser confusion,
59 bytes, text/x-review-board-request
mconley
: review+
jcristau
: approval-mozilla-aurora+
Details 
Testcase is

data:text/html,<script>window.onafterprint = function() { setTimeout("window.location = 'data:text/plain,REPLACED PAGE!'", 0); }</script><pre>INITIAL PAGE</pre>

Enter print preview and change portrait/landscape.

Based on comments in Bug 1308461 this is a regression from bug 962433.
Blocks: 962433
> Changing print preview settings may end up print preview different page

Or the existing document may be mutated by script while in print preview, resulting in the print output being different to the state of the page when the user initiated printing.
I think this might be a regression from my 'fix' in bug 1308621 instead, as it's referenced in the comments in bug 1308461. :-(

It's late here and I don't right now have time to look at this in more detail, but IIRC the sourceBrowser reference was also used for other things besides the actual fetching of stuff to print (like maybe what tab to (try to) exit to when exiting print preview, or something - not 100% sure anymore off-hand; there was a sec bug involved as well as I recall...), and so making the source browser refer to the print preview browser had other side effects. I expect we'll need to use 2 variables to track these two distinct things we need to run print preview effectively - which is unfortunate because that means we'll have 3 browser references to keep track of.

It also feels like the 'source' for the print preview should be updated to the print preview browser as soon as it's ready, rather than whenever we try to re-initialize print preview, as the latter adds raciness and creates somewhat difficult-to-track bugs. It'll be easier to guarantee code does the right thing if those references are the same regardless of whether the user changes settings after the initial print preview is complete.
[Tracking Requested - why for this release]:
This seems like a pretty bad regression.
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Summary: Changing print preview settings may end up print preview different page → The print preview document is live to changes or navigation by content script (it should be frozen at the time of the print request)
Tracking for 51 and up. We only have 2 weeks left to go for the 50 release version, so I think it is reasonable to wontfix there.

Gijs is this something you can take on? Or can you suggest someone else who might work on it? Thanks.
status-firefox50: affected → wontfix
tracking-firefox50: ? → -
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox53: ? → +
Flags: needinfo?(gijskruitbosch+bugs)
Jet, do you think this is important to fix for 51? Can we mark it fix-optional for 51 at this point?
So I was wrong in comment #2, and this regression *does* date back to bug 962433.
(In reply to :Gijs from comment #2)
> I think this might be a regression from my 'fix' in bug 1308621 instead, as
> it's referenced in the comments in bug 1308461. :-(
> 
> It's late here and I don't right now have time to look at this in more
> detail, but IIRC the sourceBrowser reference was also used for other things
> besides the actual fetching of stuff to print (like maybe what tab to (try
> to) exit to when exiting print preview, or something - not 100% sure anymore
> off-hand; there was a sec bug involved as well as I recall...), and so
> making the source browser refer to the print preview browser had other side
> effects. I expect we'll need to use 2 variables to track these two distinct
> things we need to run print preview effectively - which is unfortunate
> because that means we'll have 3 browser references to keep track of.
> 
> It also feels like the 'source' for the print preview should be updated to
> the print preview browser as soon as it's ready, rather than whenever we try
> to re-initialize print preview, as the latter adds raciness and creates
> somewhat difficult-to-track bugs. It'll be easier to guarantee code does the
> right thing if those references are the same regardless of whether the user
> changes settings after the initial print preview is complete.

Gijs, do you have any draft idea how we could come up with this solution? Would we clone our original browser somehow?
I have a patch that seems to work locally, but I'm working on an automated test, and jwatt will probably have to review it. :-)
(In reply to :Gijs from comment #9)
> I have a patch that seems to work locally, but I'm working on an automated
> test, and jwatt will probably have to review it. :-)

OK, so while the patch addresses the regression, it causes another issue. This is because we have two competing objectives:

1) we want to reuse the print preview browser to re-init print preview when settings change
2) when we tick the 'simplify' box, we will change the actual content in the print preview browser (specifically, to the readerized/simplified content).

This means that if after a settings change we re-init from the original tab, we get this bug. If we re-init from the print preview tab itself, we can never escape "simplified" mode if once the user activates it.

So, I see 4 options:

1) back out the simplified mode changes
2) fix the code to always use the print preview tab, and disable the simplified mode (there's a pref - but just toggling the pref won't fix this bug by itself)
3) fix the code and live with the bug with simplified mode because it's unlikely to be used that often. It's disabled a lot of the time, and even where it isn't, you'd have to switch to simplified mode and then go back to see the bug (you can't get back out of simplified mode), for which there is a relatively simple workaround: exit print preview, re-enter print preview.
4) fix the code and add another fix to make the simplified mode work.

I've been trying (4) but it basically needs a fourth browser. We already have:
1) the original browser/tab
2) the print preview one
3) the one we use to render the simplified content (which isn't a print preview tab, but an alternative 'source' tab)

and we'd need to add:
4) a print preview tab specific to 'simplified' mode

I have a patch that's partway there but it doesn't seem to work correctly with simplified mode, and I'm not sure why.
Ah, so the issue was with changing tabs while in print preview. I forgot I wrote that code, sigh. I have a working patch now, but the bit to keep simplified mode working seems pretty high risk to me. I'll put both on the bug and see what jwatt/mconley think.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs from comment #10)
> OK, so while the patch addresses the regression, it causes another issue.
> This is because we have two competing objectives:
> 
> 1) we want to reuse the print preview browser to re-init print preview when
> settings change
> 2) when we tick the 'simplify' box, we will change the actual content in the
> print preview browser (specifically, to the readerized/simplified content).
> 
> This means that if after a settings change we re-init from the original tab,
> we get this bug. If we re-init from the print preview tab itself, we can
> never escape "simplified" mode if once the user activates it.

I think that was the reason we ended up reusing the original browser to re-init print preview when settings change, so we could always get the correct content source. See here https://hg.mozilla.org/mozilla-central/rev/dc5692c66a02#l4.85. Unfortunately, the idea that preview was not working like a cloned-document didn't came to light while coding and reviewing (as bug 1308621 made sense to fix at that time). :(
(In reply to :Gijs from comment #13)
> Created attachment 8825210 [details]
> Bug 1329220 - use a fourth browser to deal with print previewing simplified
> mode,
> 
> Review commit: https://reviewboard.mozilla.org/r/103398/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/103398/

Awesome work, Gijs!
(In reply to :Gijs from comment #13)
> Created attachment 8825210 [details]
> Bug 1329220 - use a fourth browser to deal with print previewing simplified
> mode,
> 
> Review commit: https://reviewboard.mozilla.org/r/103398/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/103398/

One thing I noticed when testing was that we need to check if these tabs are not null prior to removing them (browser/content/browser.js:3401:9). Otherwise we get a type error on browser/content/tabbrowser.xml:2487:1.
(In reply to :Gijs from comment #10)
> I've been trying (4) but it basically needs a fourth browser. We already
> have:
> 1) the original browser/tab
> 2) the print preview one
> 3) the one we use to render the simplified content (which isn't a print
> preview tab, but an alternative 'source' tab)
> 
> and we'd need to add:
> 4) a print preview tab specific to 'simplified' mode

I assumed we would have 3 documents:

 1) the source document (the document being printed)
 2) the static clone
 3) the print preview document

The static clone would be a clone of the source document created when the user initiated the print and would never be modified after creation. The print preview document would be created from the static clone with appropriate modifications to take account of print settings such as margins, headers footers, etc. Any time the print preview document needed to be updated for changes to headers/footers, page orientation, margins, simplification/unsimplification, etc. we could either make those modifications or else use the static clone to regenerate a new print preview document.
(In reply to Jonathan Watt [:jwatt] from comment #17)
> (In reply to :Gijs from comment #10)
> > I've been trying (4) but it basically needs a fourth browser. We already
> > have:
> > 1) the original browser/tab
> > 2) the print preview one
> > 3) the one we use to render the simplified content (which isn't a print
> > preview tab, but an alternative 'source' tab)
> > 
> > and we'd need to add:
> > 4) a print preview tab specific to 'simplified' mode
> 
> I assumed we would have 3 documents:
> 
>  1) the source document (the document being printed)
>  2) the static clone
>  3) the print preview document
> 
> The static clone would be a clone of the source document created when the
> user initiated the print and would never be modified after creation. The
> print preview document would be created from the static clone with
> appropriate modifications to take account of print settings such as margins,
> headers footers, etc. Any time the print preview document needed to be
> updated for changes to headers/footers, page orientation, margins,
> simplification/unsimplification, etc. we could either make those
> modifications or else use the static clone to regenerate a new print preview
> document.

Simplification currently produces a bunch of HTML. It doesn't change a document. We also, as far as I know, don't normally change the print preview document, and I'm not sure how doing that would work. From a front-end perspective, we just let the print preview take some other docshell and produce a print preview version of what that docshell is displaying. We'd need to add this "static clone" version of the document, because right now we don't have one. AIUI we need the doc to be in print preview to prevent it running script etc. I'm not sure how to go about doing what you're suggesting.
Flags: needinfo?(jwatt)
Olli, how do you feel about leaving this bug unfixed for v51? The alternative is to take the first patch and disable reader mode, but even there we've only got 12 days to uplift. I'm inclined to thing that if this bug has been in v50 and nobody seems to have noticed then one more release is probably okay.
Flags: needinfo?(jwatt) → needinfo?(bugs)
(In reply to Jonathan Watt [:jwatt] from comment #19)
> Olli, how do you feel about leaving this bug unfixed for v51? The
> alternative is to take the first patch and disable reader mode, but even
> there we've only got 12 days to uplift. I'm inclined to thing that if this
> bug has been in v50 and nobody seems to have noticed then one more release
> is probably okay.

Nitpicking (sorry!), we wouldn't need to disable *all* of reader mode (in the browser UI etc.), but we'd have to disable the "simplify mode" option (ie checkbox) that we added in print preview.
Simplify page is disabled by default in toolkit. It is only enabled on Nightly for Linux, and for non-Linux, only enabled up to early beta. See here: http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#1527 (bug 1309980).
(In reply to Matheus Longaray (:mlongaray) from comment #21)
> Simplify page is disabled by default in toolkit. It is only enabled on
> Nightly for Linux, and for non-Linux, only enabled up to early beta. See
> here:
> http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.
> js#1527 (bug 1309980).

Oh! I wonder how I missed that. Sorry for the confusion. In that case, we could potentially fix this for 51 by only taking the first of the 2 patches I attached to this bug, which I believe is reasonably low risk (inasmuch as anything can be this late in beta). That patch includes an automated test for the issue this bug was filed about so we don't break it again. :-)
Based on the latest comments, sounds like fixing this in FF51 is still doable, even though perhaps a bit stretch. Given that bug 962433 shipped in 49, perhaps skipping yet another release isn't totally horrible.
Flags: needinfo?(bugs)
Comment on attachment 8825209 [details]
Bug 1329220 - fix print preview source browser confusion,

https://reviewboard.mozilla.org/r/103396/#review105756

Print Preview tests! \o/ Thanks Gijs, this makes a lot of sense. Sorry I didn't catch this when bug 1308621 was still going through review cycles.
Attachment #8825209 - Flags: review?(mconley) → review+
Attachment #8825209 - Flags: review?(jwatt)
Mike, should I just go ahead and land the first commit here, and we can separate out the "permanent 'simplify' mode" issue into a separate issue?
Flags: needinfo?(mconley)
(In reply to :Gijs from comment #25)
> Mike, should I just go ahead and land the first commit here, and we can
> separate out the "permanent 'simplify' mode" issue into a separate issue?

Yes, I think that's the right move. I'm still trying to find out if there's a better way to do all of this without adding yet another browser. :/
Flags: needinfo?(mconley)
Attachment #8825210 - Attachment is obsolete: true
Attachment #8825210 - Flags: review?(mconley)
Attachment #8825210 - Flags: review?(jwatt)
ni'ing myself on alternative to the 4th browser element.
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/0cfe2272eecb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
We missed the boat for 51 here, but will definitely take an Aurora approval request when ready :)
status-firefox51: affected → wontfix
Comment on attachment 8825209 [details]
Bug 1329220 - fix print preview source browser confusion,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1308621
[User impact if declined]: print previews change when changing settings
[Is this code covered by automated tests?]: it is as part of this patch! :-)
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: I think the automated test does a reasonable job, but if deemed necessary QE can verify with the steps in comment #0.
[List of other uplifts needed for the feature/fix]: Don't think there are any
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a relatively small change with an automated test
[String changes made/needed]: nope
Attachment #8825209 - Flags: approval-mozilla-aurora?
Depends on: 1332386
I've split up the regression in the behaviour of 'simplify page' into bug 1332386.
Flags: needinfo?(mconley)
Comment on attachment 8825209 [details]
Bug 1329220 - fix print preview source browser confusion,

print preview fix for aurora52
Attachment #8825209 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.