Closed Bug 871322 Opened 7 years ago Closed 7 years ago

Customize... isn't accessible on second time when Bookmarks button is removed

Categories

(Firefox :: Bookmarks & History, defect, major)

23 Branch
x86_64
Windows 7
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox22 --- unaffected
firefox23 + verified
firefox24 --- verified

People

(Reporter: Virtual, Assigned: mconley)

References

Details

(Keywords: nightly-community, regression)

Attachments

(1 file, 1 obsolete file)

Like it title. After backing out patches from bug #748894 in bug #867343 "Customize.." in toolbar context menu isn't accessible, because it's greyed out and browser window looks like in window mode, not like it should be in maximized state. It happens when you remove Bookmarks button from toolbar.
Also Error Console shows this error

Error: TypeError: this.button is null
Source File: chrome://browser/content/browser.js
Line: 2931
Blocks: 867343
Keywords: regression
This is also breaking things that use the "aftercustomization" event to rebuild themselves after toolbar customization (i.e. the download button panel).
Steps to repeat:

1. Create new profile.
2. Open toolbar "Customize..." window (toolbar context menu).
3. Remove Bookmark toolbar button from toolbar.
4. Click "Done".
5. Attempt to open toolbar "Customize..." window.

Expected result:
The "Customize..." window opens.

Actual result:
"Customize..." is greyed out/disabled.

Also, "Toolbar layout..." is also disabled.

Closing/restarting the browser re-enables "Customize...".

Keeping the Bookmark button on the toolbar does not trigger the bug nor does it appear that other customizations trigger this bug.
Assignee: nobody → mconley
It looks like we're attempting to manipulate this._button after it's been deleted in BookmarkingUI.customizeDone. Patch coming.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch Patch v1 (obsolete) — Splinter Review
The original bookmark star button did a check for this.star, and returned early if it didn't exist. Because we don't do that check anymore, we exposed ourselves to potentially using this._button *after* it'd been deleted.
Attachment #748652 - Flags: review?(mak77)
Wouldn't that still have a problem if the button was never on the toolbar? this._button would always be null in that case, and customizeChange/customizeDone are always called unchecked. Checking this.button in _updateToolbarStyle() seems like the safest route.

Also, IMHO, deleting this._button first in customizeDone makes sense to have a fresh reference to the button. I'd say the same should also go for customizeChange. If the button is removed, then immediately added back, is it the same element?
Comment on attachment 748652 [details] [diff] [review]
Patch v1

Review of attachment 748652 [details] [diff] [review]:
-----------------------------------------------------------------

the reason to delete this._button is that the original element has been recreated, so the getter must acquire a new handle.
I think you should rather reintroduce an early return if (!this.button) in _updateToolbarStyle
Attachment #748652 - Flags: review?(mak77) → review-
Attached patch Patch v2Splinter Review
Yep, you're right, this is the right approach.
Attachment #748652 - Attachment is obsolete: true
Attachment #748821 - Flags: review?(mak77)
Attachment #748821 - Flags: review?(mak77) → review+
Comment on attachment 748821 [details] [diff] [review]
Patch v2

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 867343

User impact if declined: If the user moves the bookmarks menu button from their toolbar into the palette, they will be unable to re-enter customization mode until a browser restart. If the user already has the bookmarks menu button in their palette, they will only be able to enter customization mode once until a browser restart.

Testing completed (on m-c, etc.): All on m-c

Risk to taking this patch (and alternatives if risky): Extremely little risk.

String or IDL/UUID changes made by this patch: None.
Attachment #748821 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/249c9abe3e2b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Attachment #748821 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

Verified as fixed on Firefox 23 beta 9 (buildID: 20130725195523) and latest Nightly (buildID: 20130725171558).
Verified as fixed on Firefox 24 beta 2:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
Build ID: 20130812173056
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.