Closed Bug 599325 Opened 9 years ago Closed 9 years ago

Addon bar should be shown when customizing

Categories

(Firefox :: General, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: NicolasWeb, Assigned: dietrich)

References

Details

(Whiteboard: [addon bar])

Attachments

(3 files, 3 obsolete files)

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...
Blocks: 574688
Attached image Actual behavior
Attached image Expected behavior
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.
Doesn't show for me on linux either.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Can someone confirm that it's shown (or not) on OSX too ?
Severity: normal → major
Version: unspecified → Trunk
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...
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... ;-)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Attachment #479787 - Flags: review?(mano)
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.
Attachment #479787 - Flags: review?(mano) → review+
Attached patch patch with test (obsolete) — Splinter Review
you shoulda r-'d, i didn't have a test! but now i do, so asking for review again :)
Attachment #479787 - Attachment is obsolete: true
Attachment #483473 - Flags: review?(mano)
Attached patch patch with test (obsolete) — Splinter Review
and really with the test this time.
Attachment #483473 - Attachment is obsolete: true
Attachment #483474 - Flags: review?(mano)
Attachment #483473 - Flags: review?(mano)
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.
(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.
Attached patch patch with testSplinter Review
Attachment #483474 - Attachment is obsolete: true
Attachment #485998 - Flags: review?(mano)
Attachment #483474 - Flags: review?(mano)
Requesting blocking. Drag-and-drop customization of the add-on bar is not possible if the bar itself is not visible.
blocking2.0: --- → ?
blocking2.0: ? → final+
Whiteboard: [has patch][can land]
Whiteboard: [has patch][can land] → [has patch][can land][addon bar]
http://hg.mozilla.org/mozilla-central/rev/2bcf34fc37a7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][can land][addon bar] → [addon bar]
> >+  if (addonBar.previousCollapsedState === true) {
> 
> Either way, remove " == true".

Missed this?
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.
Depends on: 612588
(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
You need to log in before you can comment on or make changes to this bug.