Closed Bug 432599 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

All
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: ehsan, Assigned: ehsan)

References

()

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 3 obsolete files)

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.
Flags: in-litmus?
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.
Assignee: nobody → ehsan.akhgari
OS: Windows Vista → All
Hardware: PC → All
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #319780 - Flags: review?(mano)
Attached patch Patch (v1) (diff -w) (obsolete) — Splinter Review
The differences without regard to white-space.
I wrote an automated test for this bug, so this shouldn't be in Litmus, but in the test suite.
Flags: in-litmus? → in-testsuite?
Whiteboard: [has patch][has unit test][needs review mano]
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.
(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.
Whiteboard: [has patch][has unit test][needs review mano] → [has patch][has unit test][needs review mano][RC2?]
(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
based on comment 7, it sounds like its windows only.  changing OS.
OS: All → Windows XP
Flags: wanted1.9.0.x+
Whiteboard: [has patch][has unit test][needs review mano][RC2?] → [has patch][has unit test][needs review mano][RC2-]
Getting this on the radar for 3.0.1.
Flags: blocking1.9.0.1?
It's on the radar for branch releases. What you really want is to get this reviewed and nominated.
Flags: blocking1.9.0.1? → blocking1.9.0.1-
Keywords: checkin-needed
Whiteboard: [has patch][has unit test][needs review mano][RC2-] → [RC2-]
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.
Attachment #319780 - Flags: approval1.9.0.2?
Not ready for check-in. Needs approval first. Ehsan, shouldn't that bug be All/All?
Keywords: checkin-needed
(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.
Keywords: checkin-needed
Duplicate of this bug: 415932
changesets: d811aa4cf0cf, 96ca77c43cb9.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: blocking1.9.0.2?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [RC2-]
Target Milestone: --- → Firefox 3.1a2
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #319780 - Flags: approval1.9.0.2?
Blocks: 448533
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #319780 - Attachment is obsolete: true
Attachment #319781 - Attachment is obsolete: true
Attachment #332732 - Flags: review?(mano)
Duplicate of this bug: 448533
Not blocking 1.9.0.2 as it needs to get on trunk first.
Flags: blocking1.9.0.2? → blocking1.9.0.2-
Mano: ping...
Status: REOPENED → ASSIGNED
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?
(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>.
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.
Attached patch Patch (v3)Splinter Review
Bail out early...
Attachment #332732 - Attachment is obsolete: true
Attachment #337438 - Flags: review?
Comment on attachment 337438 [details] [diff] [review]
Patch (v3)

I set the review flag to mano@mozilla.com which seems to have changed... :-)
Attachment #337438 - Flags: review? → review?(c3m3t3ry)
Mano, could you review Ehsan's patch?
Whiteboard: [needs review mano]
Target Milestone: Firefox 3.1a2 → ---
Landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/aa2751667595>
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review mano]
Target Milestone: --- → Firefox 3.2a1
Comment on attachment 337438 [details] [diff] [review]
Patch (v3)

asking approval for this UI polish fix, low risk, comes with test
Attachment #337438 - Flags: approval1.9.1?
Attachment #337438 - Flags: approval1.9.1? → approval1.9.1+
Verified.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.