454632 - Resizing tree widget always re-scrolls to current index

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nick Kreeger]

When resizing a tree widget (i.e. Firefox's 'Show All History' window), the scroll view will always jump back to the previously selected item.

Steps to Reproduce:
(In Firefox)
1. Open the 'Show All History' window
2. Select an item, then scroll the view so the selection is now hidden.
3. Resize the window, observe the scroll view jump back to where the selected index was.

Note: This also happens to when tree columns are resized when horizontal scrolling has been enabled.

Comment 1

[Nick Kreeger]

via: bonzai:

1.135 <[e-mail]> 2002-08-21 17:21
fix for #163404. when the height of a tree shrinks, we should ensure that the selected row is still visible. this really comes into play in the mailnews thread pane.

when I show the message pane (after it has been collapsed), the selected message can go out of view.

if the size of the thread pane shrinks because I move the splitter or make the whole window smaller, it can happen as well.

Note Outlook Express does the same thing.

r=varga, sr=bryner. thanks to martinl for the bug report.


http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/xul/base/src/tree/src&command=DIFF_FRAMESET&file=nsTreeBodyFrame.cpp&rev2=1.135&rev1=1.134

Comment 2

[Nick Kreeger]

Attachment: Patch V1

Here is a proposed patch, it simply removes the logic for ensuring the current selected index is in view after resize events.

I linked the original commit of the removed code w/ comment.

Comment 3

[Nick Kreeger]

Comment on attachment 337926 [details] [diff] [review]
Patch V1

Asking for module owner review.

Comment 4

[Brian Polidoro]

There is an old bug for this. Bug 170045

Comment 5

[Boris Zbarsky [:bzbarsky]]

Doesn't this regress bug 163404?  That doesn't seem acceptable....

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 337926 [details] [diff] [review]
Patch V1

r- per comment 5.

Comment 7

[Brian Polidoro]

That behavior may be wanted in some cases like in mail/news but this bug and bug 170045 show it's not wanted in all cases for trees.  Should this behavior be part of the tree implementation or should it be added by users of trees that want it? Or should it be a configuration option for trees?  

For example in Windows Explorer the folder tree does not scroll the selected folder into view on resize.  So saying that it's ok to do all the time because Outlook does it does not seem to be a sufficient reason.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Did I say it's ok to do all the time?  I agree we should fix it; we just shouldn't be regressing things.  If there's a corresponding change to mailnews to not break it, that should be fine by me.

That said, if the tree is scrolled all the way to the bottom, and the last item is selected (or even if it's not!), and the tree is made shorter, it seems to me it should remain scrolled all the way to the bottom.  This doesn't require unconditionally scrolling to the selected index, of course.  There are other ways to accomplish this behavior.

I would argue that we _also_ want the behavior that if there is a selected item in view before the resize starts it should still be in view after the resize ends.  If it's not in view at resize start, nothing should happen to scroll to it.

ccing some XUL module owners

Comment 9

[Nick Kreeger]

Ideally, this could be an attribute added to the tree binding?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Possibly, but we should have sane default behavior no matter what.  Prefs with bad defaults are just as much a problem for developers as for users.

Comment 11

[[e-mail]]

I'd be happy for the sane default to be "don't adjust anything". I was particularly annoyed by the patch to bug 163404 because it means that collapsing a tree always scrolls it to the current index.

On the other hand, that doesn't mean that I don't want the thread pane not to scroll to the current index when I uncollapse the message pane ;-)

It may be possible to calculate whether the current index is in view in ReflowFinished() using the old mInnerBox before CalcInnerBox() clobbers it.

Comment 12

[Stephen Lau]

Attachment: Patch v2

This is an updated version of Nick's patch that adds a keepSelectionOnResize attribute to the tree widget.  The default is to NOT scroll to the selection, so you have to specify keepSelectionOnResize=true if you want that behaviour.  I'm open to flipping the default... whatever people think is best.
I'm also not super-keen on the attribute name, if people have better suggestions?

Boris? Might reviewing this?

Comment 13

[Neil Deakin]

Comment on attachment 353631 [details] [diff] [review]
Patch v2

>+	PRBool keepSelectionOnResize;
>+    if (NS_SUCCEEDED(element->HasAttribute(NS_LITERAL_STRING("keepSelectionOnResize"), &keepSelectionOnResize)) && keepSelectionOnResize) {

This should use nsIContent::AttrValueIs and compare directly with nsGkAtoms::_true.

Technically, I think ReflowFinished could be called even when the tree hasn't resized, so the attribute is a bit misleading

keepcurrentinview (attributes should be all lowercase) might be a better choice to imply that the current row should be kept in view. I can't think of a better name either. Note also that the term 'selection' isn't right here as the current index refers to the row that is focused and may not actually be selected.

You'll need to update Thunderbird or file a bug and notify them of the change as well, of course.

minus for now, but I think the concept is ok.

Comment 14

[Stephen Lau]

Attachment: Patch v3

Updated per Neil's comments to use AttrValueIs, and to rename the attribute to 'keepcurrentinview'.

Bug 470263 has been filed (and marked dependent on this bug) to track the corresponding change needed in Thunderbird.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Uh, no.  The thunderbird change needs to happen first, so there's no breakage window.

Comment 16

[Neil Deakin]

Comment on attachment 353698 [details] [diff] [review]
Patch v3

>+    nsIAtom* keepcurrentinviewAtom = NS_NewAtom("keepcurrentinview");

Generally, we add the atom to nsGkAtomList.h instead of creating an atom each time. That way, it wouldn't leak.

>+      <property name="keepcurrentinview"

I meant, the attribute should be lowercase, but the property name should match the JS convention of being mixed-case.

Comment 17

[Stephen Lau]

Attachment: Patch v4

fourth time's the charm?

Comment 18

[Stephen Lau]

Attachment: Gah. attached the wrong patch.

Sorry - attached the wrong patch...

Comment 19

[Neil Deakin]

Comment on attachment 354224 [details] [diff] [review]
Gah. attached the wrong patch.


>+      <property name="keepCurrentInView"
>+                onget="return this.hasAttribute('keepcurrentinview');"

The getter should compare the attribute to 'true' as in getAttribute('...') == 'true'

Comment 20

[Stephen Lau]

Attachment: Final patch for checkin

Would somebody check this patch in for me please?  cheers!

Comment 21

[Stephen Lau]

Actually I suppose I need my patch for 470263 r+'d and checked in first... ?

Comment 22

[Nick Kreeger]

Stevel - I'll push this for you this morning ;-)

Comment 23

[Nick Kreeger]

(In reply to comment #22)
> Stevel - I'll push this for you this morning ;-)

Doh - I think you need an sr first.

Comment 24

[Stephen Lau]

Ah yup- you're right, I thought 'layout' was like toolkit and only required r+.

Comment 25

[Stephen Lau]

Comment on attachment 354708 [details] [diff] [review]
Final patch for checkin

Asking Boris for sr? since he's taken a look at it a couple of times already.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354708 [details] [diff] [review]
Final patch for checkin

Looks good, but again please make sure to land bug 470263 first.

Comment 27

[Nick Kreeger]

Pushed:

changeset:   23617:f957a14faefd
user:        Nick Kreeger <[e-mail]>
date:        Tue Jan 13 10:50:26 2009 -0800
summary:     Fixing Bug 454632 -  Resizing tree widget always re-scrolls to current index. r=enndeakin,sr=bz. Patch by Stephen Lau <[e-mail]>.

changeset:   23619:5a53d161e4b4
tag:         tip
parent:      23617:f957a14faefd
parent:      23618:557bc4bcdc30
user:        Nick Kreeger <[e-mail]>
date:        Tue Jan 13 11:14:01 2009 -0800
summary:     Merge bug 454632.