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)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: jruderman, Assigned: mstange)
References
()
Details
(Keywords: access, regression, testcase)
Attachments
(2 files)
199 bytes,
text/html
|
Details | |
12.48 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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?
Comment 1•16 years ago
|
||
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
Comment 2•16 years ago
|
||
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. ;)
Comment 3•16 years ago
|
||
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.
Blocks: 457864
Keywords: regressionwindow-wanted
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•16 years ago
|
||
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)
Comment 6•16 years ago
|
||
> 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).
Comment 7•16 years ago
|
||
Oh, and why does making this a deferred scroll make the scroll just not happen?
Assignee | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Updated•16 years ago
|
Attachment #346060 -
Flags: review?(bzbarsky) → review?(roc)
Assignee | ||
Comment 9•16 years ago
|
||
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.
Comment 10•16 years ago
|
||
> 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.
Attachment #346060 -
Flags: superreview+
Attachment #346060 -
Flags: review?(roc)
Attachment #346060 -
Flags: review+
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.
Assignee | ||
Comment 12•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1?
You need to log in
before you can comment on or make changes to this bug.
Description
•