see the extension in the bug URL. this patch provides the same UI: basically a centered panel that contains a text input box for adding tags to the currently visible page. it's using accel+shift+c (categorize) for the keyboard shortcut.

old reference UI: http://people.mozilla.com/~faaborg/files/20070705-kui/i1kuiTagging.png_large.png should we use the tag icon instead of the star here?

Great idea, but please open the text box below the content of the page, like the find bar. Firefox should only hide content when a different window is opened.

Hey Dietrich, I've just introduced a "KUI-panel" class name in bug 395980 in order to share the styling. I'm going to use this in bug 436304 as well.

Comment on attachment 322007 [details] [diff] [review] added label Some drive-by notes on issues that I've already run into in my bugs... >+ openPopup: function TagKUI_openPopup() { >+ this.currentURI = getBrowser().selectedBrowser.currentURI; >+ var w = document.defaultView; >+ var x = (w.innerWidth/2) - (this.panel.width/2); >+ var y = w.screenY + 200; >+ LOG("XXX: " + x + ", window.innerWidth: " + w.innerWidth + ", panel width: " + this.panel.width); >+ x = (x > 0) ? x : 0; >+ y = (y > 0) ? y : 0; >+ LOG("XXX: " + x + ", window.innerWidth: " + w.innerWidth + ", panel width: " + this.panel.width); >+ this.panel.openPopupAtScreen(x, y, false); Looks like this will do odd things for non-maximized, arbitrarily positioned windows. >+ // focus and populate input >+ var self = this; >+ setTimeout(function() { >+ var uri = getBrowser().selectedBrowser.currentURI; >+ var tags = PlacesUtils.tagging.getTagsForURI(uri, {}); >+ if (tags.length > 0) >+ self.inputField.value = tags.join(", "); >+ self.inputField.select(); >+ self._open = true; >+ }, 0); This should be done in a popupshown listener.

This bug is on the nice to have features list. Asking for wanted Firefox 3.1

Some initial feedback on the UI: -we will need a larger tag icon, I'll look into getting this for you -Using KUI-panel is great, so we can alter the appearance for all of these based on the platform, and so that they are all visually consistent -The textfield should probably consist of white/grey text on a dark background -the textfield seems a little tall given the size of the font, although I'm not sure if you can fix this. Does it scale the font automatically, or do you have to specify both the font size and the field size?

screenshot: http://www.grabup.com/uploads/f90d04c369d2a2d56ce13a63ed0a07a5.png still needs a lot of polish. should probably make the padding around the textbox even all the way around, needs bigger tag icon.

(In reply to comment #7) > Looks like this will do odd things for non-maximized, arbitrarily positioned > windows. so, i tried the approach you use in ctl-tab, and problems w/ both x and y, whereas what i had before seems to work fine. > This should be done in a popupshown listener. done

Comment on attachment 344494 [details] [diff] [review] more dao, can you do another pass over this patch? alex can you do a final UI review?

Comment on attachment 344494 [details] [diff] [review] more in terms of interactions this looks fine. I'll probably file some visual polish bugs after B2, regarding font size, level of opacity of the panel, etc.

Comment on attachment 344494 [details] [diff] [review] more - the QueryInterface method shouldn't be needed - getBrowser() should be gBrowser - Is it expected that Browser:TagCurrentPage will be used in some other place? How about adding oncommand="TagKUI.openPopup();" directly to the 'key' element? >+#tagEntryInput { >+ width: 300px; >+ min-height: 50px; >+ max-height: 50px; >+ font-size: 2em; Are min-height and max-height really needed? Shouldn't the textbox just size depending on the font-size?

Oh, and |if (aEvent.target.id == "tagEntryPanel") {| in onPopupHiding doesn't seem useful...

- all comments fixed - fixed response to TabSelect event (save and close) - removed: it would delete bookmark if no tags and only single unfiled bookmark

Comment on attachment 346205 [details] [diff] [review] more >diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js >+ _url: null, comment explaining when this is used, etc please? >+ get panel() { >+ delete this.panel; >+ var element = document.getElementById("tagEntryPanel"); >+ // initially the panel is hidden >+ // to avoid impacting startup / new window performance >+ element.hidden = false; I'm not sure I buy this comment. The element is already created (it's in the XUL file), and it already has hidden="true", making the line of code after the comment unneeded. If you really wanted to avoid Ts, you could create the element dynamically when this is called. >+ openPopup: function TagKUI_openPopup() { >+ this.panel.openPopupAtScreen(screen.availLeft + (screen.availWidth - this.panel.width) / 2, >+ screen.availTop + (screen.availHeight - this.panel.height) / 2, >+ false); use some local variables to help with line wrapping please? >+ onPopupShown: function TagKUI_onPopupShown(aEvent) { >+ this.inputField.value = ""; >+ var tags = PlacesUtils.tagging.getTagsForURI(this._uri, {}); >+ if (tags.length > 0) >+ this.inputField.value = tags.join(", "); is that right for RTL? >+ this.inputField.select(); do we really want to select all the text, or do we just want to focus? Adding a new tag means you have to go to the end now. >+ onPopupHiding: function TagKUI_onPopupHiding(aEvent) { >+ if (!this._abort) >+ this.saveTags(); >+ else >+ this._abort = false; To be clear, this isn't instant apply, but it looks like all our other UI that is instant apply? I'm wondering if it's not a good idea to keep track of what we changed and make it instant apply and undo whatever we have to. I know it won't be easy, but what about tests for this? I might be able to throw cycles at that tomorrow...

(In reply to comment #17) > >+ onPopupShown: function TagKUI_onPopupShown(aEvent) { > >+ this.inputField.value = ""; > >+ var tags = PlacesUtils.tagging.getTagsForURI(this._uri, {}); > >+ if (tags.length > 0) > >+ this.inputField.value = tags.join(", "); > is that right for RTL? I'm not sure about Hebrew, but Arabic and Persian use a different comma character altogether: ، . So it's neither RTL-friendly nor l10n-friendly I guess. Should provide a way for localizations to be able to customize the glue string.

Comment on attachment 346205 [details] [diff] [review] more >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 >@@ -363,7 +363,130 @@ >+ init: function TagKUI_init() { >+ gBrowser.tabContainer.addEventListener("TabSelect", this, false); Why do you use tabContainer rather than just gBrowser? >+ >+ saveTags: function TagKUI_saveTags() { >+ // get current URI >+ if (this._uri) { >+ // clear existing tags >+ PlacesUtils.tagging.untagURI(this._uri, null); >+ Performance-wise, it might be much better to check the tag lists against each other (the two edit-dialog does tha FWIW). I don't mind if you spin that off. >+ var tagString = this.inputField.value; >+ if (tagString.length > 0) { >+ // split and trim input >+ var tags = this.inputField.value.split(","); >+ tags = tags.map(function(el) { >+ return el.replace(/^\s\s*/, '').replace(/\s\s*$/, ''); >+ }); >+ // add the tags >+ PlacesUtils.tagging.tagURI(this._uri, tags); >+ // bookmark the page to get the proper title >+ PlacesCommandHook.bookmarkCurrentPage(false); Note here that bookmark[current]page takes care of not creating multiple bookmarks. >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >+ >+ <panel id="tagEntryPanel" class="KUI-panel" hidden="true" >+ orient="horizontal" pack="start" align="center" width="318" >+ onpopupshown="TagKUI.onPopupShown(event);"> >+ <image id="tagEntryImage"/> >+ <vbox> Don't you need flex="1" here? >+ <label id="tagEntryPanelTitleLabel" value="&tagEntryPanelTitle.label;"/> Set control= for accessibility. >+ <textbox id="tagEntryInput" type="autocomplete" >+ autocompletesearch="simple-autocomplete" completedefaultindex="true" >+ enablehistory="false" /> Where's the automcomple-source set? >diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd >--- a/browser/locales/en-US/chrome/browser/browser.dtd >+++ b/browser/locales/en-US/chrome/browser/browser.dtd >@@ -98,6 +98,9 @@ > <!ENTITY bookmarksSidebarCmd.accesskey "B"> > <!ENTITY bookmarksSidebarCmd.commandkey "b"> > <!ENTITY bookmarksSidebarWinCmd.commandkey "i"> >+ >+<!ENTITY tagCurrentPageCmd.commandkey "c"> Accel+Shift+C won't work on gtk2. Minusing for that reason only.

For the gluing of tags together, how do the places utils return tags? Sorted in any way? As in various other places, the real trick starts as soon as you tag with more than just one direction, i.e., if one tag is LTR and two tags are RTL. Like, even in a RTL interface, I'd expect tags in latin chars to sort and glue in LTR direction, and vice versa. smontagu owes us a hook to make this right in bug 455334, but you could use the regexps over there to boot. My personal suggestion would be to use the first tag's direction for the whole layout, and probably have a joiner for both RTL and LTR with explicit ordering markers at the side. I think that stuff exists, but smontagu would know.

(In reply to comment #17) > >+ get panel() { > >+ delete this.panel; > >+ var element = document.getElementById("tagEntryPanel"); > >+ // initially the panel is hidden > >+ // to avoid impacting startup / new window performance > >+ element.hidden = false; > I'm not sure I buy this comment. The element is already created (it's in the > XUL file), and it already has hidden="true", making the line of code after the > comment unneeded. If you really wanted to avoid Ts, you could create the > element dynamically when this is called. This is taken from the bookmarks panel that mano did, a response to Ts hits there. We're saving event registration, some layout nanoseconds until the panel is actually shown. I'm not sure if hidden attr provides any particular Ts win... Mano? > >+ this.inputField.select(); > do we really want to select all the text, or do we just want to focus? Adding > a new tag means you have to go to the end now. Good call, much more usable that way. (In reply to comment #19) > >+ > >+<!ENTITY tagCurrentPageCmd.commandkey "c"> > > Accel+Shift+C won't work on gtk2. Minusing for that reason only. Anybody have a suggestion for a Linuxy shortcut for this? (In reply to comment #20) > For the gluing of tags together, how do the places utils return tags? Sorted in > any way? Places returns a comma delimited string nearly everywhere. I'll file a follow up bug for properly supporting RTL and localized separator consistently across Places.

I guess RTL tags are screwed in most places in places, after a quick chat with dietrich. Can we get a bug filed to track a thorough review of what we do in that case for 3.1.next?

(In reply to comment #17) > >+ // initially the panel is hidden > >+ // to avoid impacting startup / new window performance > >+ element.hidden = false; > I'm not sure I buy this comment. The element is already created (it's in the > XUL file), and it already has hidden="true", making the line of code after the > comment unneeded. hidden=true means display: none, so we don't create any frames for it, despite it being in the document. For panel elements especially, this is non-trivial. The second part of your comment doesn't make any sense - .hidden = false is undoing the hidden="true".

I'll post the windows icons in between B2 and the next milestone (B3 or RC1), in the meantime just use either of these on XP and Vista, and we'll do image drop ins later.

(In reply to comment #21) > Places returns a comma delimited string nearly everywhere. I'll file a follow > up bug for properly supporting RTL and localized separator consistently across > Places. filed bug 464634.

not going to make 3.1, locales changes here, retargeting 3.2

What happened to this patch in the last two months? How are the chances of rolling the functionality into a new "tweez" version?

(In reply to comment #30) > What happened to this patch in the last two months? How are the chances of > rolling the functionality into a new "tweez" version? Work priorities shifted - developers are now focused on finishing up 3.1. There's a good chance this will make 3.2 however.

>Work priorities shifted - developers are now focused on finishing up 3.1. >There's a good chance this will make 3.2 however. It would be nice not just include this in 3.2, but to have it be one of several keyboard-focused user interfaces, with that being a wider theme (kind of like we focused on a variety of different privacy features for 3.1).

updated the extension to use the shortcut and styling from this patch: https://addons.mozilla.org/en-US/firefox/addon/6353

any string freeze past, asking to reevaluate as wanted 3.6

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

This is a mass change. Every comment has "assigned-to-new" in it. I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.