432599 - Double-click on the Star icon leads to incorrect display of the bookmark properties panel

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(no longer active)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050610 Minefield/3.0pre

Steps to reproduce:
1. Load any new page.
2. If it's not starred previously, click the Star icon to make it starred.
3. Double-click the star icon.

Expected result:
The "Edit This Bookmark" dialog should be displayed, allowing the user to edit or remove the bookmark.

Actual result:
The "Edit This Bookmark" dialog appears on the first click, and the "Page Bookmarked" dialog appears on the second click (if you double-click fast enough, then the "Edit This Bookmark" dialog won't be visible at all; but if double-click slowly, both dialogs appear on the screen).

Since the "Page Bookmarked" dialog should be used for bookmarking a page via Ctrl+D, it shouldn't appear on double-click.  The behavior of the star icon should be the same on both click and other click.

Further notes:
The same problem happens with pressing Ctrl+D twice, without dismissing the first dialog.  Here is a simple STR:

1. Load any new page.
2. If it's not starred previously, click the Star icon to make it starred.
3. Press Ctrl+D twice.

Comment 1

[(no longer active)]

The first call to StarUI._doShowEditBookmarkPanel begins the batch mode, and the second call sees that we're in batch mode, and thinks it's a new bookmark to add, hence the incorrect display of icons.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

A simple patch with a unit test to make sure this doesn't regress in the future.  The change to browser-places.js is quite simple.  To make this more observable, I'll attach the output of diff -w on that file as well.

Comment 3

[(no longer active)]

Attachment: Patch (v1) (diff -w)

The differences without regard to white-space.

Comment 4

[(no longer active)]

I wrote an automated test for this bug, so this shouldn't be in Litmus, but in the test suite.

Comment 5

[Tracy Walker [:tracy]]

The "Edit This Bookmark" dialog is part of test case 5024.

The expected result of steps 2 and 3 are not quite as you reported. Once the page has been bookmarked, all that is needed to open the Edit This Bookmark dialog is a single click, another click, by design, should close the dialog. 

I can reproduce the third click on the star "Page Bookmarked" bug on Windows, but not on Mac.

Comment 6

[(no longer active)]

(In reply to comment #5)
> The "Edit This Bookmark" dialog is part of test case 5024.
> 
> The expected result of steps 2 and 3 are not quite as you reported. Once the
> page has been bookmarked, all that is needed to open the Edit This Bookmark
> dialog is a single click, another click, by design, should close the dialog. 

Yes, I was talking about a double-click.  IMHO a double-click should not open and immediately close the dialog, because some users may think they should double-click the star button and would become frustrated with that behavior.  Second click after some time should close the dialog (the behavior you mention) and it does both before and after my patch.

> I can reproduce the third click on the star "Page Bookmarked" bug on Windows,
> but not on Mac.

Hmmm, I don't have a Mac so I can't test it for myself, but there is nothing platform specific on the code I touched, AFAICT.

Comment 7

[Tim Riley [:timr]]

(In reply to comment #5) 
> 
> I can reproduce the third click on the star "Page Bookmarked" bug on Windows,
> but not on Mac.
> 

I also can't reproduce this on MacOS
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008052104 Minefield/3.0pre

Comment 8

[Tony Chung [:tchung]]

based on comment 7, it sounds like its windows only.  changing OS.

Comment 9

[(no longer active)]

Getting this on the radar for 3.0.1.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

It's on the radar for branch releases. What you really want is to get this reviewed and nominated.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 319780 [details] [diff] [review]
Patch (v1)

r=mano

Comment 12

[(no longer active)]

Comment on attachment 319780 [details] [diff] [review]
Patch (v1)

This bug is wanted on branch, and this patch has an automated test.  Requesting approval for it on 3.0.2.

Comment 13

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

Not ready for check-in. Needs approval first. Ehsan, shouldn't that bug be All/All?

Comment 14

[(no longer active)]

(In reply to comment #13)
> Not ready for check-in. Needs approval first.

Actually I added the checkin-needed keyword for checking this on trunk.  Branch landing will be tracked by the pending approval1.9.0.2? flag.

>Ehsan, shouldn't that bug be All/All?

Code-wise, there is nothing platform specific about the code or the patch, but it seems like this can't be reproduced on Mac.  I don't have a Mac so I can't say for sure, but please take a look at comment 7, for example.

Comment 16

[Asaf Romano (gone)]

changesets: d811aa4cf0cf, 96ca77c43cb9.

Comment 17

[Daniel Holbert [:dholbert]]

All of the unittest Tinderboxen have been orange since this landed, with 1 "browser" test failure.
   browser
   356/1/3

Error message is:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_425884.js | Exception thrown - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITransactionManager.undoTransaction]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///D:/builds/slave/trunk_win2k3-01/build/objdir/dist/bin/components/nsPlacesTransactionsService.js :: undoTransaction :: line 195"  data: no]

Sample log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1217362462.1217368153.13626.gz

This should be backed out or fixed...

Comment 18

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Backed out: http://hg.mozilla.org/mozilla-central/index.cgi/rev/cab5a570f46e

Comment 19

[Samuel Sidler (old account; do not CC)]

Comment on attachment 319780 [details] [diff] [review]
Patch (v1)

This patch was backed out. Please request approval on a safe, landed patch after it's baked.

Comment 20

[(no longer active)]

Attachment: Patch (v2)

A better fix for the problem.  The test is nearly written from scratch, as the previous test was too simplistic and in fact directly caused the other test to fail (see comment 17).  This new test does play nicely with the rest of the browser chrome tests.

Comment 22

[Samuel Sidler (old account; do not CC)]

Not blocking 1.9.0.2 as it needs to get on trunk first.

Comment 23

[(no longer active)]

Mano: ping...

Comment 24

[Asaf Romano (gone)]

Comment on attachment 332732 [details] [diff] [review]
Patch (v2)

hrm, why don't we bail out of this method vey early if the panel isn't closed?

Comment 25

[(no longer active)]

(In reply to comment #24)
> (From update of attachment 332732 [details] [diff] [review])
> hrm, why don't we bail out of this method vey early if the panel isn't closed?

That was the first solution I thought of, but I noticed that the existing code handles the cases where the panel is already open, so I thought maybe some other functionality might break if we bail out early.  See <http://hg.mozilla.org/mozilla-central/annotate/1d58ae237bdc/browser/base/content/browser-places.js#l214>.

Comment 26

[Asaf Romano (gone)]

That code dates back to the time when there was a simple "Page Bookmarked" notification for the first click. Now that it's gone, we should clean that up.

Comment 27

[(no longer active)]

Attachment: Patch (v3)

Bail out early...

Comment 28

[(no longer active)]

Comment on attachment 337438 [details] [diff] [review]
Patch (v3)

I set the review flag to [e-mail] which seems to have changed... :-)

Comment 29

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

Mano, could you review Ehsan's patch?

Comment 30

[Asaf Romano (gone)]

Comment on attachment 337438 [details] [diff] [review]
Patch (v3)

r=mano

Comment 31

[(no longer active)]

Landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/aa2751667595>

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 337438 [details] [diff] [review]
Patch (v3)

asking approval for this UI polish fix, low risk, comes with test

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 337438 [details] [diff] [review]
Patch (v3)

a191=beltzner

Comment 34

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cc69d6120485

Comment 35

[Charles Fenwick]

Verified.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre

Comment 36

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv