Closed Bug 427039 Opened 17 years ago Closed 17 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: fittysix, Assigned: fittysix)

Details

Attachments

(1 file, 2 obsolete files)

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.
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040402 Minefield/3.0pre
Status: UNCONFIRMED → NEW
Component: Bookmarks → Places
Ever confirmed: true
Flags: blocking-firefox3?
QA Contact: bookmarks → places
Version: unspecified → Trunk
Attached patch Patch (obsolete) (deleted) — Splinter Review
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.
Attachment #313745 - Flags: ui-review?
Attachment #313745 - Flags: ui-review? → ui-review?(beltzner)
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
Attachment #313745 - Flags: ui-review?(beltzner) → ui-review-
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.
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?
Whiteboard: FFT
Attached patch Revert URL Bar on bookmark Patch v1 (obsolete) (deleted) — Splinter Review
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?
Attachment #314231 - Flags: ui-review?(beltzner)
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 on attachment 314231 [details] [diff] [review] Revert URL Bar on bookmark Patch v1 Agree with mconnor, no need to keep the half-written URL.
Attachment #314231 - Flags: ui-review?(beltzner) → ui-review+
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.
Attachment #314231 - Flags: review+
Assignee: nobody → fittysix
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: checkin-needed
Whiteboard: FFT → [has patch][has reviews] FFT
Attached patch unbitrotted patch (deleted) — Splinter Review
I removed the reference to the bug in the comment, I think blame is sufficient.
Attachment #313745 - Attachment is obsolete: true
Attachment #314231 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews] FFT
Target Milestone: --- → Firefox 3
Comment on attachment 315434 [details] [diff] [review] unbitrotted patch requesting post-hoc approval.
Attachment #315434 - Flags: approval1.9?
Attachment #315434 - Flags: approval1.9? → approval1.9+
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
This is an edge case that really goes outside the scope of our FFT's...setting the in-litmus- flag.
Flags: in-litmus? → in-litmus-
I'm not sure this is such an edge case, we blocked on this for Firefox 3...
Flags: in-litmus- → in-litmus?
Aakash, the specific placement of the dialog can be added to expected results in the existing test case.
The test case https://litmus.mozilla.org/show_test.cgi?id=5954 was updated on litmus for regression testing this bug.
Flags: in-litmus? → in-litmus+
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.

Attachment

General

Creator:
Created:
Updated:
Size: