Closed
Bug 299555
Opened 19 years ago
Closed 17 years ago
F5 reloads Print Preview after right clicking several times on this specific testcase
Categories
(Core :: Print Preview, defect)
Core
Print Preview
Tracking
()
VERIFIED
FIXED
People
(Reporter: bugzilla, Assigned: martijn.martijn)
References
Details
(Keywords: testcase)
Attachments
(3 files, 6 obsolete files)
75 bytes,
text/html
|
Details | |
2.93 KB,
patch
|
neil
:
review+
neil
:
superreview+
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
bzbarsky
:
review+
jst
:
superreview+
jst
:
approval1.9+
|
Details | Diff | Splinter Review |
Build id: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050703 Firefox/1.0+
Now that bug 126719 is worksforme I noticed that F5 still works, but in a very
special case
Steps to reproduce:
1. Visit testcase
2. Go to Print Preview
3. Press F5 or Ctrl + r, nothing will h appen
4. Right click several time on the page
5. Press F5 and notice it will reload and mess up PP
Reporter | ||
Updated•19 years ago
|
Reporter | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
Just focusing something in the chrome area (input or select or button) and then
pressing F5 allways shows the bug to me.
Alo Esc, F1, and F11 work in print preview.
Assignee | ||
Comment 3•19 years ago
|
||
This fixes the F5 reload bug, but I would have expected that reloading a
document would follow this code path:
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3079
The Esc + Ctrl-W bug might have something to do with the code here:
http://lxr.mozilla.org/seamonkey/source/toolkit/components/printing/content/printUtils.js#261
but I'm not really sure about that.
Assignee | ||
Comment 4•19 years ago
|
||
No, wait, I get an error message in the js console with the escape key:
* Call to xpconnect wrapped JSObject produced this error: *
[Exception... "'[JavaScript Error: "navigate is not defined" {file: "chrome://br
owser/content/browser.xul" line: 1}]' when calling method: [nsIDOMEventListener:
:handleEvent]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAIL
S)" location: "<unknown>" data: yes]
Assignee | ||
Comment 5•19 years ago
|
||
Ok, it happens because of the onblur="navigate" code.
I can't just remove it, because it would regress bug 126726.
But I can solve it this way.
I think the F1 key should just be allowed to work. Maybe the F11 key should be
disabled, but it doesn't really do that much harm, afaik.
Assignee | ||
Updated•19 years ago
|
Attachment #188711 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 6•19 years ago
|
||
Comment on attachment 188711 [details] [diff] [review]
patch for escape
Try changing the onblur to onchange instead?
Assignee | ||
Comment 7•19 years ago
|
||
Yes, that's a much better idea and it works. Thanks. Uhm, but I can't really
say I made this patch.
Attachment #188711 -
Attachment is obsolete: true
Attachment #188860 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•19 years ago
|
Attachment #188711 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 8•19 years ago
|
||
Comment on attachment 188698 [details] [diff] [review]
patch
Maybe you can also review this?
Attachment #188698 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 9•19 years ago
|
||
Comment on attachment 188860 [details] [diff] [review]
patch2 for escape
>- onblur="navigate(0, this.value, 0);"
>+ onchange="navigate(0, this.value, 0);"
> onkeypress="if (event.keyCode==13) navigate(0, this.value, 0);"/>
Well, I had a look at this, and it would seem that the old code (for bug
126726) hasn't worked since the checkin for bug 147058. But looking at the code
more closely (sorry for overlooking it last time) I'd like to point out that
onchange does in fact fire for DOM_VK_RETURN keypresses as well, thus you
should find that the fix also makes the onkeypress attribute redundant.
Assignee | ||
Comment 10•19 years ago
|
||
You're right, again.
(I thought I checked if I could remove this, oh well..)
Attachment #188698 -
Attachment is obsolete: true
Attachment #188860 -
Attachment is obsolete: true
Attachment #188914 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•19 years ago
|
Attachment #188914 -
Flags: superreview+
Attachment #188914 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #188914 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #188914 -
Flags: approval1.8b4?
Assignee | ||
Comment 11•19 years ago
|
||
Comment on attachment 188698 [details] [diff] [review]
patch
Er, I accidently made this obsolete. Neil, this patch is for fixing the F5
reload issue. I hope you can review this too.
Attachment #188698 -
Attachment is obsolete: false
Updated•19 years ago
|
Attachment #188914 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Updated•19 years ago
|
Attachment #188860 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 12•19 years ago
|
||
Neil, you might not have noticed the other patch, that's why I'm cc-ing you.
Could you review that also? Thanks!
Comment 13•19 years ago
|
||
Comment on attachment 188698 [details] [diff] [review]
patch
Sorry, I don't know enough about the doc shell to review this.
Attachment #188698 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•19 years ago
|
Attachment #188698 -
Flags: review?(bzbarsky)
Comment 14•19 years ago
|
||
Comment on attachment 188698 [details] [diff] [review]
patch
Don't we need to also check whether some ancestor is printing/pp?
Comment 15•19 years ago
|
||
And in any case, if we're printing don't we want to handle the load once
printing is done?
Assignee | ||
Comment 16•19 years ago
|
||
(In reply to comment #14)
> Don't we need to also check whether some ancestor is printing/pp?
I I understand it correctly, all the docShells are set to mIsPrinting:
http://lxr.mozilla.org/seamonkey/source/layout/base/nsDocumentViewer.cpp#3773
(In reply to comment #15)
> And in any case, if we're printing don't we want to handle the load once
> printing is done?
Well, I just copied this code from:
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3080
But you mean you want the load to happen after printing/print preview has done?
Wouldn't that be very odd?
Comment 17•19 years ago
|
||
> I I understand it correctly, all the docShells are set to mIsPrinting:
Ah, ok. Check to make sure via printf or something, but looks like it...
> But you mean you want the load to happen after printing/print preview has done?
> Wouldn't that be very odd?
For print preview, yes. For printing, no (imo).
Assignee | ||
Updated•19 years ago
|
Attachment #188698 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•19 years ago
|
||
Well, I tried to use something like this:
nsCOMPtr<nsIContentViewer> kungfuDeathGrip = mContentViewer;
if (mContentViewer) {
nsCOMPtr<nsIDocumentViewerPrint> docviewer =
do_QueryInterface(mContentViewer);
if (docviewer) {
if (docviewer->GetIsPrintPreview()) {
return NS_OK; // JS may not handle returning of an error code
}
}
}
to get only print preview, but that doesn't work. Complaints about
nsIDocumentViewerPrint not existing.
Any ideas to check if only printpreview is turned on?
The other patch - "patch3 for escape" - can be checked in, by the way, if anyone
likes that (it's more Neil's patch than mine).
Comment 19•19 years ago
|
||
> Complaints about nsIDocumentViewerPrint not existing.
Did you include nsIDocumentViewerPrint.h?
In any case, I think I'd like some UI-folks buy-in on what should happen here
for print....
Assignee | ||
Comment 20•19 years ago
|
||
(In reply to comment #19)
> Did you include nsIDocumentViewerPrint.h?
Yes, when I do that, it also complains that that file doesn't exist.
Comment 21•19 years ago
|
||
Ah, that's not exported out of layout/base.... That would do it.
Assignee | ||
Comment 22•19 years ago
|
||
For clarity's sake, "patch3 for escape" was checked in at 2005-07-19 13:43.
Comment 23•19 years ago
|
||
So is this bug fixed, then?
Assignee | ||
Comment 24•19 years ago
|
||
(In reply to comment #23)
> So is this bug fixed, then?
No, the original bug is still there.
Comment 25•19 years ago
|
||
*** Bug 325349 has been marked as a duplicate of this bug. ***
Comment 26•18 years ago
|
||
observed on linux as well.
resolvable with: https://bugzilla.mozilla.org/show_bug.cgi?id=133787
(as noted by aleksej)
OS: Windows XP → All
Hardware: PC → All
Comment 27•18 years ago
|
||
Discovered this bug today during QA. I reproduced it by having print preview open and launching a link from another application (Mirc in this case) and when it opened a new tab in Firefox it opened it inside the print preview pane and closing it proved difficult. Confirmed in FF 1.5.0.8 RC2 and the current Bon Echo build on Windows XP x64.
The F5 bug still seems present although I find it only really happens with two or more tabs open.
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.8) Gecko/20061025 Firefox/1.5.0.8 - Build ID: 2006102516
Comment 28•18 years ago
|
||
*** Bug 363778 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 30•17 years ago
|
||
I crash now in current trunk build with the steps to reproduce.
I filed bug 396024 for that, since a fix for this, doesn't necessarily has to fix that crash.
But either way, it's probably important to prevent this crash from occuring this way, so I think this bug and/or bug 396024 needs to be fixed for Firefox3.
Depends on: 396024
Flags: blocking1.9?
Assignee | ||
Comment 31•17 years ago
|
||
Ok, I finally know how to export from layout/base.
I'm not sure if this is the right solution, though. Why shouldn't someone from script allow to reload a document in print preview?
Attachment #280747 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 32•17 years ago
|
||
Comment 33•17 years ago
|
||
Comment on attachment 280747 [details] [diff] [review]
docshellpatch
This looks about right, but no need for that export and the complicated code. Just change the mFiredUnloadEvent check to be more like the check in GoBack() and company.
Attachment #280747 -
Flags: review?(bzbarsky) → review-
Comment 34•17 years ago
|
||
i'm not really sure what the impact of this is and whether it should be blocking or not. bz thoughts?
Comment 35•17 years ago
|
||
> i'm not really sure what the impact of this is
Most likely crashes under the control of websites if a site does window.open and then you print preview in the window it opened. I think we should block on the patch from comment 33 as adjusted to comment 34.
Assignee | ||
Comment 36•17 years ago
|
||
You mean like this?
But I'm confused about this, because this is basically what my first patch is, but you didn't want that because you wanted printing to not behave like this.
Attachment #282302 -
Flags: review?(bzbarsky)
Comment 37•17 years ago
|
||
Comment on attachment 282302 [details] [diff] [review]
patch?
>Index: docshell/base/nsDocShell.cpp
>+ if (mFiredUnloadEvent || !IsNavigationAllowed()) {
That should just be |if (!IsNavigationAllowed()) {|
And yes, I think we could do better. But for 1.9 it's apparently not happening....
Attachment #282302 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 38•17 years ago
|
||
Ok, thanks. (this is not really my patch, but anyway)
Attachment #188698 -
Attachment is obsolete: true
Attachment #280747 -
Attachment is obsolete: true
Attachment #280749 -
Attachment is obsolete: true
Attachment #282302 -
Attachment is obsolete: true
Attachment #282319 -
Flags: review?(bzbarsky)
Updated•17 years ago
|
Attachment #282319 -
Attachment is patch: true
Attachment #282319 -
Attachment mime type: application/octet-stream → text/plain
Comment 39•17 years ago
|
||
Comment on attachment 282319 [details] [diff] [review]
patch
r=bzbarsky
Attachment #282319 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #282319 -
Flags: superreview?(jst)
Updated•17 years ago
|
Attachment #282319 -
Flags: superreview?(jst)
Attachment #282319 -
Flags: superreview+
Attachment #282319 -
Flags: approval1.9+
Assignee | ||
Updated•17 years ago
|
Assignee: printing → martijn.martijn
Flags: blocking1.9?
Assignee | ||
Comment 40•17 years ago
|
||
Checking in nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp
new revision: 1.859; previous revision: 1.858
done
Checked into trunk.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•17 years ago
|
||
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092904 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Comment 42•17 years ago
|
||
After closing Print dialog ,navigation keys still works on:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010813 Minefield/3.0b3pre
Assignee | ||
Comment 43•16 years ago
|
||
(In reply to comment #42)
> After closing Print dialog ,navigation keys still works on:
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010813
> Minefield/3.0b3pre
I'm not sure what you mean. Is there still a problem here?
You need to log in
before you can comment on or make changes to this bug.
Description
•