Closed Bug 126725 Opened 23 years ago Closed 22 years ago

Personal Toolbar appears when exiting Print Preview

Categories

(Core :: Print Preview, defect, P1)

Product:
Core
Component:
Print Preview
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: deanis74, Assigned: samir_bugzilla)

References

Details

Attachments

(2 files, 1 obsolete file)

much better patch
3.56 KB, patch
Details | Diff | Splinter Review
I am an idiot, but that is okay
3.85 KB, patch
bzbarsky
: review+
alecf
: superreview+
Details | Diff | Splinter Review 
1. View a page
2. Hide the Personal Toolbar
3. File > Print Preview
4. Close Print Preview

Expected results: everything as it was after step 1

Actual results: everything after step 1, plus Personal Toolbar is visible,
although there is no check beside its menu item
UI issue
Assignee: rods → sgehani
Blocks: 126664
Need to remember the state of the main menu bar, the navigation toolbar, the
personal toolbar, and even the links toolbar (aka site navigation bar: bug
1266644).  I need to hide the ``navigator-toolbox'' and append the print preview
toolbar as a visible sibling instead of a child instead of dealing with each of
the afore mentioned toolbars.
No longer blocks: 126664
Nominating.
Keywords: nsbeta1
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
Blocks: 126664
*** Bug 127011 has been marked as a duplicate of this bug. ***
Couldn't this be more easily done by setting the "chromehidden" attribute on the
window, this is an existing method of showing/hiding sections of chrome
(obviously saving the old value for when you exit print preview).

While I'm harping on about print preview, what are all those skin URLs doing in
communicator/content/communicator.css (see also c: below)?

And if you are going to use a binding on the print preview toolbar,
a: you might as well make it a box, rather than a toolbar
b: you might as well use a class, rather than an attribute
c: did you know about XBL scoped stylesheets?
   Rather than including the image styles in communicator.css,
   include a scoped stylesheet for (say) communicator/skin/printPreview.css
Scoped stylesheet would look something like this:

  <resources>
    <stylesheet src="chrome://communicator/skin/printPreview.css"/>
  </resources>
</binding>

Then move all the -list-style-image CSS rules into new skin files.

You might not even have to create and destroy the toolbar each time,
I hear that hidden="true" might do all the work for you.
Yes, I know about XBL-scoped stylesheets.  Thanks.

It was originally a class but I made it an attribute (printpreviewtoolbar ->
toolbar[printpreview="true"]) because my super-reviewer raised the point that we
don't need a new tag/class for this.
Status: NEW → ASSIGNED
*** Bug 127313 has been marked as a duplicate of this bug. ***
IMHO a new class reuses an existing attribute rather than creating a new one.
BTW, Bill Law graciously moved these images into a skin css file being shared by
the page setup UI.  This addresses the concerns in comment 6.
Target Milestone: mozilla0.9.9 → mozilla1.0
I'm also seeing this under Windows XP with 2002030604.  (Although I see no way
of specifying multiple OSs.  Maybe "Windows" or "Windows NT/2000/XP" should be
added?)
nsbeta1- per Nav triage team, ->1.2
Keywords: nsbeta1 → nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.2
*** Bug 130285 has been marked as a duplicate of this bug. ***
Yep, that new skin css file looks great, thank Bill Law for me!
*** Bug 130867 has been marked as a duplicate of this bug. ***
*** Bug 137080 has been marked as a duplicate of this bug. ***
I see this under Linux, with 0.9.9.  Someone should change OS to "all"...
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 137545 has been marked as a duplicate of this bug. ***
*** Bug 138056 has been marked as a duplicate of this bug. ***
*** Bug 138364 has been marked as a duplicate of this bug. ***
*** Bug 140707 has been marked as a duplicate of this bug. ***
*** Bug 143186 has been marked as a duplicate of this bug. ***
*** Bug 147941 has been marked as a duplicate of this bug. ***
This patch addresses the issue of both the Personal Toolbar and the Navigation
Toolbar appearing when exiting Print Preview when they were originally hidden. 
It also fixes the issue with the link toolbar by hiding it while in print
preview and then properly restoring it if necessary when exiting.  Only tested
on linux, but since it is all in javascript it should be platform independent.
Actually read comments in bug and realized that comment #2 is a *much* better
way to do it.  Doh!  This patch is about 40 less lines of code and accomplishes
exactly the same thing and basically follows the suggestion of comment #2.  Old
patch is obsolete.
Attachment #85542 - Attachment is obsolete: true
tabbrowser isn't part of the navigator-toolbox...  Sorry for 'spamming' the bug
with three subsequent patches...
*** Bug 148832 has been marked as a duplicate of this bug. ***
*** Bug 150058 has been marked as a duplicate of this bug. ***
Does that last patch handle the case when other toolbars have been added by
installing XPIs?  (eg. the useragent toolbar, prefs toolbar, etc.)
I just explicitly tested uabar and pubmed toolbars and both work properly(they
hide when entering print preview and then unhide if they are supposed to).  In
general anything that adds itself as a child of navigator-toolbox will work and
this is what most toolbars seem to do.
*** Bug 150507 has been marked as a duplicate of this bug. ***
Ok.  So let me get this straight.  This puts the print preview toolbar next to
the navigator-toolbox instead of inside it, right?  Would it maybe make more
sense to put the toolbar inside navigator-toolbox but make sure we walk the
child list and hide them all first?  (Storing the visible/hidden state for each
one.)

I just ask because I'm not sure toolbars should live outside toolboxes.... 
Another approach would be to have a "print-preview-toolbox" (this may make the
most sense).
> I just ask because I'm not sure toolbars should live outside toolboxes....

When I last looked the print preview toolbar wasn't a real toolbar, i.e. it
doesn't contain a grippy. In another bug I griped that calling it a <toolbar>
didn't make sense but I got ignored.
Comment on attachment 85550 [details] [diff] [review]
I am an idiot, but that is okay

r=zbarsky
Attachment #85550 - Flags: review+
*** Bug 151382 has been marked as a duplicate of this bug. ***
*** Bug 152194 has been marked as a duplicate of this bug. ***
*** Bug 152240 has been marked as a duplicate of this bug. ***
Travis, you requested sr for this, right?
yup, last friday, so the weekend...
Comment on attachment 85550 [details] [diff] [review]
I am an idiot, but that is okay

sr=alecf
Attachment #85550 - Flags: superreview+
fix checked in on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 156640 has been marked as a duplicate of this bug. ***
*** Bug 156639 has been marked as a duplicate of this bug. ***
*** Bug 163747 has been marked as a duplicate of this bug. ***
*** Bug 164106 has been marked as a duplicate of this bug. ***
*** Bug 167201 has been marked as a duplicate of this bug. ***
verified working on recent Win2k trunk build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: