631374 - "Edit This Bookmark" dialog: checking an existing tag in the tags list scrolls the tag out of view

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ilja Sekler]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110203 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110203 Firefox/4.0b12pre

When assigning an existing tag to a bookmark or deleting a tag from a bookmark in "Edit This Bookmark" dialog by checking or unchecking the checkbox for this tag in the expanded tags list, the tags list scrolls one page (for me: 8 tags) up, moving the just (un)checked tag out of view.

This behavior was introduced by checkin <http://hg.mozilla.org/mozilla-central/rev/e31e67a25d54> in Bug 394353, verified by a local backout.


Reproducible: Always

Steps to Reproduce:
1. Ensure that you have a lot of tags (>16 should be OK)
2. Load a bookmark, click on the star to edit it, expand the tags list
3. Scroll the tags list one or more pages down
4. Check (or uncheck if already checked) a checkbox for a tag.

Actual Results:  
The tags list scrolls ("jumps") one page up, the just (un)checked tag is moved out of view.


Expected Results:  
The scroll position of the tags list doesn't change.

Comment 1

[Marco Bonardo [:mak]]

well, without that fix the list would not be even updated correctly, the problem is that after an update we don't scroll to the last selected entry.

Comment 2

[ovidiu]

Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Same issue on xp. So changing to all sys.

(In reply to comment #1)
That is correct, cause any click on a tag scrolls up, so it refreshes.
*But could it be that the updating list does more than that ?*

Cause there is a funny flickering of the list if I drag the scroll down while for a second the thumb does shrink a bit too much and comes back to the original size. As if regenerating the list and figuring out the scroll thumb size as it reaches the bottom and I keep scrolling. 
But that happens without the need to refresh, just scrolling issue.

[This may be another issue, or not.]

Comment 3

[Benjamin Stover (:stechz)]

Nominating for blocking because this is rather prevalent in Firefox's UI, and it is an annoying bug. Feel free to b-.

Comment 5

[Marco Bonardo [:mak]]

While annoying, I don't think has potential to block, but I'd like to fix it in a maintenance release.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

I wouldn't even say that it blocks a maintenance release, but may be considered a safe patch for one.

Comment 7

[Perry Wagle]

See how you feel if you depend on tags.  Give each of 30 new bookmark 5 tags of at least a hundred possibilities.  Continuously change the tags on existing bookmarks.  Have it pause and scroll up each time you try to toggle a tag.  Have it then toggle the wrong tag (because it scrolled first, and accepted the click second) and scroll up again, so you can't see the tag it incorrectly toggled.

I vote blocker.  This is a regression from ff 3.6.

Comment 8

[Marco Bonardo [:mak]]

Nobody is reducing the importance of this bug, I added [places-next-wanted] to the whiteboard because I'll track these as high priority stuff that should be fixed ASAP.
But as said, for how much this could be annoying, it's not something that should prevent us from giving a really awesome product to hundreds millions of users, sorry.

Comment 9

[Perry Wagle]

"Hundreds of millions of users" times 1% (percentage of them that use tags a lot) is still millions of users who will feel screwed over by converting to ff 4.0 final release.  Yeah, I DID think of that.

But I heard you when you said "high priority".  Thanks.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.0

this seems to work pretty well, and it's faster too. As things are now, the more tags you have the slower the panel gets, this fixes that problem.
Still needs a test though.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.1

changes bookmarks service to not notify "tags" changes to tags themselves, and restores the listbox scroll position on rebuild. With tests.

Comment 12

[Marco Bonardo [:mak]]

Note that while working on this I've found other perf problems caused by tags-as-bookmarks (filed apart), we should really solve this thing, once and for all.

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 529277 [details] [diff] [review]
patch v1.1


>   // Double ordering covers possible lastModified ties, that could happen when
>   // importing, syncing or due to extensions.
>   // Note: not using a JOIN is cheaper in this case.
>   RETURN_IF_STMT(mDBFindURIBookmarks, NS_LITERAL_CSTRING(
>-    "SELECT b.id "
>+    "SELECT b.id, t.parent "
>     "FROM moz_bookmarks b "
>+    "JOIN moz_bookmarks t on t.id = b.parent "
>     "WHERE b.fk = (SELECT id FROM moz_places WHERE url = :page_url) "
>     "ORDER BY b.lastModified DESC, b.id DESC "));

have you done a test with a large dataset and some query analysis to determine the performance impact of this?


>   /**
>    * TArray version of getBookmarksIdForURI for ease of use in C++ code.
>    * Pass in a reference to a TArray; it will get filled with the
>    * resulting list of bookmark IDs.
>    */
>   nsresult GetBookmarkIdsForURITArray(nsIURI* aURI,
>-                                      nsTArray<PRInt64>& aResult);
>+                                      nsTArray<PRInt64>& aResult,
>+                                      bool aSkipTags);

Add a comment for the new param?

r=me w/ above comments addressed.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> Comment on attachment 529277 [details] [diff] [review] [review]
> patch v1.1
> 
> 
> >   // Double ordering covers possible lastModified ties, that could happen when
> >   // importing, syncing or due to extensions.
> >   // Note: not using a JOIN is cheaper in this case.
> >   RETURN_IF_STMT(mDBFindURIBookmarks, NS_LITERAL_CSTRING(
> >-    "SELECT b.id "
> >+    "SELECT b.id, t.parent "
> >     "FROM moz_bookmarks b "
> >+    "JOIN moz_bookmarks t on t.id = b.parent "
> >     "WHERE b.fk = (SELECT id FROM moz_places WHERE url = :page_url) "
> >     "ORDER BY b.lastModified DESC, b.id DESC "));
> 
> have you done a test with a large dataset and some query analysis to determine
> the performance impact of this?

GetBookmarksForURI doesn't have that kind of usage that could slowdown navigation without an explicit user action, those kind of callers are already using an async version of it (either AsyncGetBookmarksForURI in naNavBookmarks, or PU.asyncGetBookmarkIds in js).
Not doing this is instead making any tagging UI really slow, since our target in Firefox is to have most actions act in < 50ms, the filtering is needed.

Btw, will do a small perf comparison with a large bookmarks table.
Once tags will be moved, all of this filtering could go away and I'll be happier.

Comment 15

[Marco Bonardo [:mak]]

ok, done a test on the max dirty profile that has a uselessy inflated moz_bookmarks table, the difference is uninteresting (sqlite can't even measure it), since the url matching comes first. EXPLAIN confirms that the only added operation is an indexed search in moz_bookmarks using the primary key, that is pretty fast.

Comment 16

[Marco Bonardo [:mak]]

with fixed comment:
http://hg.mozilla.org/projects/places/rev/96cbb26bed19

Comment 17

[Perry Wagle]

Just for some data:  I have about 7000 bookmarks and 1000 tags, and it's quite slow in 4.0.1.  I also am running xmarks, noscript, zotero, and tagsieve extensions.  If you want me to try anything tell me how.  The alpha says its version 6, which none of the extensions are compatible with.

Comment 18

[Marco Bonardo [:mak]]

This fix as others will be in Nightly (yes, what you could call alpha 6) when resolved, it could take a while before extensions are updated for it though.

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/96cbb26bed19

Comment 20

[Simona Badau, Desktop QA]

Verified on:
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110512 Firefox/6.0a1

Comment 21

[Perry Wagle]

bug is still there in ff 5.0 beta 2

Comment 22

[Marco Bonardo [:mak]]

Did you notice the target milestone of the bug above?

Comment 23

[Perry Wagle]

Weeks ago, when I encountered the name "firefox 6", I thought it a fiction that I didn't understand the logic behind.  Firefox 5 coming out didn't change my confusion.  In any case, firefox 6 sounds years in the future, despite firefox 5 starting to come out weeks after firefox, so figured you might want to retarget for 5, since it was more real or something.  I would sort of like my continuous PAIN to stop more sooner than later, since I have to deal with the scrollback thing many dozens of times a day and it is not merely annoying.

Comment 24

[Perry Wagle]

PS.  But thanks much for even doing the fix.  Seriously.

Comment 25

[Marco Bonardo [:mak]]

Firefox 5 will be out in June, FF6 in August, FF7 in October (I'm rounding here, so don't take these dates as perfect), the concept is that with our new fast-releases approach you will get a new version every 6 weeks.

Comment 26

[Perry Wagle]

Bug is retriggered if you are editing bookmarks while scrolled down when a calendar popup appears.  The editing bookmarks dialog goes away when you click on the calendar popup to make *it* go away.  When you then restart the editing bookmarks dialog, it starts out partially scrolled down, and when you click on a bookmark, it scrolls up.

Comment 27

[Marco Bonardo [:mak]]

File a separate bug with a good description of the steps. I'm not sure what do you mean by calendar popup, Firefox doesn't have any calendar popup.

Comment 28

[Perry Wagle]

I run MacOSX.  iCal/BusyCal do popups when something is due.  When you snooze or dismiss the popup, the bookmarks editor goes away too.  Then it's very likely to trigger the original bug, which seems not to be completely fixed.  Just using the bookmarks editor (with a large number of long tags?) for any period of time also retriggers the bug.  I think the original bug wasn't actually fixed, just made less likely to trigger?

Comment 29

[Marco Bonardo [:mak]]

may be a completely different issue from what you say, involving other popups and focus and such, there may be a relation but it's for sure not the same bug.