Closed Bug 462793 Opened 16 years ago Closed 16 years ago

Changing listbox selection with keyboard no longer scrolls to make the newly selected item visible

Categories

(Core :: Layout: Form Controls, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: jruderman, Assigned: mstange)

References

()

Details

(Keywords: access, regression, testcase)

Attachments

(2 files)

Attached file testcase
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081102 Minefield/3.1b2pre

Steps to reproduce:
1. Click item 2.
2. Press '8' on the keyboard, or press the down arrow key six times.

Result: Item 8 becomes selected, but the listbox doesn't scroll to make it visible.

Expected: Item 8 should become selected and visible.

This is a regression from about a week ago.
Flags: blocking1.9.1?
This worksforme in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081101 Minefield/3.1b2pre
But I can confirm that Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081102 Minefield/3.1b2pre fails.

Sounds like we have a regression range here.  ;)
Regression range seems to be something like http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-11-01+00%3A00%3A00&enddate=2008-11-02+02%3A00%3A00

The start is sorta a guess; the end is based on my about:buildconfig changeset.

Given that list, I will lay money on bug 457864.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
The PR_TRUE in nsListControlFrame.cpp's call to ScrollTo was misinterpreted as NS_VMREFRESH_DEFERRED. Before bug 457864 it was just ignored.

I also fixed (hopefully) all the other callers of ScrollTo that pass something different than NS_VMREFRESH_DEFERRED or NS_VMREFRESH_SMOOTHSCROLL to ScrollTo.
Attachment #346060 - Flags: review?(bzbarsky)
> patch v1: pass 0 to ScrollTo in all cases where neither NS_VMREFRESH_DEFERRED
> nor NS_VMREFRESH_SMOOTHSCROLL are used

Why, exactly?  Is it because ScrollTo only recognizes those two flags?  Should that be documented in the API, if so?  Did the VMREFRESH_IMMEDIATE here not work before?  The API comments make it sound like it should have...

> The PR_TRUE in nsListControlFrame.cpp's call to ScrollTo

Gah.  So rods just totally passed the wrong thing to ScrollTo in all the code he wrote?  :(

Honestly, roc probably needs to review this (but I'd like an answer about the API situation here).
Oh, and why does making this a deferred scroll make the scroll just not happen?
(In reply to comment #6)
> > patch v1: pass 0 to ScrollTo in all cases where neither NS_VMREFRESH_DEFERRED
> > nor NS_VMREFRESH_SMOOTHSCROLL are used
> 
> Why, exactly?  Is it because ScrollTo only recognizes those two flags?

Yes.

> Should
> that be documented in the API, if so?

Probably. Maybe we should even create new flags instead of reusing the VMREFRESH flags; that would make it clearer that they're never passed to UpdateView.

> Did the VMREFRESH_IMMEDIATE here not
> work before?  The API comments make it sound like it should have...

I suppose it did work at some point, but I haven't checked the CVS history yet.

> > The PR_TRUE in nsListControlFrame.cpp's call to ScrollTo
> 
> Gah.  So rods just totally passed the wrong thing to ScrollTo in all the code
> he wrote?  :(

Maybe ScrollTo expected a bool at the time he wrote that code. I don't know.

(In reply to comment #7)
> Oh, and why does making this a deferred scroll make the scroll just not happen?

Good question. I haven't found out yet.
Attachment #346060 - Flags: review?(bzbarsky) → review?(roc)
I just had a look at the CVS history. It looks like the aUpdateFlags paramater has been there from the start but has always been ignored. Only when smooth scrolling was added (rev 3.35), a (aUpdateFlags & NS_VMREFRESH_SMOOTHSCROLL) check was introduced.
> Maybe ScrollTo expected a bool at the time he wrote that code.

I didn't.  I double-checked that....

It sounds like we at least want a separate bug on the deferred thing breaking scrolling.
Comment on attachment 346060 [details] [diff] [review]
patch v1: pass 0 to ScrollTo in all cases where neither NS_VMREFRESH_DEFERRED nor NS_VMREFRESH_SMOOTHSCROLL are used

Scrolling always was "immediate", so the IMMEDIATE flag was just window dressing.
pushed: http://hg.mozilla.org/mozilla-central/rev/32b52c83b1a4

I'll file a new bug for the "deferred thing breaking scrolling" investigation.

We'll also want a test for this bug.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Flags: blocking1.9.1?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: