168410 - The bookmark tree jumps unexpectedly to the bottom of its view when the the first visible row is not the first one

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Pierre Chanial]

This is the nastiest bug.
Step to reproduce: 
- open the bookmark manager
- expand your folders eventually so that there is a scrollbar
- scroll some rows
- select an item and remove it

expected: first row visible still visible
current: view jumps to the bottom

The removal of the row is wrapped in a batch (doCommand in bookmarksTree.xml)
The relevent part is here:

this.mOuter.treeBoxObject.selection.selectEventsSuppressed = true;
var observer =
this.mOuter.tree.builder.QueryInterface(Components.interfaces.nsIRDFObserver);
observer.beginUpdateBatch(this.db);
ookmarksController.doCommand(aCommand, selection, target);
observer.endUpdateBatch(this.db);
this.mOuter.tree.builder.rebuild();
this.mOuter.updateTreeSelection();
this.mOuter.treeBoxObject.selection.selectEventsSuppressed = false;

For phoenix 0.1, I could disable the batching, but it will slow down the things.

Comment 1

[Pierre Chanial]

-> hyatt, he will take care of it

Comment 2

[Pierre Chanial]

*** Bug 176401 has been marked as a duplicate of this bug. ***

Comment 3

[David Hyatt]

Checked in a hack fix for 0.4. --> 0.5.

Comment 4

[Jan Varga [:janv]]

hyatt, "additional patch" in bug 162542 fixes this problem.

Comment 5

[Blake Ross]

So this is fixed now.  Thanks Jan!

Comment 6

[Blake Ross]

Oops, I thought it was a C++ patch but it's not.  We'll have to port that fix
over to Phoenix.

Comment 7

[Pierre Chanial]

I haven't tested new builds, but iirc Jan's fix for this issue is in attachment
101158 [details] [diff] [review]. It patches nsXULTreeBuilder.cpp and it's been checked in.

Comment 8

[Pierre Chanial]

tested 11/10 build. if one comments the hack
this.mOuter.treeBoxObject.scrollToRow(firstVisibleRow);
in bookmarksTree.xml, the tree scrolls to the first row when rebuilding (before
Jan's fix, it was scrolling to the bottom).
That's still not what we want.

Comment 9

[Asa Dotzler [:asa]]

setting status to New

Comment 10

[Joshua Kwan]

Works for me. It jumps back and forth a bit, probably because of its hack-ness,
but the overall result is satisfactory for now?

Is there a better way to do this?

Comment 11

[Blake Ross]

Is this still a problem?

Comment 12

[Asa Dotzler [:asa]]

This is no longer a problem. If pch wants to continue work here then feel free
to reopen and adjust the summary or file a new bug to work on a better fix.

Comment 13

[Jan Varga [:janv]]

The XUL tree builder behaves correctly. I think it's up to the caller what to do
after a batch operation (save/restore selection/scroll position).

Comment 14

[Brant Gurganus]

Verified in latest nightly as of today.