427039 - New Bookmark / StarUI pops up in the wrong place if the star is not on the location bar

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ryan A. C.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre

When you type something in to the location bar the star disappears, and the go button shows up in its place. If you try to bookmark something afterwords the starui will show at the wrong place.

Reproducible: Always

Steps to Reproduce:
1. type something in the location bar
2. go to Bookmarks > Bookmark this page
Actual Results:  
The starui popup will show on the left side of the content area.

Expected Results:  
For visual consistency the starui should show at the end of the location bar.

The openPopup anchor is obviously the star, which is almost always there, but we could check if the go button is visible and set the anchor to that when it is.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040402 Minefield/3.0pre

Comment 2

[Ryan A. C.]

Attachment: Patch

Pops the Page Bookmark out of the go button when the star-button is not visible and the go button is visible. Tested with go button always visible userChrome mod with no problems. The original comment and code looks like content area was intentional, but with the go button we can also get the same location.

Comment 3

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

Comment on attachment 313745 [details] [diff] [review]
Patch

When the go button is showing, the content in the location bar is "dirty", and the URL shown in is *not* the URL that will be bookmarked.

I think the better solution in this case would be to:
 - undo the edit to the location bar
 - this will remove the Go button and return the Star
 - have the drop-down appear out of the star as normal

Comment 4

[Ryan A. C.]

The URL showing part is actually why I asked for ui-review, I kind of thought the same thing. That being said I don't entirely like reverting the user input since:
1. user inputs url
2. user decides to bookmark page before leaving
3. user has to re-type url

Though, since there is no indication of URL on the starui I do think reverting it is probably the better behavior, since even in the current iteration there is no indication of the URL being bookmarked.
It would be possible to save the location bar contents, and put them back after bookmarking of course, but I think that's a bit more complex of a solution than we might want to do for this, since this is a fairly edge-case problem.

Comment 5

[Tracy Walker [:tracy]]

Clobbering the users input to make the Star appear seems clunky to me.  Is it possible to do what Ryan originally suggested; anchor the Page Bookmarked dialog to the Go button if the star is not present?  

Comment 6

[Ryan A. C.]

Attachment: Revert URL Bar on bookmark Patch v1

This one is pretty simple, it just reverts the URL bar before ever attempting to show the popup.

Beltzner: should I save urlbar contents and re-set after the bookmark is done?

Comment 7

[Mike Connor [:mconnor]]

I think this is the right fix.  Here's why:

1) The user is entering a URL to navigate away from the page (or do another page)
2) They decide to interrupt themselves and bookmark the current page

We should have the UI reflect that interruption, not do something clunky like associate the bookmark with the go button.

I don't even think we need to revert back, I think the user has stopped doing what they were doing in 1 and chosen another task.  Anything else is pure guesswork about user intent after bookmarking.

Comment 8

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

Comment on attachment 314231 [details] [diff] [review]
Revert URL Bar on bookmark Patch v1

Agree with mconnor, no need to keep the half-written URL.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 314231 [details] [diff] [review]
Revert URL Bar on bookmark Patch v1

r=mconnor

this always reverts the URL when the star is clicked, but that's ok.

as a note, when you click the site button (Larry) we revert there as well.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: unbitrotted patch

I removed the reference to the bug in the comment, I think blame is sufficient.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I checked this in pending approval, since it seems pretty clear that we want to take this.

mozilla/browser/base/content/browser-places.js 	1.126 

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 315434 [details] [diff] [review]
unbitrotted patch

requesting post-hoc approval.

Comment 13

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

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041404 Minefield/3.0pre ID:2008041404

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041406 Minefield/3.0pre ID:2008041406

Comment 14

[Aakash Desai [:aakashd]]

This is an edge case that really goes outside the scope of our FFT's...setting the in-litmus- flag.

Comment 15

[Mike Connor [:mconnor]]

I'm not sure this is such an edge case, we blocked on this for Firefox 3...

Comment 16

[Tracy Walker [:tracy]]

Aakash, the specific placement of the dialog can be added to expected results in the existing test case.

Comment 17

[Aakash Desai [:aakashd]]

The test case https://litmus.mozilla.org/show_test.cgi?id=5954 was updated on litmus for regression testing this bug.

Comment 18

[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