134345 - Improve Sidebar discoverability when Sidebar is closed

Status: VERIFIED FIXED

(SeaMonkey :: Sidebar, defect, P1)

Description

[Evelyn Prime-MacAdam]

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

Comment 1

[Peter Trudelle]

Marlon, would you please transfer that spec to mozilla.org and get some public
feedback/review?

Comment 2

[marlon bishop]

just transferred spec to mozilla.org for review.  give it an hour 

Comment 3

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 4

[scottputterman]

marking adt1 per nav triage

Comment 5

[Samir Gehani]

Nav triage team: nsbeta1+/adt1

Comment 6

[rbs]

possibily insert a persistent "sidebar icon" prior the other tools' icons?
http://bugzilla.mozilla.org/attachment.cgi?id=38694&action=view

Comment 7

[Samir Gehani]

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.

Comment 8

[Samir Gehani]

Attachment: Patch v1.0.

Comment 9

[Samir Gehani]

morse, please r.
hewitt, please sr.

Thanks.

Comment 10

[Stephen P. Morse]

Comment on attachment 79073 [details] [diff] [review]
Patch v1.0.

r=morse

Comment 11

[Joe Hewitt (gone)]

+  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.

Comment 12

[Samir Gehani]

> 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).

Comment 13

[rbs]

> 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. 

Comment 14

[scottputterman]

changing to [adt1 rtm].  We can ship beta without this.  Let's reconsider this
for rtm.

Comment 15

[Samir Gehani]

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)

Comment 16

[Samir Gehani]

Attachment: Patch v1.1.

sr=jag (last week).

Comment 17

[Samir Gehani]

Comment on attachment 80714 [details] [diff] [review]
Patch v1.1.

r=morse; sr=jag

Comment 18

[Evelyn Prime-MacAdam]

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.

Comment 19

[Samir Gehani]

Checked into trunk.  Seeking approval to land on branch (added adt1.0.0 keyword).

Comment 20

[Peter Trudelle]

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.

Comment 21

[scottputterman]

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.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

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 ;) 

Comment 23

[Mike Pinkerton (not reading bugmail)]

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).

Comment 24

[Tobias Tinkerman]

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.

Comment 25

[Aaron Kaluszka]

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"

Comment 26

[scottputterman]

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.

Comment 27

[sujay]

I see the new icon on the sidebar.. so that works..

however we have a new bug 139741 which I can reproduce.

Comment 28

[sujay]

also another new bug having to do with tooltip state for the new sidebar icon:
bug 139691

Comment 29

[Michele Carlson]

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

Comment 30

[Samir Gehani]

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.  

Comment 31

[Scott MacGregor]

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. 

Comment 32

[Brendan Eich [:brendan]]

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

Comment 33

[Brendan Eich [:brendan]]

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

/be

Comment 34

[Morten Nilsen]

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

Comment 35

[Samir Gehani]

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.

Comment 36

[Samir Gehani]

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.

Comment 37

[timeless]

at last check, seamonkey was open although there was a commercial blocker.

Comment 38

[Robert Kaiser]

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...)

Comment 39

[Håkan Waara]

We keep adding more chrome when what we should focus on, is putting our chrome
on a big diet.

Comment 40

[scottputterman]

adding adt1.0.0- for beta.  Let's try again for rtm.

Comment 41

[[e-mail]]

I agree, that separator should have position="2".

Comment 42

[Scott Collins]

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?

Comment 43

[Samir Gehani]

Turned the pref for the icon off per discussion with Brendan till we get
agreement from module owners and drivers.  

Comment 44

[[not reading bugmail]]

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.

Comment 45

[Matthew T (active 1999-2002)]

*** Bug 139962 has been marked as a duplicate of this bug. ***

Comment 46

[Robert Kaiser]

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.

Comment 47

[Judson Valeski]

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 .

Comment 48

[Gavin Sinclair]

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.

Comment 49

[Gavin Sinclair]

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.

Comment 50

[Samir Gehani]

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.

Comment 51

[Asa Dotzler [:asa]]

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.

Comment 52

[[e-mail]]

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);

Comment 53

[Samir Gehani]

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.

Comment 54

[sujay]

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.

Comment 55

[rbs]

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."

Comment 56

[Peter Trudelle]

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.

Comment 57

[Adam D. Moss]

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.

Comment 58

[Peter Trudelle]

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.

Comment 59

[Ben Guthrie]

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

Comment 60

[Peter Trudelle]

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.

Comment 61

[Robert Kaiser]

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

Comment 62

[Adam D. Moss]

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?

Comment 63

[Peter Trudelle]

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.

Comment 64

[rbs]

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.

Comment 65

[Peter Trudelle]

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.

Comment 66

[rbs]

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.

Comment 67

[Christine Wozniak]

Adding custrtm-; no impact on customization.

Comment 68

[Samir Gehani]

Sujay (QA) has tested a build with the button (commercial-only) in modern and
signed off on it.  

Comment 69

[Jaime Rodriguez, Jr.]

Removing adt1.0.0-, for reconsideration for final release.

Comment 70

[Samir Gehani]

Attachment: Patch v2.0.

Only add sidebar button to personal toolbar in the netscape builds.

Comment 71

[Blake Ross]

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.

Comment 72

[Samir Gehani]

Attachment: Patch v2.1.

Forgot a file (sidebarButton.js).

Comment 73

[Samir Gehani]

Attachment: Patch v2.2.

Incorporated caillon's feedback.

Comment 74

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 86718 [details] [diff] [review]
Patch v2.2.

r=caillon

Comment 75

[Joe Hewitt (gone)]

Comment on attachment 86718 [details] [diff] [review]
Patch v2.2.

sr=hewitt

Comment 76

[Samir Gehani]

Bringing on adt's radar by adding the ``adt1.0.1'' keyword.

Comment 77

[Samir Gehani]

Mail sent to drivers seeking checkin approval for the mozilla portion of this fix.

Comment 78

[Mike Shaver (:shaver emeritus)]

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.

Comment 79

[scottputterman]

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.

Comment 80

[rchen]

Approved for UI change from L10N. Please have it check in before 06/10. Thanks.

Comment 81

[Jaime Rodriguez, Jr.]

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!

Comment 82

[Judson Valeski]

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.

Comment 83

[Samir Gehani]

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.

Comment 84

[Ricky Webb]

I'm not seeing the icon in 2002061106. Was this only added in the commercial builds?

Comment 85

[Samir Gehani]

Fixed for commercial builds only on branch.  (Should show up in next commercial
branch builds.)

Comment 86

[[e-mail]]

-  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;

Comment 87

[Stephen P. Morse]

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.

Comment 88

[[e-mail]]

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...

Comment 89

[Mike Shaver (:shaver emeritus)]

.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?

Comment 90

[[e-mail]]

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?

Comment 91

[Peter Trudelle]

On the Tab Bar?

Comment 92

[Jaime Rodriguez, Jr.]

claudius - can you pls verify this one on the branch (in sujay's absence) the
replace"fixed1.0.1", with "verified1.0.1"? thanks!

Comment 93

[/\/\arcio Galli]

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. 

Comment 94

[sujay]

verified in 7/18 branch

removing fixed1.0.1 keyword

will re-verify when this fix lands on the trunk.

Comment 95

[Tiger Feng]

Some other bugs found as Bug 152228.
Is it caused by this patch?
I mean the toolbar > .toolbar-prefix part?

Comment 96

[Erich 'Ricky' Iseli]

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.

Comment 97

[shliang]

Attachment: updated patch for the trunk

Comment 98

[shliang]

reassigning

Comment 99

[Samir Gehani]

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.

Comment 100

[jag (Peter Annema)]

Comment on attachment 111667 [details] [diff] [review]
updated patch for the trunk

sr=jag

Comment 101

[Christian :Biesinger (don't email me, ping me on IRC)]

wow, is the Txul and Ts win from this patch here for real?

Comment 102

[shliang]

fixed

Comment 103

[R.K.Aa.]

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?

Comment 104

[Grace Bush]

verified all platforms 3/11 builds

Comment 105

[R.K.Aa.]

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?

Comment 106

[Grace Bush]

Bug 88725 looks like a duplicate to me

Comment 107

[shliang]

the button is only in ns, the fix that was checked in was for the mozilla
changes needed to do that