80494 - Extra vertical separator drawn when Bookmarks & Home button are not shown.

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P5)

Description

[doctor__j]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9+) Gecko/20010512
BuildID:    2001051208

Reproducible: Always
Steps to Reproduce:
1. Config Moz so that the Bookmarks and Home button is NOT shown in the Personal
toolbar.
2. Show the Personal toolbar and see the extra vertical separator drawn.

Expected Results:  No need to draw two vertical separators.

Comment 1

[doctor__j]

Attachment: Extra vertical separator

Comment 2

[timeless]

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

Comment 3

[doctor__j]

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

Comment 4

[Håkan Waara]

confirming bug.

Comment 5

[Håkan Waara]

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

Comment 6

[Joe Hewitt (gone)]

Not a themes bug, let's try XPApps.  Some js needs to mark an attribute for when
all icons in the left portion are hidden.

Comment 7

[Fabian Guisset]

My present for mpt :-)
Here's a patch that fixes this bug, I'm not sure whether this is the correct thing
to do, but it does work. CC'ing alecf since he wrote that function.
Any reviews or comments?
Thanks.

Comment 8

[Fabian Guisset]

Attachment: Fix bug 80494

Comment 9

[Alec Flett]

bzzzt. Wrong fix. 
There are other buttons that can appear in that toolbar in the commercial
version of this product. what you need to do is see if ALL the buttons (don't
hardcode "home") are hidden, then hide the separator.

Comment 10

[Viswanath Ramachandran]

-> ben

Comment 11

[Matthew T (active 1999-2002)]

alecf: How do you test for the visibility of buttons which don't exist? For 
every pref which hides/shows a particular UI element, should that element be 
placed inside a <box id="whatever"> and the pref hide/show that box instead, 
just in case some distributor somewhere decides to lump other elements in next 
to that element, controlled by the same pref (as in this case)? If so, what 
sort of performance impact would would adding all those boxes have?

Comment 12

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

I like the 'test for visibility of elts prior to separator' method that alec
suggests better than the box version, as it is likely to be simpler. 

Comment 13

[Cormac F]

Take a look at bug 85032 - we position the bookmark button permanently on the
far left of the personal toolbar (which makes sense - vendors can add other
buttons here, shifting the bookmarks about the place. Better to have it in a
permanent place). This would probably make this bug simpler - you'd only have to
check if the bookmarks button was there.

Comment 14

[Fabian Guisset]

I produced my patch knowing that the home and bookmarks buttons should be
swapped (see the bug mentioned above), however that is not really my problem,
and it's Ben's call whether we should swap them or not. I'll get you a more
generic patch tomorrow.

Comment 15

[Paul Chen]

nav triage team:

marking nsbeta1+

Comment 16

[Fabian Guisset]

Attachment: New attempt to patch bug 80494

Comment 17

[Fabian Guisset]

New patch posted. It can handle any number of buttons left of the separator. I
tested it by moving the bookmarks button left of the separator and it worked
fine. I added the persist="hidden" to the separator because the pref is not read
on startup. I wrote a new function for clarity, but I could as easily do
everything in nsButtonPrefListener.
Comments or reviews?
Thanks.

Comment 18

[Matthew T (active 1999-2002)]

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

Comment 19

[Stephen Walker]

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

Comment 20

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

sr=[e-mail] for this providing you add a call to hasAttribute("hidden")
to the if check in your checking function because a distributor may add buttons
which do not have the hidden attribute. 

Comment 21

[Fabian Guisset]

Attachment: Patch number 3, add hasAttribute check.

Comment 22

[Fabian Guisset]

Attachment: Patch#4, fix typo and forgot to diff navigator.xul

Comment 23

[Håkan Waara]

r=hwaara

Comment 24

[Håkan Waara]

Fix that comment's grammar first though.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 26

[Jeremy M. Dolan]

You do need a new profile before this will be fixed. I was wondering why I saw
the check in weeks ago and I still have the divider.

Verified.

Comment 27

[basic]

remove self