Closed Bug 631374 Opened 9 years ago Closed 9 years ago
"Edit This Bookmark" dialog: checking an existing tag in the tags list scrolls the tag out of view
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.]
OS: Linux → All
Nominating for blocking because this is rather prevalent in Firefox's UI, and it is an annoying bug. Feel free to b-.
blocking2.0: --- → ?
While annoying, I don't think has potential to block, but I'd like to fix it in a maintenance release.
I wouldn't even say that it blocks a maintenance release, but may be considered a safe patch for one.
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.
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.
"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.
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.
Hardware: x86 → All
changes bookmarks service to not notify "tags" changes to tags themselves, and restores the listbox scroll position on rebuild. With tests.
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 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.
Attachment #529277 - Flags: review?(dietrich) → review+
(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.
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.
with fixed comment: http://hg.mozilla.org/projects/places/rev/96cbb26bed19
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-places]
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Verified on: Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110512 Firefox/6.0a1
Status: RESOLVED → VERIFIED
bug is still there in ff 5.0 beta 2
Did you notice the target milestone of the bug above?
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.
PS. But thanks much for even doing the fix. Seriously.
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.
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.
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.
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?
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.
You need to log in before you can comment on or make changes to this bug.