434946 - should provide a keyboard-oriented UI for quickly tagging a page

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, enhancement)

Description

[Dietrich Ayala (:dietrich)]

Attachment: wip

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.

Comment 1

[Dietrich Ayala (:dietrich)]

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?

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: wip

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: screenshot

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: added label

Comment 5

[Andrés Delfino]

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.

Comment 6

[Dão Gottwald [:dao]]

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 7

[Dão Gottwald [:dao]]

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.

Comment 8

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

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

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

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?

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: more

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.

Comment 11

[Dietrich Ayala (:dietrich)]

(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 12

[Dietrich Ayala (:dietrich)]

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 13

[Alex Faaborg [:faaborg] (Firefox UX)]

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 14

[Dão Gottwald [:dao]]

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?

Comment 15

[Dão Gottwald [:dao]]

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

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: more

- 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 17

[Shawn Wilsher :sdwilsh]

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...

Comment 18

[(no longer active)]

(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 19

[Asaf Romano (gone)]

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.

Comment 20

[Axel Hecht [:Pike]]

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.

Comment 21

[Dietrich Ayala (:dietrich)]

(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.

Comment 22

[Axel Hecht [:Pike]]

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?

Comment 23

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

(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".

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: OS X large tag

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Linux large tag

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 27

[Dietrich Ayala (:dietrich)]

(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.

Comment 28

[Dietrich Ayala (:dietrich)]

Attachment: screenshot w/ large tag icon and autocomplete

Comment 29

[Marco Bonardo [:mak]]

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

Comment 30

[Thomas Stache]

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

Comment 31

[Shawn Wilsher :sdwilsh]

(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.

Comment 32

[Alex Faaborg [:faaborg] (Firefox UX)]

>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).

Comment 33

[Dietrich Ayala (:dietrich)]

updated the extension to use the shortcut and styling from this patch:

https://addons.mozilla.org/en-US/firefox/addon/6353

Comment 34

[Marco Bonardo [:mak]]

any string freeze past, asking to reevaluate as wanted 3.6

Comment 35

[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

Comment 36

[Michael Kohler [:mkohler]]

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.

Comment 37

[BMO Automation]

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.