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)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3.6a1
People
(Reporter: beltzner, Assigned: asaf)
References
(Depends on 1 open bug, )
Details
Attachments
(1 file, 1 obsolete file)
4.53 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•16 years ago
|
||
<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
Reporter | ||
Updated•16 years ago
|
Comment 2•16 years ago
|
||
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.
Reporter | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #364413 -
Flags: review?(dietrich)
Comment 5•16 years ago
|
||
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+
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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.
Comment 8•16 years ago
|
||
+ var prefs = Cc["@mozilla.org/preferences-service;1"].
+ getService(Ci.nsIPrefBranch);
this needs formatting (the getService) in both places
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #364413 -
Attachment is obsolete: true
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 364433 [details] [diff] [review]
for checkin
mozilla-central: 23aa9ede6535
Attachment #364433 -
Flags: approval1.9.1?
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Reporter | ||
Comment 12•16 years ago
|
||
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+
Comment 13•16 years ago
|
||
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.
Comment 14•16 years ago
|
||
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.
Comment 15•16 years ago
|
||
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.
Comment 17•16 years ago
|
||
Comment on attachment 364433 [details] [diff] [review]
for checkin
can be renominated when bug 489972 is ready
Attachment #364433 -
Flags: approval1.9.1+
Reporter | ||
Comment 18•16 years ago
|
||
(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!
Updated•16 years ago
|
Hardware: x86 → All
Reporter | ||
Comment 19•16 years ago
|
||
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?
Comment 20•16 years ago
|
||
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.
Comment 21•15 years ago
|
||
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
Comment 23•9 years ago
|
||
This did work once, but now the focus is on the title field again every time.
FF 45.0.1 on arch linux
Comment 24•9 years ago
|
||
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
Comment 25•9 years ago
|
||
please file a separate bug?
You need to log in
before you can comment on or make changes to this bug.
Description
•