Closed Bug 68541 Opened 24 years ago Closed 21 years ago

no checkmark for View|Toolbar in bookmarks manager

Categories

(SeaMonkey :: Bookmarks & History, defect, P5)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: bzbarsky, Assigned: bugs)

References

Details

(Keywords: helpwanted, polish, Whiteboard: [br])

Attachments

(2 files, 3 obsolete files)

BUILD: Linux 2001-02-11-06 and Windows 2001-02-10 STEPS TO REPRODUCE: 1) open bookmark manager 2) open the view menu. EXPECTED RESULT: See a checkmark next to the "Toolbar" option ACTUAL RESULT: No checkmark. Switching the toolbar on and off does not fix this.
Blocks: 68550
*** Bug 69240 has been marked as a duplicate of this bug. ***
Toggling the toolbar (switching on->off, off->on) "fixes" this. But it's still a problem. Bookmarks Manager should check at startup whether toolbar is showed and then check/uncheck based on that information. Downgrading to trivial
Severity: minor → trivial
Keywords: polish, ui
Whiteboard: [br]
Wieeerd. I tried to add a dump(document.getElementById("command-toolbar").hidden); in the Startup() function of bookmarks.js, but it returned "undefined". Also (document.getElementById("viewCommandToolbar").checked is undefined. If the "hidden" attribute of the toolbar wasn't undefined when the window is launched, the checkmark would probably be there. Any idea? bz? hwaara?
Attached patch patch; add persist="checked" to the menuitem (obsolete) (deleted) — Splinter Review
Makes sense. r=hwaara
a=dbaron for trunk checkin
Blocks: 83989
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Seeing this on new installs of build 2001100303 on Win2K. Checking and unchecking fixes the problem, so it looks like a one-time issue, but it's back.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image Screenshot of bug on today's build. (deleted) —
Status: REOPENED → ASSIGNED
Keywords: helpwanted
Target Milestone: --- → Future
Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter email notifications caused by this by searching for 'ilikegoats'.
Assignee: ben → pchen
Status: ASSIGNED → NEW
Mass move Ben's bugs dumped on me marked future with p5 to get off my untriaged radar. You can filter out this email by looking for "ironstomachaussie"
Priority: -- → P5
mass reassign of pchen bookmark bugs to ben
Assignee: pchen → ben
removing self from cc list
Not seeing this in 20021229 win2k.
Attachment #36233 - Attachment is obsolete: true
Attached patch patch v1.0 (obsolete) (deleted) — Splinter Review
Migrating patch from bug 203818 Same as timeless' patch except the cleanup and the backing out of the previous patch. the persistent attribute is already set in goToggleToolbar.
Attachment #111117 - Attachment is obsolete: true
Attachment #122052 - Flags: superreview?(jaggernaut)
Attachment #122052 - Flags: review?(varga)
Comment on attachment 122052 [details] [diff] [review] patch v1.0 looks good
Attachment #122052 - Flags: review?(varga) → review+
Comment on attachment 122052 [details] [diff] [review] patch v1.0 >Index: communicator/resources/content/utilityOverlay.js >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/communicator/resources/content/utilityOverlay.js,v >retrieving revision 1.74 >diff -u -r1.74 utilityOverlay.js >--- communicator/resources/content/utilityOverlay.js 14 Mar 2003 09:23:14 -0000 1.74 >+++ communicator/resources/content/utilityOverlay.js 29 Apr 2003 19:28:06 -0000 >@@ -160,29 +160,19 @@ > > function goToggleToolbar( id, elementID ) > { >+ var toolbar = document.getElementById(id); >+ var element = document.getElementById(elementID); >+ if (toolbar) > { >+ var isShown = !toolbar.hidden; >+ toolbar.hidden = isShown; > document.persist(id, 'hidden'); >+ if (element) { >+ element.checked = isShown; >+ document.persist(elementID, 'checked'); >+ } > } > } This doesn't seem right. If currently the toolbar is hidden (isShown = false), we unhide it (hidden = false), and we set checked to false (checked = false). Shouldn't that be |element.checked = !isShown|?
Comment on attachment 122052 [details] [diff] [review] patch v1.0 sr=jag with that bug fixed.
Attachment #122052 - Flags: superreview?(jaggernaut) → superreview+
Attached patch patch v1.1 (deleted) — Splinter Review
good catch, Jag.
Attachment #122052 - Attachment is obsolete: true
Comment on attachment 122061 [details] [diff] [review] patch v1.1 Carrying reviews Requesting approval: Risk is low. Strictly speaking, this patch has not been tested on Seamonkey, but it has been tested on phoenix and consumers of |goToggleToolbar| have been checked. (codes are the same in the patch scope).
Attachment #122061 - Flags: superreview+
Attachment #122061 - Flags: review+
Attachment #122061 - Flags: approval1.4b?
Comment on attachment 122061 [details] [diff] [review] patch v1.1 have you smoke tested the toolbars in browser, mail, addrbook, compose, etc?
Comment on attachment 122061 [details] [diff] [review] patch v1.1 "this patch has not been tested on Seamonkey" seek approval after testing. we can't a= something that hasn't even been tested.
Attachment #122061 - Flags: approval1.4b?
Comment on attachment 122061 [details] [diff] [review] patch v1.1 >+ element.checked = isHidden; Maybe in Firebird menuitems have a checked property but in Mozilla they do not, patch is ok if you use element.setAttribute("checked", isHidden);
I think this was fixed by Jan's checkin to bug 205378.
Status: NEW → RESOLVED
Closed: 23 years ago21 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: