154711 - Make it clearer Home and Bookmarks buttons appear on Personal, and not Navigation, Toolbar

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Tito Sierra]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.0) Gecko/20020529
BuildID:    2002052918

In Navigator Preferences window (under "Select the buttons you want to see in
the toolbar") I check ON all five buttons to display in the toolbar:  Go,
Search, Print, Home, Bookmarks.  However only 3 of these buttons actually
display.  The missing buttons are Home and Bookmarks.  This is the case for both
Modern and Classic themes.

Reproducible: Always
Steps to Reproduce:
1. go to Mozilla, Preferences..., Navigator
2. check ON all toolbar button options
3. close the Preferences window

You should notice that 2 of these buttons do not appear in the toolbar.

Actual Results:  Missing Home and Bookmarks buttons.

Expected Results:  The Home and Bookmarks buttons should be displayed when
checked ON in preferences.

Comment 1

[Matthias Versen [:Matti]]

Can you try to rename/backup/remove the file "localstore.rdf" in your profile ?
(you will loose all window settings and toolbar enable/disable status)

Comment 2

[Pete Boyd]

do you have a USER.JS file in your Mozilla profile directory perhaps with
settings to disable these buttons?, because if you do they would override your
menu choices

Comment 3

[Greg K.]

WorksForMe using FizzillaCFM/2002062203. The preference says "toolbarS," not
"toolbar."

"Home" and "Bookmarks" appear on the Personal Toolbar, not the Navigation Toolbar.

Comment 4

[Tito Sierra]

Greg Kolanek's comment clears this issue up for me.  I had my personal toolbar
turned off (not displayed) so I never noticed the Bookmarks/Home buttons in the
personal toolbar.  Now that I think about it, having these links in the personal
toolbar makes a lot of sense (since they're not navigation functions but
hyperlinks).  My only gripe then, is that the preferences window does not make
clear that the buttons you can check ON appear in either the Navigation Toolbar
or Personal Toolbar.  I switched the severity to "enhancement". 

Comment 5

[Greg K.]

I'll confirm as a trivial bug in its' modified form.

Comment 6

[Gertjan Klein]

I feel at least the home button should be on the navigation toolbar, or better
yet, the toolbars shoul be configurable. I don't need any other buttons from the
personal toolbar, and feel it's a waste of screen real-estate to have it on just
for that button. (Not looking for a debate here, just demonstrating people have
different views on this issue.)

Comment 7

[zug_treno]

Attachment: Diff made with Patch Maker and 1.8b2 Gecko/20050409

This patch moves the Bookmark and Home checkboxes to the second column and adds
text to the column headers. It also adds accesskeys for the checkboxes as a
bonus. I don't know who can r/sr this, so I requested these from Neil because
he's the UI-Czar and I hope he knows who to ask.

Comment 8

[zug_treno]

Attachment: Screenshot

Screenshot of how the changed checkbox columns look with the Classic theme.

Comment 9

[Ian Neal]

Comment on attachment 180199 [details] [diff] [review]
Diff made with Patch Maker and 1.8b2 Gecko/20050409

Some of the indentation looks strange but I put that down to patch maker. Could
you create one with -pud8 for checkin purposes?

Comment 10

[Ian Neal]

As Neil pointed out to me on IRC - checkbox order should be:

[ ] Go          [ ] Home
[ ] Search      [ ] Bookmarks
[ ] Print

so they match the order of how they appear on the toolbar.

Comment 11

[[e-mail]]

Comment on attachment 180199 [details] [diff] [review]
Diff made with Patch Maker and 1.8b2 Gecko/20050409

While this is a good start, I'd appreciate it if you could expand it to include
the following features:
* List the buttons in the order in which they appear in the toolbar
* Change the <groupbox orient="horizontal"> into an <hbox> and the <vbox> tags
into <groupbox>
* Change the <caption label> into <description> and place it before the hbox
* Change the new <description> tags into <caption label>
* Make the indentation consistent (2 spaces indent per new tag, no tabs,
attributes line up as shown)
Also, your diff isn't working properly, probably due to a mixture of types of
line endings.

Comment 12

[[e-mail]]

Comment on attachment 180199 [details] [diff] [review]
Diff made with Patch Maker and 1.8b2 Gecko/20050409

Oh, and IanN suggests putting equalsize="true" on the new hbox and flex="1" on
the new groupboxes.

Comment 13

[Ian Neal]

Attachment: How it could look

Mock up for you to reference

Comment 14

[zug_treno]

Attachment: Second diff made with Patch Maker and 1.8b2 Gecko/20050410

Both thanks for the prompt review and the help with making this patch better.
With this patch the panel should look like the mock up from comment 13. I
remade the patch with another editor so the new diff is smaller, I hope I got
the indentation right.

Comment 15

[Ian Neal]

Comment on attachment 180316 [details] [diff] [review]
Second diff made with Patch Maker and 1.8b2 Gecko/20050410

>+  <description>&toolbarIntro.label;</description>
>+  <hbox flex="1" equalsize="true" id="prefShowButtons">
>+    <groupbox flex="1" id="prefShowButtonsBox1">
>+    <caption label="&navToolbarIntro.label;"/>
>+      <checkbox id="goButton"
>+          label="&goButton.label;"
>+		  accesskey="&goButton.accesskey;"
>+          prefstring="browser.toolbars.showbutton.go"/>
>+      <checkbox id="searchButton"
>+          label="&searchButton.label;"
>+		  accesskey="&searchButton.accesskey;"
>+          prefstring="browser.toolbars.showbutton.search"/>
>+      <checkbox id="printButton"
>+          label="&printButton.label;"
>+		  accesskey="&printButton.accesskey;"
>+          prefstring="browser.toolbars.showbutton.print"/>
>+    </groupbox>
>+    <groupbox flex="1" id="prefShowButtonsbox2">
>+    <caption label="&persToolbarIntro.label;"/>
>+      <checkbox id="homeButton"
>+          label="&homeButton.label;"
>+		  accesskey="&homeButton.accesskey;"
>+          prefstring="browser.toolbars.showbutton.home"/>
>+      <checkbox id="bookmarksButton"
>+          label="&bookmarksButton.label;"
>+		  accesskey="&bookmarksButton.accesskey;"
>+          prefstring="browser.toolbars.showbutton.bookmarks"/>
You still have not got the indents right and you are still using tabs, what it
should be is, for example:
      <checkbox id="bookmarksButton"
		label="&bookmarksButton.label;"
		accesskey="&bookmarksButton.accesskey;"
		prefstring="browser.toolbars.showbutton.bookmarks"/>

>+    </groupbox>
Probably should have in:
      <vbox id="prefShowButtonBox"/>
So others can overlay.
>+  </hbox>

Comment 16

[[e-mail]]

Comment on attachment 180316 [details] [diff] [review]
Second diff made with Patch Maker and 1.8b2 Gecko/20050410

Nit: I think we need to keep as many of the existing ids as possible to avoid
breaking extensions.

Comment 17

[Ian Neal]

Comment on attachment 180316 [details] [diff] [review]
Second diff made with Patch Maker and 1.8b2 Gecko/20050410

Oh, one more thing, you should indent after <groupbox> but not after <caption>

Comment 18

[[e-mail]]

Sorry for the conflict...

You'll need both prefShowButtonsbox and prefShowButtonBox somewhere.

Comment 19

[zug_treno]

Attachment: Third diff also made with PM and /20050410

OK, indentation before <acceskey> redone with 10 spaces (and set my editor to
'replace tab by space'), indented after <groupbox> but not after <caption>,
changed the ids back to their original values and added <vbox
id="prefShowButtonBox"/> between </groupbox> and </hbox>.

Comment 20

[Ian Neal]

Comment on attachment 180350 [details] [diff] [review]
Third diff also made with PM and /20050410

Looks good

Comment 21

[[e-mail]]

Comment on attachment 180350 [details] [diff] [review]
Third diff also made with PM and /20050410

>+      <checkbox id="goButton"
>+          label="&goButton.label;"
You probably misunderstood, the indentation here isn't our preferred style
(although it is our second most preferred style); most preferred would have
been to line up the "la" under the "id".

>+    </groupbox>
>+      <vbox id="prefShowButtonBox"/>
>+  </hbox>
Second nit: wrong indent on the vbox. Can whomever checks this in please fix
this at the same time, thanks.

Comment 22

[Ian Neal]

Attachment: Tweaked patch v0.3a (Checked in)

Carrying forward r/sr and requesting a= for low risk UI tidy up which improves
accessibility

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 181423 [details] [diff] [review]
Tweaked patch v0.3a (Checked in)

a=asa

Comment 24

[Ian Neal]

Comment on attachment 181423 [details] [diff] [review]
Tweaked patch v0.3a (Checked in)

Checking in content/pref-navigator.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-navigator.xu
l,v  <--  pref-navigator.xul
new revision: 1.74; previous revision: 1.73
done
Checking in locale/en-US/pref-navigator.dtd;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/locale/en-US/pref-navigat
or.dtd,v  <--  pref-navigator.dtd
new revision: 1.26; previous revision: 1.25
done

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050422