Closed Bug 134345 Opened 18 years ago Closed 17 years ago

Improve Sidebar discoverability when Sidebar is closed

Categories

(SeaMonkey :: Sidebar, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: evelyn, Assigned: shliang)

References

()

Details

(Keywords: icon, Whiteboard: [adt1 rtm][icon],custrtm-)

Attachments

(1 file, 5 obsolete files)

Per spec at:  http://rocknroll/users/marlon/publish/sidebar/machv.html

Implement option 4. Sidebar Icon/Toolbar Button with "Integrated Appearance"

Just as users before have been unable to find the Sidebar item in the View menu
to close it (evidenced in user feedback and usability studies), they will not be
able to find it to *reopen* the feature once they click on the newly implemented
X and close out the sidebar. 

Nominating nsbeta1
Keywords: nsbeta1
Marlon, would you please transfer that spec to mozilla.org and get some public
feedback/review?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → mozilla1.0
just transferred spec to mozilla.org for review.  give it an hour 
Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!
marking adt1 per nav triage
Whiteboard: [adt1]
Nav triage team: nsbeta1+/adt1
Keywords: nsbeta1nsbeta1+
possibily insert a persistent "sidebar icon" prior the other tools' icons?
http://bugzilla.mozilla.org/attachment.cgi?id=38694&action=view
Adding icon keyword and status whiteboard tag (per Marlon).  Probably best to
check the icons in at the same time as the code is checked in since we are in
the end game of mozilla1.0.
Keywords: icon
Whiteboard: [adt1] → [adt1][icon]
Whiteboard: [adt1][icon] → [adt1][icon][ETA: 04/12 + time to get icons]
Attached patch Patch v1.0. (obsolete) — Splinter Review
morse, please r.
hewitt, please sr.

Thanks.
Keywords: patch, review
Comment on attachment 79073 [details] [diff] [review]
Patch v1.0.

r=morse
Attachment #79073 - Flags: review+
+  var newValue = false;
+  if (sidebarButton)
+  {

You can declare that newValue inside the next block since you only use it there.

+if (document.documentElement.id != "pref-navigator")
+{
+  addEventListener("load", sidebar_overlay_init, false);
+  addEventListener("unload", sidebar_overlay_destruct, false);
+}

Not crazy about the way this breaks encapsulation.  Should this overlay really
have to know about which xul docs are including it?  I'd prefer to make these
init and destruct methods safe to call from pref-navigator.  Perhaps you should
check for the existence of the sidebar box in the xul document.
> You can declare that newValue inside the next block since you only use 
> it there.

I was under the errorneous impression that declaring it in an ``internally''
scoped block would cause a JS strict warning (folks have filed bugs saying so
before).  But running with the JS strict mode pref turned on proved me wrong. 
Fix incorporated.

> Not crazy about the way this breaks encapsulation.  Should this overlay really
> have to know about which xul docs are including it?  I'd prefer to make these
> init and destruct methods safe to call from pref-navigator.  Perhaps you should
> check for the existence of the sidebar box in the xul document.

Yes, I didn't like this either.  I'll change it to check for the existence of
the ``sidebar-box''.  I'll move the check into the init and destruct functions
(the extra load and unload listeners shouldn't affect performance since these
will only be dummy calls in the case of pref-navigator).
Whiteboard: [adt1][icon][ETA: 04/12 + time to get icons] → [adt1][icon][ETA: 04/23]
> Implement option 4. Sidebar Icon/Toolbar Button with "Integrated Appearance"

Note that this has the limitation that it won't fit with other non-browser 
specific windows (e.g.,  addressbook, mailnews, or other things that may come 
along later), so it won't scale well. The option in comment 6 still seems 
appealing to me. 
changing to [adt1 rtm].  We can ship beta without this.  Let's reconsider this
for rtm.
Whiteboard: [adt1][icon][ETA: 04/23] → [adt1 rtm][icon][ETA: 04/23]
Regarding comment 14:

I think we should ship the beta with this since we need user feedback on the
usability/discoverability of this feature and it is low risk (isolated).  

Lori, Evelyn, Marlon, feel free to chime in.  :o)
Attached patch Patch v1.1. (obsolete) — Splinter Review
sr=jag (last week).
Attachment #79073 - Attachment is obsolete: true
Comment on attachment 80714 [details] [diff] [review]
Patch v1.1.

r=morse; sr=jag
Attachment #80714 - Flags: superreview+
Attachment #80714 - Flags: review+
Scott - this is an ADT1 and will be landing tonight with low risk. This is a PRD
item and we want to test it in beta to ensure that it has the desired end
user-benefit by reviewing feedback.
Checked into trunk.  Seeking approval to land on branch (added adt1.0.0 keyword).
Keywords: reviewadt1.0.0
Whiteboard: [adt1 rtm][icon][ETA: 04/23] → [adt1 rtm][icon][ETA: upon approval for branch checkin]
Better to take a little risk to get thig in the beta, where there is time to
recover, rather than taking the risk in RTM, where there is none.
adding adt1.0.0+.  Please check this into the branch as soon as possible add the
fixed1.0.0 keyword.  This is pending approval from mcarlson for the string
changes.  It might just be easier to skip the tooltips until rtm.
Keywords: adt1.0.0adt1.0.0+
Aside from issues such as the fact that it's impossible to hide this personal
toolbar space eating button, this "discoverability" improvement seems to have
regressed sidebar usability. See bug 139741. Suggest not describing patches as
"low risk" until they've been sufficiently tested ;) 
why can't this button be removed? i think that's a fairly serious issue. our
chrome is overly-complicated as is, why are we adding MORE buttons to make it
less complicated? 

When I hide the sidebar, I want no indication of a sidebar. I don't want to see
anything in the chrome about a sidebar. This button should go away when I've
removed the sidebar from my window (with the View menu).
Here's some interesting things about the new button:

Just lost another personal bookmark from the good ol' personal toolbar pushed
off the screen due to the new button, hooray!  

Is the button supposed to completely close the sidebar or just minimize it? 
Because after a restart it will only minimize the sidebar.  Also, in this
closed/minimized state, if I drag the sidebar open hit the button, it works in
reverse - open will close, close will open.  If I use the button to close the
sidebar, or minimize, and drag open, shouldn't the button change its state?  It
currently does not and retains its closed state.

Here's another thought. Why don't we just split the Location bar and Nav bar
into 2 separate toolbars already!!??  That gets the **** out of the personal bar
and you have the whole rest of the nav bar to fill with this new sidebar button,
home button, etc. and THEN make it removable from there.  Oh, but that's all
part of the customizable toolbar bug which will be at a standstill for another 3
yrs.
Closing and opening the sidebar manually with the menu or F9 toggles the state
of the new button no matter what state the sidebar is in.  This also leads to
the previously mentioned "open will close, close will open"
removing adt1.0.0+.  Please address the concerns mentioned in this bug and make
sure that this didn't cause the empty sidebar bug 139741.
Keywords: adt1.0.0+adt1.0.0
I see the new icon on the sidebar.. so that works..

however we have a new bug 139741 which I can reproduce.
also another new bug having to do with tooltip state for the new sidebar icon:
bug 139691
Please do not make any UI changes at this point. We are 3 1/2 weeks PAST
localization freeze. If we want to sim-ship products, then this must be respected.
thanks
From comment 22:
> Aside from issues such as the fact that it's impossible to hide this personal
> toolbar space eating button, this "discoverability" improvement seems to have
> regressed sidebar usability. See bug 139741. Suggest not describing patches as
> "low risk" until they've been sufficiently tested ;) 

Not being able to hide this button from a pref is an oversight of my checkin.  I
forgot one file that sets ids so they can be overlaid.  I have commented in bug
139741 why this is not a smoketest blocker or a regression.  At any rate, I'll
have the missed file checked in.  I'll also fix bug 137941 of course.  
I just filed a spin off bug, 139813 for adding a checkbox under the navigator
prefs panel for turning off the sidebar icon in toolbars. This keeps it
consistent with all the other items we add to the toolbar. You should be able to
remove it just like you can remove print, search, mail, etc. from the toolbar. 
This should not have been checked into cvs.mozilla.org.  It should be backed
out, or pref'd off by default, in Mozilla.  We've been over this personal
toolbar raid recently, and the covenant was to steal space from the PT with
hacks in the ns tree only.  Nothing has changed since then.

The way this was reviewed is fishy too.  I'm not happy about any of this.

/be
Drives won't be approving anything like this for the 1.0 branch, btw.

/be
also, this checkin introduced a javascript warning;

Warning: reference to undefined property this._elementIDs
Source File: chrome://communicator/content/sidebar/sidebarOverlay.xul
Line: 80

which is the most annoying one at the moment in my tree.
it appears six times whenever I open a new navigator window.

please either back this patch out per comment 32, or fix it up properly
Regarding all the comments that there is no pref to turn this icon off (from
comment 30 by me):

Not being able to hide this button from a pref is an oversight of my checkin.  I
forgot one file that sets ids so they can be overlaid.

Brendan,
Regarding this being a fishy checkin, I'm not sure what was fishy about it.  It
was reviewed.  Then hewitt sr'ed it and made some comments.  I addressed them. 
Then I approached jag since he was around at the time and he sr'ed it with my
word that I had addressed hewitt's comments.  I am open to correcting what I did
wrong.  Please advise.  Thanks.
Brendan,
If drivers dictates that this be backed out from cvs.mozilla.org say the word
and I'll do it when the tree opens.
at last check, seamonkey was open although there was a commercial blocker.
Samir, an additional thing I saw, and I first thought it might just be a bug of
my own theme:

The seperator you added which should be at the right of this button is actually
showing at the very right of personal toolbar! Should you add some position=
magic to that, too? (And switch it off together with the button, of course...)
We keep adding more chrome when what we should focus on, is putting our chrome
on a big diet.
adding adt1.0.0- for beta.  Let's try again for rtm.
Keywords: adt1.0.0adt1.0.0-
I agree, that separator should have position="2".
I feel I must recapitulate brendan's words in comment #32.  How did this get
into the trunk, and why aren't we backing it out and getting into the ns tree
where it belongs and where we agreed this machinery must go?
Turned the pref for the icon off per discussion with Brendan till we get
agreement from module owners and drivers.  
I have to say that this a really good button.. if we had another for full-screen
mode we'd be well on our way to making an idiot proof browser.  It works good..
good work.
*** Bug 139962 has been marked as a duplicate of this bug. ***
Samir:
I see major problems left:

1) The separator is still at the right edge of the toolbar while it should be at
the right of the sidebar button

2) The separator doesn't hide together with the sidebar button

3) Hiding sidebar button doen't persist at a restart of Mozilla, it only hides
during the current session and is shown again on restart. Even worse, the pref
panel thinks it's hidden, so you have to call prefs, check the item so that
prefs think it's turned on again, click OK, call prefs again, uncheck it and
click OK again to hide the button - and redo all that after every launch of Mozilla.
Using today's trunk build. I can't figure out how to get rid of the icon (how do
I do this?); it's taking up my real-estate on the toolbar, and even when the
sidebar is collapsed, I still have a fat grippy vertical bar on the left side of
the window .
Using rc1 2002042510 build.

The new button that has appeared in the personal folder is confusing. It doesn't
fit in with the modern theme because it uses quite bold red and green colours (I
don't know why red is open sidebar and green is close sidebar - but that is
another matter) everywhere else in the modern theme there is much subtler use of
colour highlighting.

The open and close tooltips/colours and directions of arrows are not consistent
and about half the time the button to open sidebar has tooltip 'close sidebar'.

The preference to get rid of this was unchecked by default - so to get rid of
the thing I had to check for display then uncheck to finally get rid of it.

Following a switch of themes - the sidebar button had some effect but the
sidebar is now empty when I open it with the button. 

I have recovered the contents of the sidebar by using view > Show/hide >
sidebar. Side bar discoverability is of little use if I discover an empty panel
taking 1/5 of my screen.

1. Switch from modern to classic theme with sidebar button on.
2. close browser
3. open browser (still in modern)
4. close browser and quick start.
5. open browser (now in classic theme)
6. click sidebar button

expect to see sidebar with all tabs and content
actual saw sidebar with close button and tab pulldown - no tabs or content.

The pref isn't quite working right because the personal toolbar maintains hidden
state for personal toolbar buttons in 2 places: prefs.js and localstore.rdf.  I
plan to fix this and other problems with the implementation.  I am waiting for a
discussion with UE about making the toolbar button work like F9: show or hide
(and do not involve the collapsed state which introduces potential for errors
and is confusing to users; the collapsed state existed as affordance to
rediscover the sidebar once closed and the new toolbar button obviates this need).

To get rid of the sidebar icon you can follow these steps:
(a) turn it on from prefs
(b) close prefs
(c) go back and turn it off from prefs
The icon should be gone.

Also, the icons are not by any means final.  They are glaringly
counter-intuitive to make sure we don't ship with them.  :o)  The real icons
will come from Marlon.  This is why I added the `icon' keyword.
Samir, this has been a poor landing of a new feature. It has many problems and
is causing users pain. Can you please pull this until the problems are solved.
How did this lame hack get checked into Mozilla?
You really need a sidebar-prefOverlay.xul to do this right.

      // add a popupset to quiesce complaints from the cookie context menus
      const kXULNS = 
        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
      var popupset = document.createElementNS(kXULNS, "popupset");
      popupset.setAttribute("id", "contentAreaContextSet");
      document.documentElement.appendChild(popupset);
Yes, this landing has many problems with it that I would have addressed faster
but it is yet unclear to me whether this will eventually make it into mozilla
since some navigator module owners want it whereas some drivers do not.  

Feature backed out.
Keywords: patch
Whiteboard: [adt1 rtm][icon][ETA: upon approval for branch checkin] → [adt1 rtm][icon]
I have installed the 4/26 trunk and I can get rid of the icon
following these instructions:

To get rid of the sidebar icon you can follow these steps:
(a) turn it on from prefs
(b) close prefs
(c) go back and turn it off from prefs
The icon should be gone.

However, If I exit Netscape and relaunch I get the sidebar icon again
on the toolbar.
Blocks: 143047
From: http://www.zdnet.com/anchordesk/stories/story/0,10738,2867381,00.html

"I also like the way Netscape has implemented the "Sidebar" pane that runs down 
the left edge of the screen. Internet Explorer does the same thing (click on 
Favorites or History and a thin left-hand window opens), but not as well. 
Netscape's Sidebar provides tabbed access to your buddy list, bookmarks, news, 
history, and a variety of other more-or-less useful things, like shopping, 
maps/directions, and movies/music. You can select the sidebar tabs that display 
by default. One problem: Once the Sidebar closed, I had a hard time figuring out 
how to reopen it. I discovered that the easiest way is to press F9."
We did usability tests recently where intermediate to advanced users had no such
problems discovering how to open the sidebar, thanks mostly to a new button to
the left of the Personal Toolbar.
This button is not on the mozilla trunk, right?
I know there was briefly such a button on the trunk
but it got removed (rightly so) because it had some
real issues, but I think that the idea sound.
It was tested in a developer build.  The design was good, but the landing had
problems, and some people took issue with it.  This will apparently only be a
solution for Netscape users, which I think is unfortunate.
Peter, is their a way we can add a line in our user.js to get the button back? 
I know it's stupid, but I found myself using the sidebar when the button was
there. without it I only seem to remember the sidebar is there when I need to
look at my history
Not unless mozilla is willing to take it.  BTW, I don't think your scenario is
the least bit stupid.  Ease of use is not just for beginners.
If I get it right, the bug was left open because we (most contributors) have no
problem with that button, as long as it can be turned off with a pref - as in 
"completely turned off", like other buttons in location bar or personal toolbar.

The problem it was backed out was
1) when you turned the pref off, the button was back on Mozilla restart, and
hard to get away again
2) the "seperator" that should have been between that button and the home
button, appeared completely mis-placed and couldn't even be turned off with the
pref.

Many people argued they'd have no problem with the button if it
1) was able to switched off completely and
2) was disabled by default
Issue 3) The button's iconic view of whether the
sidebar was already open or not often didn't
match reality.

Do you know what issues the drivers might have
had that preclude this from getting back into
mozilla in any shape or form or is there just
a communications breakdown?

Some driver comments in here seem opposed to any solution that further takes
space away from the personal toolbar.  Also, it is likely that the
implementation would degrade startup time by some 0.02%, or more than 20ms.
Do you have any comment for my suggestion in comment 6? It isn't conventional
but it is worth a thought or two. Plus, you could well try it in your usuability
lab and see how it fares.
That seems a good idea too, IMO. As i recall, this and a few suggestions like it
were considered.  We didn't test others in the lab, as there really much reason
to, since the first option we picked tested so well.
As per your comment #58, that first option is unfortunately remaining a
Netscape-only possibility, whereas the other option might offer a middle ground
that might appeal to all interested parties.
Adding custrtm-; no impact on customization.
Whiteboard: [adt1 rtm][icon] → [adt1 rtm][icon],custrtm-
Sujay (QA) has tested a build with the button (commercial-only) in modern and
signed off on it.  
Removing adt1.0.0-, for reconsideration for final release.
Keywords: adt1.0.0-
Attached patch Patch v2.0. (obsolete) — Splinter Review
Only add sidebar button to personal toolbar in the netscape builds.
Attachment #80714 - Attachment is obsolete: true
Good. We can always use another button on the "personal" toolbar.  Hopefully
there will still be enough room on the toolbar in rtm for the user to put his
favorite bookmark.
Attached patch Patch v2.1. (obsolete) — Splinter Review
Forgot a file (sidebarButton.js).
Attachment #86667 - Attachment is obsolete: true
Attached patch Patch v2.2. (obsolete) — Splinter Review
Incorporated caillon's feedback.
Attachment #86684 - Attachment is obsolete: true
Comment on attachment 86718 [details] [diff] [review]
Patch v2.2.

sr=hewitt
Attachment #86718 - Flags: superreview+
Bringing on adt's radar by adding the ``adt1.0.1'' keyword.
Keywords: adt1.0.1
Mail sent to drivers seeking checkin approval for the mozilla portion of this fix.
Is there a reason that a dynamic overlay can't be used to do this all from the
commercial tree?  I thought we Had The Technology.
My guess is that this won't be allowed in the 1.1alpha build, so could you make
a test build and get QA verification?

In addition, please get rchen's and drivers approval and checkin the string
changes asap because of the l10n freeze.
Approved for UI change from L10N. Please have it check in before 06/10. Thanks.
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
this in asap, then add the "fixed1.0.1" keyword. thanks!
Keywords: adt1.0.1adt1.0.1+
Attachment #86718 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Samir, also please provide the text you sent to drivers in response to shaver's
question in this bug.
Keywords: mozilla1.0.1+
Sure Jud.  

I mentioned that we are doing surgery on the toolbar XBL and adding notification
of state change to the core sidebar code which were needed in order to achieve
Marlon's seamless sidebar ``meta tab'' button (which is why the mozilla code was
affected as well).  So the code change is not specific to the sidebar button but
provides infrastructure for this button.  Per development and testing this
change should be benign in mozilla.
I'm not seeing the icon in 2002061106. Was this only added in the commercial builds?
Fixed for commercial builds only on branch.  (Should show up in next commercial
branch builds.)
-  var is_collapsed =
document.getElementById('sidebar-box').getAttribute('collapsed') == 'true' ?
true : false;
+  var is_collapsed =
document.getElementById('sidebar-box').getAttribute('collapsed') == 'true';

I just had to laugh:

var is_collapsed = document.getElementById('sidebar-box').collapsed;
Keep laughing as you ponder this:

   var z = 'false';
   if (z) alert ("z = true");

will print out z = true.  However if the code used true and false (instead of 
'true' and 'false'), what you said would be correct.

Um... the code did not at any point use 'false'.
It did use 'true', but only to compare the return value of getAttribute.

So now I'm crying...
.collapsed should be a JS boolean, not a string, if I'm reading the code
correctly.  (This is different from getAttribute('collapsed'), but I'm sure you
all knew that.)  Does .collapsed really provide a stringified boolean value?
I don't really care, at least the code is better than it was.

How about putting the sidebar show/hide button next to the new tab button?
On the Tab Bar?
claudius - can you pls verify this one on the branch (in sujay's absence) the
replace"fixed1.0.1", with "verified1.0.1"? thanks!
Peter, 

I agree with Neil. I think usability is better if we place that icon in the Tab
bar (which is more associated with the idea of the *application* instead the
*user personal links*). Fixing this and placing there will be less confusing to
the user and more effective (the access to the button):

Right now, the bad impact of placing in the Personal Toolbar is the fact that is
so easy to collapse and we don't want to loose the Sidebar Reopen button when
users are collapsing their personal links. 
verified in 7/18 branch

removing fixed1.0.1 keyword

will re-verify when this fix lands on the trunk.
Some other bugs found as Bug 152228.
Is it caused by this patch?
I mean the toolbar > .toolbar-prefix part?
Tell me if I'm missing anything. But if you put it on the _tab_ bar, it won't be
visible at all on startup, unless you have the pref "Hide the tab bar when only
one tab is open" checked.
Attachment #86718 - Attachment is obsolete: true
Attachment #111667 - Flags: superreview?(sgehani)
Attachment #111667 - Flags: review?(jaggernaut)
reassigning
Assignee: sgehani → shliang
Status: ASSIGNED → NEW
Comment on attachment 111667 [details] [diff] [review]
updated patch for the trunk

>+#popupIcon {
>+  list-style-image: url("chrome://navigator/skin/icons/popup-blocked.png");
>+}
>+

Not part of this patch?

>+dump("prefixhidden: " + !buttonShown + "\n");

Elminate this dump statement.

r=sgehani with those changes.
Attachment #111667 - Flags: superreview?(sgehani)
Attachment #111667 - Flags: superreview?(jaggernaut)
Attachment #111667 - Flags: review?(jaggernaut)
Attachment #111667 - Flags: review+
Comment on attachment 111667 [details] [diff] [review]
updated patch for the trunk

sr=jag
Attachment #111667 - Flags: superreview?(jaggernaut) → superreview+
wow, is the Txul and Ts win from this patch here for real?
fixed
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
was this fix supposed to add a button somewhere in Mozilla, so it becomes
possible to open the sidebar without digging in the view menu?
QA Contact: sujay → gbush
verified all platforms 3/11 builds
Status: RESOLVED → VERIFIED
was this fix supposed to add a button somewhere in Mozilla, so it becomes
possible to open the sidebar without digging around in the view menu?
Is bug 88725 a duplicate of this bug?
Bug 88725 looks like a duplicate to me
the button is only in ns, the fix that was checked in was for the mozilla
changes needed to do that
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.