Closed Bug 454632 Opened 11 years ago Closed 11 years ago

Resizing tree widget always re-scrolls to current index

Categories

(Core :: Layout, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: nick.kreeger, Assigned: steve)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
via: bonzai:

1.135 <sspitzer@netscape.com> 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
Attached patch Patch V1 (obsolete) — Splinter Review
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.
Assignee: nobody → nick.kreeger
Status: NEW → ASSIGNED
Comment on attachment 337926 [details] [diff] [review]
Patch V1

Asking for module owner review.
Attachment #337926 - Flags: review?
There is an old bug for this. Bug 170045
Attachment #337926 - Flags: review? → review?(bzbarsky)
Doesn't this regress bug 163404?  That doesn't seem acceptable....
Attachment #337926 - Flags: review?(bzbarsky) → review-
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.
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
Blocks: 170045
Ideally, this could be an attribute added to the tree binding?
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.
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #353631 - Flags: review?(bzbarsky)
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.
Attachment #353631 - Flags: review?(bzbarsky) → review-
Blocks: 470263
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #353631 - Attachment is obsolete: true
Attachment #353698 - Flags: review?(enndeakin)
Uh, no.  The thunderbird change needs to happen first, so there's no breakage window.
Summary: Resizing tree widget always makes re-scrolls to last selected index → Resizing tree widget always re-scrolls to current index
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.
Attached patch Patch v4 (obsolete) — Splinter Review
fourth time's the charm?
Assignee: nick.kreeger → steve
Attachment #354223 - Flags: review?(enndeakin)
Attachment #353698 - Attachment is obsolete: true
Attachment #353698 - Flags: review?(enndeakin)
Attachment #337926 - Attachment is obsolete: true
Sorry - attached the wrong patch...
Attachment #354223 - Attachment is obsolete: true
Attachment #354224 - Flags: review?(enndeakin)
Attachment #354223 - Flags: review?(enndeakin)
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'
Attachment #354224 - Flags: review?(enndeakin) → review+
Would somebody check this patch in for me please?  cheers!
Keywords: checkin-needed
Actually I suppose I need my patch for 470263 r+'d and checked in first... ?
Stevel - I'll push this for you this morning ;-)
(In reply to comment #22)
> Stevel - I'll push this for you this morning ;-)

Doh - I think you need an sr first.
Ah yup- you're right, I thought 'layout' was like toolkit and only required r+.
Keywords: checkin-needed
Attachment #354708 - Flags: superreview?(bzbarsky)
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 on attachment 354708 [details] [diff] [review]
Final patch for checkin

Looks good, but again please make sure to land bug 470263 first.
Attachment #354708 - Flags: superreview?(bzbarsky) → superreview+
No longer blocks: 470263
Depends on: 470263
Pushed:

changeset:   23617:f957a14faefd
user:        Nick Kreeger <nick.kreeger@park.edu>
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 <steve@grommit.com>.

changeset:   23619:5a53d161e4b4
tag:         tip
parent:      23617:f957a14faefd
parent:      23618:557bc4bcdc30
user:        Nick Kreeger <nick.kreeger@park.edu>
date:        Tue Jan 13 11:14:01 2009 -0800
summary:     Merge bug 454632.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 170045
Duplicate of this bug: 505351
You need to log in before you can comment on or make changes to this bug.