171303 - [cust] Creating a new toolbar with same name as standard toolbar should give an error

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P4)

Description

[David McCormack]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20020927 Phoenix/0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20020927 Phoenix/0.2

Create a new toolbar called 'Bookmark Toolbar'. No error is reported that a
toolbar with that name already exists and a new toolbar and menu item are created.


Reproducible: Always

Steps to Reproduce:


Actual Results:  
Phoenix creates the toolbar and you end up with duplicate menu items on the
Toolbars menu.

Expected Results:  
If you try and create a second user generated toolbar with the same name as a
previous user generated toolbar an error is reported and you are prompted to
change the name. This should also happen with the 'Status Bar', 'Navigation
Toolbar' and 'Bookmarks Toolbar' names.


The new toolbar facilities are excellent, it means an end to having alter
browser.xul to get an 'Address toolbar'.

Comment 1

[Asa Dotzler [:asa]]

confirmed. not sure what we want to do about this. over to hewitt to decide. 

Comment 2

[Yuting Kuo]

Also, the default 2 toolbar names can not be changed. Try this:

1. Customize toolbar
2. Move all the buttons/address bar/search bar onto the Menu Bar, next to "Help"
3. So now the "Navigation Bar" is completely empty.

ACTUAL results:
The empty "Navigation Bar" is still called "Navigation Bar". This is confusing
if you want to check them on/off in View->ToolBars. Same goes for "Bookmark Bar".

Solutions:

* Allow renaming of default toolbars.

* "Grips" in toolbars (Bug #172818) so you can move the whole Navigation Bar and
won't have this issue.

* Empty rows should not be allowed, if the 2nd row is empty, the 3rd row should
move up automatically.

Comment 3

[Henrik Lynggaard Hansen]

Attachment: Now also blocks Bookmark/Navigation toolbar.

This pacth also blocks new toolbar by their display name, taken from the list
used for the View -> Toolbars menu.

It also fixes stripping the spaces from the id's generated. It now strips all
spaces insteda of just the first one.

I suggest checking in this patch (if it passes review) and leave the other
issues to more relevant bugs.

Comment 4

[Blake Ross]

Hey Henrik, thanks for that patch.  Question: why do we still need the whole
nameToId check?  Custom toolbars have a toolbarname also.  It seems like the new
check you've added will be sufficient.

Comment 5

[Henrik Lynggaard Hansen]

HI blake

In though about removeing it since it seems redundant, but on second thought I
think we do need to 

The reason is that two toolbars with different display names may generate the
same id.
All these toolbar names (without the quotes) make for the same id
"My Toolbar"
"My    Toolbar"
"   My Toolbar"
"My Toolbar "

I see two options:

1: Leave the patch as is with the double check.
2: Change the nameToId conversion to use underscorer for spaces, instead of
stripping them.

I you like #2 best, I thinkI can quickly change the patch.

Comment 6

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 7

[Henrik Lynggaard Hansen]

Attachment: Get rid og the double cheking and alter toolbar.xml to generate uniqe ids.

This patch is an update, it has two changes:
- the extra dupe check has been removed so we now only look at the display
names.
- toolkit/content/wedget/toolbar.xml has been altered to generate uniqe ids for
custom toolbars, needed for the first change to work

Comment 8

[Wayne Woods]

Does this patch also prevent the creation of toolbars with no name? If you guys
agree that you shouldn't be able to do that, a fix at the same time would be
handy, per bug 214707. Thanks :)

Comment 9

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

I'm giving this one to blake, as he's more likely to review and check this in
than hewitt is. 

Comment 10

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

-> 0.9

Comment 11

[Mike Connor [:mconnor]]

not a blocker for 0.9, maybe 1.0

Comment 12

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

+ing since this has a patch. 

Comment 13

[Blake Ross]

Fixed.

Comment 14

[djk]

Note that my patch to bug 180164 will give the 'menu bar' a displayable name, so
the type check in the following line will have to go.

+            if (toolbarName==name.value && type != "menubar") {

Thanks,
David

Comment 15

[Wayne Woods]

On what build(s) is this fixed? I can still create a second "Bookmarks Toolbar"
on the 20040715 nightly build on the Mac, so the bug still seems to exist. I
can't check Windows nightlies, since they're still down.

Comment 16

[Wayne Woods]

I apologize. It seems that the original intent of this report was to make it so
that two toolbars with the same name can't exist, but it's been changed to allow
duplicate names, but assign each a unique id for the code to handle. So I
presume it is fixed afterall, even though I can create a second "Bookmarks
Toolbar" in the latest Win XP nightly.