221482 - Bookmarks toolbar temporarily cleared when clicking "Restore Default Set" in customize mode

Status: RESOLVED FIXED

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

Description

[Jon Henry]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20031006 Firebird/0.7+ (aebrahim)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20031006 Firebird/0.7+ (aebrahim)

When customizing the toolbars, clicking the Restore Default Set button will
result in the bookmarks toolbar being empty until a new window is opened or
Firebird is restarted. The bookmarks are not actually lost, but they cannot be
seen or clicked.

Reproducible: Always

Steps to Reproduce:
1. Right click a toolbar
2. Customize...
3. Restore Default Set

Actual Results:  
Bookmarks menu is cleared and appears/acts empty.

Expected Results:  
The bookmarks should be properly rendered and clickable.

I realize this is an unofficial build, I'm only using it because there have been
no official Win32 builds in recent days.

Comment 1

[Andrew Smith]

Setting OS to All because I see this under Linux. I don't get it with 0.7 so
it's a fairly recent regression.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a)
Gecko/20040128 Firebird/0.8.0+

Comment 3

[miahz]

i believe i'm seeing the same thing in the 0.8 release when i drag the bookmark
toolbar items into the palette, and back onto the toolbar.  exiting customize
mode reveals a blank bookmarks bar.

Comment 4

[Andrew Smith]

I can confirm this behaviour with 0.8 too.

Comment 5

[Andrew Smith]

See also bug 232707.

Comment 6

[Evan Hamrick]

Confirmed in WinXP SP1 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040314
Firefox/0.8.0+

Comment 7

[Hussam Al-Tayeb]

Any ideas what broke this? Maybe there's something that might help get a testcase.

Comment 8

[Joshua W.]

I'm running into this problem with the 0.9 release candidate [Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608 Firefox/0.8]. It's
nothing major, but it's certainly a pain in the butt when I restore the defaults
then try a bunch of things to try and recover the bookmarks, only to discover
all I have to do is close the window and reopen.

Comment 9

[mmortal03]

This bug also grays the back and foward buttons so data loss occurs.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch for incorrect back/forward button state

Attached is a patch for fixing the incorrect state of the back/forward (and
potentially other) buttons; when the default toolbar is restored, the saved
state for each item -- whether it's disabled or not, also any attached command
-- was being clobbered.  Probably a better way to save the state than this;
suggestions appreciated.  I'm not having much luck tracking down the bookmarks
toolbar issue, though; the builder ain't building, for some reason.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: personal bookmarks toolbar resync patch

This patch fixes the bookmarks folder clearing.  The personal toolbar template
depends on its ref being set to the actual rdf resource of the folder; this
happens when the bookmarks are first loaded in delayedStartup(), but needs to
happen again when the toolbar changes.

Comment 12

[Jon Henry]

Cool! Vladimir, you should request a review from Ben or mconner, so this can
actually get checked in. I think your patch for the incorrect button state is
what was blocking me from finishing up bug 215838, so I definitely appreciate
this work.

Comment 13

[Adam Hauner]

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

Comment 14

[Mike Connor [:mconnor]]

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

Comment 15

[Markus Lindström]

It's odd, the bookmarks toolbar does vanish, but only the "Back" button gets
greyed out "forever". The cache DOESN'T get cleared though! You can still move
backwards using the context menu on the page.

The forward button still works, though!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040623
Firefox/0.9.0+

Comment 16

[Hussam Al-Tayeb]

Is there any chance we can get this patch checked in for firefox 0.9.1 ?

Comment 17

[Hussam Al-Tayeb]

Sorry for the spam, but since the mentioned patch also fixes bug 232707, can a
developer please look at the patch? 
Again, sorry for the spam.

Comment 18

[Mike Shaver (:shaver emeritus)]

Comment on attachment 151012 [details] [diff] [review]
personal bookmarks toolbar resync patch

Looks good.

Comment 19

[Mike Shaver (:shaver emeritus)]

Comment on attachment 150921 [details] [diff] [review]
patch for incorrect back/forward button state

You should unify the save and restore functions to take an attribute-name
parameter, since they're virtually identical (other than the |true|
specialization for disabledItems).

Comment 20

[Jon Henry]

Vlaidimr, regarding the disabled buttons issue, I might have found a simpler
solution. I just did a quick test, and it looks like my patch for bug 215838
fixes the problem and is much shorter. You might give that a try, and if it
works your other patch can probably be checked in and this bug resolved fixed.

Basically, the code for dealing with the disabled state on a button assumed the
"disabled" attribute existed, which as it turns out isn't a safe assumption.
Using setAttribute / getAttribute rather than trying to access the value
directly lead to the state not getting clobbered anymore.

Comment 21

[Jon Henry]

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

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch for incorrect back/forward button state v2

Refactoring FTW.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #20)

The patch from bug 215838 doesn't fix the issue for me... tested with opening
ffox, clicking on a link from the start page, selecting customize toolbar,
clicked restore default, and closed the dialog box.  Back button remained
disabled, even though it should have been enabled.  The
getAttribute/setAttribute's should be functionally identical to the current
code, no?

Comment 24

[Jon Henry]

That's strange, that same process works fine for me (on WinXP). I thought the
get/set logic was the same as the previous logic too, which is why it took me so
long to find. I was watching the buttons in the DOM inspector and sometimes they
had no disabled attribute at all, which screwed things up. getAttribute doesn't
throw a fit when an attribute doesn't exist, and now I haven't had any problems
with the buttons randomly becoming disabled.

But if your patch is what it takes to get things to work on all systems, great,
cause it's an annoying bug :).

Comment 25

[Mike Shaver (:shaver emeritus)]

Comment on attachment 151847 [details] [diff] [review]
patch for incorrect back/forward button state v2

Bingo. r=shaver, please land on the aviary branch.

Comment 26

[Hussam Al-Tayeb]

nominating blocking-aviary1.0RC1 since there is a patch.

Comment 27

[Hussam Al-Tayeb]

Since the patch was approved, any ideas if it will land anytime on the aviary
branch anytime soon?

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in on aviary.

Comment 29

[PikeUK]

I think this caused Bug 250179 (and while I'm commenting, I think it *fixed* Bug
237866).

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 31

[Marco Iannaccone]

Same problem with version 1.0.3 of Firefox.