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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: jelle, Assigned: bzbarsky)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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.
Confirmed on 2.0.0.1/Linux/PPC.
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
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.
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...
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.
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.
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...
Attached patch Proposed fixSplinter Review
Attachment #443266 - Flags: review?(matspal)
Assignee: nobody → bzbarsky
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 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+
> 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...
Attached patch WIP mochitest (obsolete) — Splinter Review
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.
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?
Attached patch mochitest.diffSplinter Review
(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.
Attachment #443546 - Attachment is obsolete: true
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?
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.
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
Flags: in-testsuite? → in-testsuite+
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: