Closed Bug 480151 Opened 16 years ago Closed 16 years ago

remember the last field modified first in the Add Bookmark dialog and start with focus there next time

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: beltzner, Assigned: asaf)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

Some users change titles a lot. Other users change tags a lot. Right now we default the focus in the Add Bookmark dialog to the title field, which is annoying for users who are heavy taggers and want to quickly tag a new bookmark. I could see how bookmarkers who change titles a lot would be similarly annoyed if we simply changed the default focus. Instead the proposal of this bug (grace aux Shaver, Mano and others who were chatting about it) is that we should: - keep track of which field (tags or folders) the user modified most recently - start with focus in that field the next time the user adds a bookmark
<Mano> i would say the first field which was modified <Mano> i.e. if the title was modified, then tags <Mano> we shouldn't remember "tags" imo <beltzner> that makes sense
Summary: remember the last field modified in the Add Bookmark dialog and start with focus there next time → remember the last field modified first in the Add Bookmark dialog and start with focus there next time
Won't this ruin UI predictability for keyboard users who don't consistently change make the same sort of changes? Currently I blindly know that typing right after Ctrl+ will change the title - with this change, Firefox might start behaving "randomly" and feel broken instead of fixed.
The patch, as proposed, would not break your behaviour at all. It would be adaptive to user behaviour in a way that would feel consistent. Good thing to be concerned about, though.
Attached patch Patch (obsolete) — Splinter Review
Attachment #364413 - Flags: review?(dietrich)
Comment on attachment 364413 [details] [diff] [review] Patch >diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js >--- a/browser/base/content/browser-places.js >+++ b/browser/base/content/browser-places.js >@@ -231,22 +231,34 @@ > { hiddenRows: ["description", "location", > "loadInSidebar", "keyword"] }); > }, > > panelShown: > function SU_panelShown(aEvent) { > if (aEvent.target == this.panel) { > if (!this._element("editBookmarkPanelContent").hidden) { >- var namePicker = this._element("editBMPanel_namePicker"); >- namePicker.focus(); >- namePicker.select(); >+ var fieldToFocus = "editBMPanel_namePicker"; >+ try { >+ var prefs = Cc["@mozilla.org/preferences-service;1"]. >+ getService(Ci.nsIPrefBranch); >+ fieldToFocus = "editBMPanel_" + >+ prefs.getCharPref("browser.bookmarks.editDialog.firstEditField"); >+ } >+ catch(ex) { /* not yet set, use default */ } i'd prefer to have the pref in firefox.js, with a default value, and documented. it'll end up documented on kb and mdc sooner or later, so best to just be explicit about it now. >+ >+ var elt = this._element(fieldToFocus); >+ elt.focus(); >+ elt.select(); > } >- else >+ else { >+ // Note this isn't actually used anymore, we should remove this >+ // once we decide not to bring back the page bookmarked notification > this.panel.focus(); >+ } should just remove it now. make a note on that bug, and we can add it back later if needed. >@@ -586,18 +591,35 @@ > if (txns.length > 0) { > var aggregate = PlacesUIUtils.ptm.aggregateTransactions("Update tags", > txns); > PlacesUIUtils.ptm.doTransaction(aggregate); > > // Ensure the tagsField is in sync, clean it up from empty tags > var tags = PlacesUtils.tagging.getTagsForURI(this._uri, {}).join(", "); > this._initTextField("tagsField", tags, false); >+ return true; > } > } >+ return false; >+ }, >+ >+ _mayUpdateFirstEditField: function EIO__mayUpdateFirstEditField(aNewField) { please add jsdoc to this function so that it's clear what it actually does. and/or rename it so that it's clearer. r=me
Attachment #364413 - Flags: review?(dietrich) → review+
I'm not removing that block now because there's much more cruft to remove from that dialog, and I don't think that's a safe thing to do this late in the game.
That pref is set implicitly as you use the dialog, thus having it in a "documented" place is meaningless - the user cannot actually alter the behavior.
+ var prefs = Cc["@mozilla.org/preferences-service;1"]. + getService(Ci.nsIPrefBranch); this needs formatting (the getService) in both places
Attached patch for checkinSplinter Review
Attachment #364413 - Attachment is obsolete: true
Comment on attachment 364433 [details] [diff] [review] for checkin mozilla-central: 23aa9ede6535
Attachment #364433 - Flags: approval1.9.1?
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
No test? This is an easy one too :(
Flags: in-testsuite?
Comment on attachment 364433 [details] [diff] [review] for checkin a=beltzner, but please also include a test when you checkin. You can do eeeet!
Attachment #364433 - Flags: approval1.9.1? → approval1.9.1+
I have a doubt on this patch now, you set the pref in editItemOverlay, but use it only in the StarUI. there's no risk that other users of editItemOverlay (bookmarks properties dialog and the Library) could change the setting so that: - in star UI you focus tags first so reopening star ui it will be focused - you add a livemark, the dialog uses editItemOverlay, and you change name first because you can't tag a livemark. Same would be if you bookmark all tabs, or right click and choose New bookmark (hardly you will go to tags before filing a name there) - next time you will open star UI it will focus name field even if you want it to focus tags field. I guess if this code should live completely in StarUI object instead, and use listeners.
Looks like this won't make 3.5. It's fixed on trunk though, so... (In reply to comment #13) > I have a doubt on this patch now ... this should probably be moved to a new bug.
Mano told me he wanted to provide a new patch for this, since actually this would not work as expected. So, i think it won't make 3.5. Right, that needs a new bug.
Depends on: 489972
Comment on attachment 364433 [details] [diff] [review] for checkin can be renominated when bug 489972 is ready
Attachment #364433 - Flags: approval1.9.1+
Blocks: 407366
(In reply to comment #13) > I have a doubt on this patch now, you set the pref in editItemOverlay, but use > it only in the StarUI. there's no risk that other users of editItemOverlay > (bookmarks properties dialog and the Library) could change the setting so that: > - in star UI you focus tags first so reopening star ui it will be focused > - you add a livemark, the dialog uses editItemOverlay, and you change name > first because you can't tag a livemark. Same would be if you bookmark all tabs, > or right click and choose New bookmark (hardly you will go to tags before > filing a name there) > - next time you will open star UI it will focus name field even if you want it > to focus tags field. I'm not sure that this issue should block us from making the change in Firefox 3.5, to be honest. Is that the only problem with this patch? If so, I'd say let's keep bug 489972 as a post-3.5 followup, and make the behaviour mostly-better for 3.5!
Hardware: x86 → All
Dietrich: would appreciate your input here. I think Mano agrees with me, though he's worried that there may be focus-loss issues, I think?
iiuc, with the patch as-is it will Do What You Want *most* of the time. and the rest of the time, it'll do whatever you did last. imo, that's shippable.
No longer blocks: 407366
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
This did work once, but now the focus is on the title field again every time. FF 45.0.1 on arch linux
I was used to have the focus on the Tags field, now the bookmark dialog is focusing the Name field every time I create a new bookmark. I got to "fix" this setting browser.bookmarks.editDialog.firstEditField to tagsField in about:config FF 47.0.1 - OS X 10.11.5
please file a separate bug?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: