Closed
Bug 365410
Opened 18 years ago
Closed 15 years ago
[FIX]page up/down shows strange behaviour on multiple select with css height
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: jelle, Assigned: bzbarsky)
References
()
Details
Attachments
(2 files, 1 obsolete file)
2.47 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
4.42 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061230 Minefield/3.0a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061230 Minefield/3.0a2pre
When using a <select multiple="multiple"> with some <option>s, normal behaviour is that when using the pageup or pagedown key you can browse through the list.
But when I dictate a certain height through CSS on this <select> element, like 'height:12em' or 'height:120px', the behaviour of these keys differ. The pageup and pagedown keys behave like arrow down and arrow up, in this order. This seems undesirable to me.
Reproducible: Always
Steps to Reproduce:
1. Open testcase page
2. Click the first item in the list
3. Press pageup or pagedown and inspect behaviour
Actual Results:
The selected item changes by one item at a time instead of multiple items. Additionally, the direction of change (page UP and page DOWN) is changed into down and up.
Expected Results:
It should behave like page up and page down when not being applied a height-attribute.
I tested it with a nightly build, but I got reports from other users saying that they used Firefox 2 stable and still had problems.
Comment 1•18 years ago
|
||
Confirmed on 2.0.0.1/Linux/PPC.
Comment 2•18 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061229 Minefield/3.0a2pre
When I press pg dn it goes up (one position) and when I press pg up down. Also in Firefox 1.x. Maybe I have a strange driver. But it reacts to home and end in the right way.
Component: General → Keyboard Navigation
QA Contact: general → keyboard.navigation
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Component: Keyboard Navigation → Keyboard: Navigation
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: keyboard.navigation → keyboard.navigation
Hardware: PC → All
Version: unspecified → Trunk
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
I confirm this bug on ff 2.0.0.7. However, when a size attribute is specified and no height style attribute is specified, the behavior is correct.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
This bug still exists as described by Ria Klaasen. I was able to make a workaround by changing the height after the page had rendered the select with the following js.
setTimeout(
function(){
document.getElementsByTagName('select')[0].style.height="500px";
},
1000
);//100 milliseconds was too short on my system.
This does not work if the height has already been set(via CSS or otherwise). It's not ideal but hopefully it suggests when the bug occurs.
Reporter | ||
Updated•16 years ago
|
Blocks: 512932
![]() |
Assignee | |
Comment 6•15 years ago
|
||
So is this just something that happens any time a <select> is given a height bigger than its "natural" height? Sounds like we're ending up with a "pagesize" of -1 or something somewhere...
Reporter | ||
Comment 7•15 years ago
|
||
No, this happens as soon as you set any height to the <select> element, not just a bigger height than its natural height. It seems like a desynchronization occurs between the height, rows and pagesize properties at page load.
![]() |
Assignee | |
Comment 8•15 years ago
|
||
Oh, I see the problem. mNumDisplayRows is what we use as the page size, and it's only initialized in CalcIntrinsicHeight. Which is not called if we have a non-intrinsic height specified, of course.
![]() |
Assignee | |
Comment 9•15 years ago
|
||
It's simple enough to fix that, but the optgroup behavior would still be pretty broken (as is it now, I guess). mNumDisplayRows is really not computed all that well, nor is using it to scroll the right thing, since the number of rows being displayed is not actually constant...
![]() |
Assignee | |
Comment 10•15 years ago
|
||
Attachment #443266 -
Flags: review?(matspal)
![]() |
Assignee | |
Updated•15 years ago
|
Assignee: nobody → bzbarsky
![]() |
Assignee | |
Updated•15 years ago
|
Summary: page up/down shows strange behaviour on multiple select with css height → [FIX]page up/down shows strange behaviour on multiple select with css height
Comment 12•15 years ago
|
||
Comment on attachment 443266 [details] [diff] [review]
Proposed fix
>+ mNumDisplayRows = state.ComputedHeight() / rowHeight;
In my trunk Linux build, if I set height <= 23px in the example
I get mNumDisplayRows == 0 which reverses PG_UP/PG_DOWN.
I think the above line would be better as
mNumDisplayRows = NS_MAX(1, state.ComputedHeight() / rowHeight);
(not that that actually works, but that's a different bug --
I couldn't find it reported so I filed bug 563642)
r=mats with the NS_MAX
Regression test please?
Attachment #443266 -
Flags: review?(matspal) → review+
![]() |
Assignee | |
Comment 13•15 years ago
|
||
> mNumDisplayRows = NS_MAX(1, state.ComputedHeight() / rowHeight);
Ah, good catch. will do.
> Regression test please?
Excellent question. I have no idea how to write one for this...
Comment 14•15 years ago
|
||
Something like this works for me locally on Linux and OSX.
I'm submitting it to TryServer now (with the fixes) to see what it says.
(the test requires the fix in bug 563642 also)
And, yeah, please file a follow-up bug to calculate the displayed
rows dynamically. The test works around that by re-creating the frame
after setting the height.
![]() |
Assignee | |
Comment 15•15 years ago
|
||
So that test makes sure that mNumDisplayRows is 1, basically?
> file a follow-up bug to calculate the displayed rows dynamically
That should happen with the patch in this bug. Does it not?
Comment 16•15 years ago
|
||
(In reply to comment #15)
> So that test makes sure that mNumDisplayRows is 1, basically?
Without the fix, the test gives (in the !mMightNeedSecondPass block):
test0: mNumDisplayRows=15
test1: mNumDisplayRows=1
test2: mNumDisplayRows=0
test3: mNumDisplayRows=1
test4: mNumDisplayRows=0
with the attached patch:
test0: mNumDisplayRows=5
test1: mNumDisplayRows=5
test2: mNumDisplayRows=0
test3: mNumDisplayRows=5
test4: mNumDisplayRows=0
and the NS_MAX addition will fix the zeros to one.
> That should happen with the patch in this bug. Does it not?
You're right, I must have made some silly mistake when testing...
it's getting late here.
So, this is the test without re-creating the frames.
Updated•15 years ago
|
Attachment #443546 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 17•15 years ago
|
||
Oh, I see; you explicitly measure the height of an option and use that to set the heights. OK. That works. Do you want to just roll it into your patch for bug 563642? Or should I fold it into my patch and wait to push this until you push the other?
Comment 18•15 years ago
|
||
I can add it with bug 563642. Now that you point it out it would be
simpler to just use a <select> with five <option>s and see what the height
of that is, I'll try that instead.
![]() |
Assignee | |
Comment 19•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/bc9cb887ee79 with the NS_MAX(1, ...) thing.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Comment 20•15 years ago
|
||
Landed the test:
http://hg.mozilla.org/mozilla-central/rev/4b2141b01ffc
Flags: in-testsuite? → in-testsuite+
Updated•6 years ago
|
Component: Keyboard: Navigation → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•