394353 - Tag list is not updated when manually adding, renaming, or deleting tags from within the tags field

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Al Billings [:abillings - ex-MoCo]]

In the new dialog for adding a bookmark, there is a text box to type in tags. This has an expansion arrow next to it that shows the tags that are available (having already been used for other bookmarks).

Whether this section with tags is displayed or not is a sticky session. Once the user opens it, it will be open whenever the "Add Bookmark" dialog is brought up. If it is closed, it will stay closed.

I found that if it is expanded and a *new* tag is added to a bookmark, the new tag will not be added to the list of available tags immediately. If the user is on the page where the new tag was added and invokes the "Add Bookmark" dialog, they will see their new tag in the textbox for tags but not the list of available tags.

If the expansion arrow next to the tags text box is selected to close the tag list and then selected again to reopen it, the new tag will *then* appear in the list. This happens even if you are bookmarking a different page. Any added tags do not appear in the list of available tags, at least in that session, until the list is collapsed and re-expanded.

Steps to Reproduce
1. Navigate to a new page.
2. Select control/command-d to add the page to bookmarks.
3. Select the button next to the "tags" textbox to expand the tags list.
4. Type in the tag, '1234', into the tag textbox.
5. Select the "Done" button to close the dialog.
6. Navigate to a different page.
7. Select control/command-d to add the page to bookmarks.
8. Look at the tags list (the list will be expanded). '1234' will not be in the list of available tags.
9. Select the button next to the "tags" textbox twise, to collapse and expand the tags list.

Result: '1234' will now appear in the list of tags when it wasn't there before.

Comment 1

[Tracy Walker [:tracy]]

we need a full battery of new tests in litmus to cover this UI

Comment 2

[Dietrich Ayala (:dietrich)]

Requesting blocking. This bug is pretty obvious if you use tags a lot.

Comment 3

[RenegadeX]

I was just about to file a similar bug - specifically for the same behaviour in 'Places Organizer', but was pointed to this Bug by someone on IRC. Testing Places (3.0a9pre) for the first time, I'd imported my old bookmarks and was in the process of tagging them in the 'Places Organizer', and after clicking the Tag list 'roll down arrow' noticed that the now-exposed Tags list was not refreshing when I clicked on a different bookmark. It only refreshes the list if I 'roll up' the Tags list and then roll it down again.

This should be 'BLOCKING', IMHO - as it severely affects the usability of Places.

Update: lol -- Dietrich beat me to it while i was typing this!

Comment 4

[Florian Quèze [:florian]]

I can reproduce with the 2007-10-07-04 nightly but I can't reproduce with a current trunk build. At step 8 the tag list is not expanded.

WORKFORME?

Comment 5

[(not reading, please use [e-mail] instead)]

I'm seeing what Florian sees, but I remember seeing what Al saw.

We appear to now be resetting the state of the twisties in the edit bookmark popup.

Comment 6

[RenegadeX]

Also confirming  what Florian & Seth observe here on XP with 2007110605 build, which I assume is the intended behaviour (unless of course it's meant to remember the open/close status of the Tags List, which might be nice). However, to restate what I was talking about before, try this:

Steps to Reproduce:
1. Open 'Places Organizer'
2. In the upper-right pane, scroll down until you come to some bookmarks without any Tags. Select one of them.
3. Click the 'expand Tags' down arrow to expand the Tags list.
3. Give the item a Tag such as 'xyz'.
4. Now click on any other bookmark. Observe that 'xyz' is not present in the list of available Tags. Click a few other bookmarks just to confirm -- 'xyz' still won't appear.
5. Now close the Tags list by clicking the Tags 'up' arrow (it now changes to a 'down' arrow).
6. Click the 'down' arrow.
7. Observe that 'xyz' is now present in the list.

Expected Result:
- The Tags list should refresh automatically when a different bookmark item is selected; no Tags list up-&-down arrow clicking should be required.

***Related Symptoms***
- As the Tags List is not currently updating as long as it remains open, still in Places Organizer:
1. If, with the Tags List hidden, a bookmarked item with no Tag assigned to it is selected, and then the Tags List is opened, clicking on other previously-Tagged bookmarked appears to show that NO Tags have been assigned to any (ie: the checkboxes remain empty for all).
2. Similarly, if the Tags List is expanded when a bookmark item with Tags already assigned to it is selected, it appears that ALL following-clicked bookmark items have picked up that original item's Tags (ie: the checkboxes that were checked for it remain checked for every bookmark item).

Comment 7

[Florian Quèze [:florian]]

So what's the expected behavior here?

When the tag list is expended and the user selects another bookmark, should the tag list get collapsed while the info of the newly selected bookmark is displayed or should the tag selector stay expended and it's content be updated?

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Any chance on this getting fixed soon?

Florian, I would think that the contents would update.

Comment 9

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

I agree. The tag list should stay open when selecting a new bookmark. Another behavior will confuse and especially bother the user to re-open the list each time.

But the question still remains. When the tag list should be updated? Should we listen to each keypress event or should it be done when the user hits return or leaves the input field?

Comment 10

[Michael Schonfeld]

Just tried to reproduce this bug, and was unable to. The tags list in the "add bookmark" start popup isn't state-remembering, thus the list is always closed when adding a new bookmark.  This is also un-reproducible in the bookmarks library. Should we close as WFM?

Comment 11

[RenegadeX]

I can confirm the reported problems in Comment #0 and Comment #6 are now not present. Rather than "WFM" should it not be "FIXED, VERIFIED"?

Comment 12

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

A part of this issue is still visible but I don't know if it is still covered by another bug. I cannot find a similar one. Just do following:

1. Open the Library and expand the Tag folder
2. Open edit dialog of a bookmark
3. Expanding the tag list
4. Add some tags in the tag text field
5. Hit tab key

As you see now the list of tags within the Library is updated but the tag list inside the edit bookmark dialog doesn't show this newly added tags. It should also be updated when leaving the tag text field.

Comment 13

[Michael Schonfeld]

What you are saying is true, but you don't have to close the tags list and reopen it in order for it to update. All you have to do is click on another bookmark, and then click on the one you edited again.  But I agree, this is a bug.  Maybe we should change the title here accordingly then, or start a new one all together (seems more appropriate).

Comment 14

[Michael Schonfeld]

Attachment: v1

One-liner.... Hopefully approved!

Comment 15

[Michael Schonfeld]

This is going to be real tricky. Asaf has pointed my attention to some scenarios where my patch will simply be inaffective. For example, if the library is open and showing a specific bookmark and its tags, while that same bookmark is changed through the browser window... Anyway, I am canceling the review request and rewriting this.

Comment 16

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

(In reply to comment #15)
> scenarios where my patch will simply be inaffective. For example, if the
> library is open and showing a specific bookmark and its tags, while that same
> bookmark is changed through the browser window... Anyway, I am canceling the

The only problem I can see with this example is that the bookmark properties are getting closed when leaving the tags field within the browser window. I think that this shouldn't happen. Asaf, is there already a bug about this issue?

Comment 17

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 18

[Marco Bonardo [:mak]]

the behavior is already well defined, the field should update in every situation where tags for the edited item change, so on tagsField blur but also when the bookmark gets a new tag while it is being edited.

Comment 19

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?id=5122 has been updated on the 3.0 test run for regression testing.

Comment 20

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

Huh, looks like this bug was getting forgotten. Marco, Dietrich, any chance to get this fixed for FF3.1? If this one-line patch will help it would be fantastic.

Aakash, this bug is not fixed yet. Means the Litmus test should be disabled. Further please see comment 0 for detailed steps. Your test isn't correct. It misses major parts (step 3 and step 9). Even please try to focus on already fixed bugs. It will save you from adding not needed tests for now. Thanks.

Comment 21

[Aakash Desai [:aakashd]]

Ah, Whoops! Thanks for catching that, Henrik. I've reverted the test case back to its original form and have created a new disabled test case here.  The new test case is located here: https://litmus.mozilla.org/show_test.cgi?id=7486

Comment 22

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

Michael, are you still wanna work on this bug?

Comment 24

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

No response from assignee. Lets assign it to default. Further I run into this issue several times while doing the FFT's on XP.

Comment 25

[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 27

[Marco Bonardo [:mak]]

Attachment: patch v1.0 [Checked in: Comment 28 & 29]

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/61546ddb725f

Comment 29

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/61546ddb725f

Comment 30

[Marco Bonardo [:mak]]

Reopening because I missed some of the cases (Thus this can't be verified) and the fix can use a better observer

Comment 31

[Marco Bonardo [:mak]]

Attachment: part 2: more tests and fixes

This i why I hate tests, just writing a test with more cases found 3 or 4 small bugs related to this simple bug.

afaict this should cover all the testcases reported in this bug, and the chrome test is good enough to ensure them.

Comment 32

[Dietrich Ayala (:dietrich)]

Comment on attachment 506743 [details] [diff] [review]
part 2: more tests and fixes


>   _getCommonTags: function(aArrIndex) {

s/aArrIndex//

also, please add a comment explaining what this function does. i had to poke around to remember what it does, since the name is ambiguous.

>   onItemChanged: function EIO_onItemChanged(aItemId, aProperty,
>                                             aIsAnnotationProperty, aValue,
>                                             aLastModified, aItemType) {
>+    if (aProperty == "tags") {
>+      // Tags case is special, since they should be updated if either:
>+      // - the notification is for the edited bookmark
>+      // - the notification is for the edited history entry
>+      // - the notification is for one of edited uris
>+      let shouldUpdateTagsField = this._itemId == aItemId;
>+      if (this._itemId == -1 || this._multiEdit) {
>+        // Check if the changed uri is part of the modified ones.
>+        let changedURI = PlacesUtils.bookmarks.getBookmarkURI(aItemId);
>+        let uris = this._multiEdit ? this._uris : [this._uri];
>+        if (uris.some(function (aURI) aURI.spec == changedURI.spec)) {
>+          shouldUpdateTagsField = true;
>+        }
>+      }
>+
>+      if (shouldUpdateTagsField) {
>+        if (this._multiEdit) {
>+          this._tags = this._uris.map(function (aURI) PlacesUtils.tagging.getTagsForURI(aURI));

what if i select all in my unsorted folder? seems like an easy way to block the UI. we don't need to address this now if it's no worse than whatever happens currently in that situation, only if it makes things worse.

r=me, but leaving a? until confirmed that the above change doesn't cause a new hang, or measurably more hang.

Comment 33

[Marco Bonardo [:mak]]

Attachment: part 2: more tests and fixes (v1.1)

good point, this version will update tags only for uris touched by the notification, thus reducing reads for a lot of entries, I still have to walk the full array of entries, but this does not involve any read.

Comment 34

[Marco Bonardo [:mak]]

Attachment: part 2: more tests and fixes (v1.1) [Checked in: Comment 36]

bah I keep attaching old version of the patches :\ sorry

Comment 35

[[e-mail]]

Comment on attachment 506822 [details] [diff] [review]
part 2: more tests and fixes (v1.1)
[Checked in: Comment 36]

Seems reasonable to me.

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 506822 [details] [diff] [review]
part 2: more tests and fixes (v1.1)
[Checked in: Comment 36]

http://hg.mozilla.org/mozilla-central/rev/e31e67a25d54