599325 - Addon bar should be shown when customizing

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Nicolas Mandil (:NicolasWeb)]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100924 Firefox-4.0/4.0b7pre Ubuntu/10.04
Build Identifier: 

All tool bars are shown when customizing, except Addon bar. This prevent to make it discoverable and forbidden the user to drag addons that aren't installed by default in this toolbar.

Reproducible: Always

Steps to Reproduce:
1.Open menu View/Toolbars/Customize...

Comment 1

[Nicolas Mandil (:NicolasWeb)]

Attachment: Actual behavior

Comment 2

[Nicolas Mandil (:NicolasWeb)]

Attachment: Expected behavior

Comment 3

[Csaba Kozák [:WonderCsabo]]

It's shown when  customizing for me on Win7.

Are you tried with fresh profile. If yes, i guess it's a linux only bug.

Comment 4

[Dietrich Ayala (:dietrich)]

Doesn't show for me on linux either.

Comment 5

[Nicolas Mandil (:NicolasWeb)]

Can someone confirm that it's shown (or not) on OSX too ?

Comment 6

[pino_sb]

It doesn't show for me on Windows 7 actually. The steps to reproduce are wrong though, they should be:

1. Hide addon bar
2. Open customize menu
Now the addon bar should show, even though it is hidden by default, as other toolbars do.

So that might cause the confusion...

Comment 7

[Nicolas Mandil (:NicolasWeb)]

STR are not wrong, they are just Linux STR. On linux, when the bug was filled, the addon bar was hidden by default (no sure what it is now) ;-)
So, you gave STR for Windows... ;-)

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: patch

Comment 9

[Asaf Romano (gone)]

Comment on attachment 479787 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -3436,16 +3436,22 @@ function BrowserCustomizeToolbar()
>   if (splitter)
>     splitter.parentNode.removeChild(splitter);
> 
>   CombinedStopReload.uninit();
> 
>   PlacesToolbarHelper.customizeStart();
>   BookmarksMenuButton.customizeStart();
> 
>+  let addonBar = document.getElementById("addon-bar");
>+  if (addonBar.collapsed) {
>+    addonBar.previousCollapsedState = addonBar.collapsed;

I prefer  "wasCollapsed = true".

>+  if (addonBar.previousCollapsedState === true) {

Either way, remove " == true".

r=mano otherwise.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: patch with test

you shoulda r-'d, i didn't have a test! but now i do, so asking for review again :)

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: patch with test

and really with the test this time.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 483474 [details] [diff] [review]
patch with test


>+    // Can't use the property, since the binding may have since been removed
>+    // if the element is hidden (see bug 422590)
>+    is(addonBar.getAttribute("collapsed"), "true",

This doesn't make sense to me - the collapsed property is not part of any binding afaict.

Comment 13

[Dietrich Ayala (:dietrich)]

(In reply to comment #12)
> Comment on attachment 483474 [details] [diff] [review]
> patch with test
> 
> 
> >+    // Can't use the property, since the binding may have since been removed
> >+    // if the element is hidden (see bug 422590)
> >+    is(addonBar.getAttribute("collapsed"), "true",
> 
> This doesn't make sense to me - the collapsed property is not part of any
> binding afaict.

leftover from http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug422590.js#56, the test i based this one on.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: patch with test

Comment 15

[Asaf Romano (gone)]

Comment on attachment 485998 [details] [diff] [review]
patch with test

r=mano

Comment 16

[Dietrich Ayala (:dietrich)]

Requesting blocking. Drag-and-drop customization of the add-on bar is not possible if the bar itself is not visible.

Comment 17

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/2bcf34fc37a7

Comment 18

[Dão Gottwald [:dao]]

> >+  if (addonBar.previousCollapsedState === true) {
> 
> Either way, remove " == true".

Missed this?

Comment 19

[Dão Gottwald [:dao]]

The test is also bogus. All other toolbars are temporarily shown with CSS (xul.css). If we ever make this work for the add-on bar, which we should, the test will start failing.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> If we ever make this work for the add-on bar, which we should,

I filed bug 612588 on this