Regression: On closed lists, page up/down works like arrow up/down
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
People
(Reporter: foss, Assigned: Jamie)
References
Details
(Keywords: access, regression)
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
|
Details | Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20180702075147 Steps to reproduce: 1) Open this test case in the URL bar: data:text/html,<select><option value="-1"> </option><option value="00">00</option><option value="01">01</option><option value="02">02</option><option value="03">03</option><option value="04">04</option><option value="05">05</option><option value="06">06</option><option value="07">07</option><option value="08">08</option><option value="09">09</option><option value="10">10</option><option value="11">11</option><option value="12">12</option><option value="13">13</option><option value="14">14</option><option value="15">15</option><option value="16">16</option><option value="17">17</option><option value="18">18</option><option value="19">19</option><option value="20">20</option><option value="21">21</option><option value="22">22</option><option value="23">23</option></select> 2) Click on the list (keep it closed) 3) Press page down Actual results: The currently selected item on the list moves on 00 like if I've done arrow down Expected results: The currently selected item on the list should move on 18 like it was in Firefox ESR 52 Additionnal information: If I open the list with alt + arrow down pressing page down works correctly.
Reporter | ||
Comment 1•6 years ago
|
||
This regression is annoying for keyboard users especially visual-impaired users that rely only on keyboard. Best regards, Alex.
Comment 2•6 years ago
|
||
I can confirm the issue with my firefox 61.0.1 on Linux, as well as with the nightly snapshot.
Comment 3•6 years ago
|
||
David, could you please help with that? Thanks
Comment 4•6 years ago
|
||
Hi, I managed to get a regression range for this issue using windows 10: Last good revision: ad16863d1d45bd3fd7906c76fa1ac1e12d24a133 (2015-12-22) First bad revision: 35b211eaad1fa828064514c547057e4400e24459 (2015-12-23) Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad16863d1d45bd3fd7906c76fa1ac1e12d24a133&tochange=35b211eaad1fa828064514c547057e4400e24459 Please note i've also been getting these errors during mozregression and it might not be that accurate: CRITICAL: First build ad16863d1d45 is missing, but mozregression can't find a build before - so it is excluded, but it could contain the regression! WARNING: Skipping build f7fbc524f9f3: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.revision.f7fbc524f9f378ab381f02412adceffed5f6a1c7.firefox.win64-opt' 8:21.51 CRITICAL: Last build 35b211eaad1f is missing, but mozregression can't find a build after - so it is excluded, but it could contain the regression! 8:28.65 WARNING: Skipping build 1954eb87782e: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.revision.1954eb87782e963a783284ede82fca7698c48a13.firefox.win64-opt' 8:28.70 WARNING: Skipping build 5ed5f6f57359: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.revision.5ed5f6f573597534d396a1e59298fd9c58a15ac9.firefox.win64-opt' 8:28.73 INFO: There are no build artifacts on inbound for these changesets (they are probably too old).
Comment 5•6 years ago
|
||
#c0 makes it sound like ESR52 worked OK, but the regression range in #c4 makes this sound like a much older issue. Would be nice to clarify that discrepancy.
Reporter | ||
Comment 6•6 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5) > #c0 makes it sound like ESR52 worked OK, but the regression range in #c4 > makes this sound like a much older issue. Would be nice to clarify that > discrepancy. Indeed, I'm currently using Firefox ESR 52 and I cannot reproduce the issue with it. As the issue has been reported to me by two people in CC (Samuel and Texou). I assume there is a strange things with the culprit commit. As I've never tried to track regression in Firefox I couldn't really help because I've not many time to dedicate to Firefox in the next month. Maybe MarcoZ could help us? Best regards, Alex.
Comment 7•6 years ago
|
||
Bumping my NI over to Jamie (new head of a11y).
Assignee | ||
Comment 8•6 years ago
|
||
This works as expected with e10s disabled. This suggests that the differences in the way <select size="1"> is rendered in e10s are the culprit here. That means trying to find an exact regression point probably isn't useful, since these controls probably didn't work at all in e10s without the changes. The more important question is what makes this misbehave in e10s. Bug 1229850 changed the <select> code so that dropdown size isn't calculated in the content process, since the dropdown is rendered in the parent. This also sets the number of display rows (mNumDisplayRows) to 1: https://searchfox.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#580 mNumDisplayRows is used to calculate the number of options to move by when using page up/page down: https://searchfox.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#2262 The simple solution here is to set mNumDisplayRows to some reasonable constant for content processes: 580 mNumDisplayRows = XRE_IsContentProcess() ? 15 : 1; I tested this and it seemed to work as expected. An alternative is to make this tweak in the handlers for page up/page down (in nsListControlFrame::KeyDown), which limits the risk. An open question: what is a reasonable constant? I'd say somewhere between 10 and 20 (inclusive). It's arbitrary, but we don't have anything else to go on here. On my system for this test case, page down moves 19 options forward when the dropdown is expanded (mNumDisplayRows is 20, since the key handlers subtract 1). The reporter (comment 0) notes the same. This will probably be different in different situations, but it seems like a good starting point. DBaron, how does this sound to you?
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to James Teh [:Jamie] from comment #8) > That means trying to find an exact regression point probably isn't > useful, FWIW, it was probably bug 1229850, for reasons given above. :)
Reporter | ||
Comment 10•6 years ago
|
||
(In reply to James Teh [:Jamie] from comment #9) > (In reply to James Teh [:Jamie] from comment #8) > > That means trying to find an exact regression point probably isn't > > useful, > > FWIW, it was probably bug 1229850, for reasons given above. :) What do you plan to do with this regression between non-e10s and e10s? Users ask me about this and I just want to know to inform them and decide or not to disable e10s for the time frame. Best regards, Alex.
Updated•6 years ago
|
Updated•5 years ago
|
Comment 11•5 years ago
|
||
Sean, can you help find an owner for this regression? Thanks!
Comment 12•5 years ago
|
||
Too late for 66 but we can still take a patch for 67.
Comment 13•5 years ago
|
||
Bulk change for all regression bugs with status-firefox67 as 'fix-optional' to be marked 'affected' for status-firefox68.
Updated•5 years ago
|
Comment 14•5 years ago
|
||
Asa, this bug seems to have fallen into the cracks, can it be prioritized please? Thanks
Comment 15•5 years ago
|
||
This does seem like something that would be good to unregress for keyboard users ASAP.
A year ago, in comment #8, Jamie asked for dbaron's thoughts on two possible solutions, clamping mNumDisplayRows to some reasonable constant, or tweak in the handlers for page up/page down (in nsListControlFrame::KeyDown). IMO, that's where we need to pick this back up.
Assignee | ||
Comment 16•5 years ago
|
||
The number of rows for the select dropdown can't be calculated in the content process because the dropdown has to be rendered in the parent process.
Therefore, we previously set the number of rows to 1 in this case.
That meant that the page up and page down keys only moved one item at a time for a collapsed select control, making them effectively useless.
Instead, set the number of rows to the maximum in this case.
Comment 17•5 years ago
|
||
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f3fff7c1f2ba For collapsed select controls in the content process, set the number of rows to the maximum instead of 1. r=dholbert
Comment 18•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Hi,
Many thanks for this great job! The bug is fixed.
@Joanmarie: now the thing is that when I switch to focus mode, pgdn just scrolls one line. It scrolls 20 lines only if I pres alt-down arrow first. Is it a Firefox issue or can Orca help?
regards
Comment 21•5 years ago
|
||
oops I have just checked again and now it works perfectly! Great and sorry for the noise. Bug fixed, great, congrats and thanks!
Comment 22•5 years ago
|
||
Is a non-regressino test planned to avoid this bug to come back?
Regards
Comment 23•5 years ago
|
||
Seems like https://hg.mozilla.org/mozilla-central/rev/f3fff7c1f2ba includes a test.
Assignee | ||
Comment 24•5 years ago
|
||
Yes, the test I landed with the patch should prevent this from regressing.
Comment 25•5 years ago
|
||
Is this something we should consider uplifting to Beta or can it ride Fx71 to release?
Assignee | ||
Comment 26•5 years ago
|
||
Comment on attachment 9091347 [details]
Bug 1488828: For collapsed select controls in the content process, set the number of rows to the maximum instead of 1.
Beta/Release Uplift Approval Request
- User impact if declined: Inefficient keyboard functionality in collapsed HTML select controls (regressed in Firefox 57). Several users were actively requesting a fix.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Very straightforward patch covered by automated tests.
- String changes made/needed: None.
Assignee | ||
Updated•5 years ago
|
Comment 27•5 years ago
|
||
Comment on attachment 9091347 [details]
Bug 1488828: For collapsed select controls in the content process, set the number of rows to the maximum instead of 1.
Better keyboard control for collapsed menu selection, let's uplift for beta 9.
Comment 28•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Comment 29•5 years ago
|
||
Hello!
Reproduced the issue with Firefox 64.0a1 (20180905223809) on Windows 10x64.
The issue is verified fixed with Firefox 71.0a1 (20190923215658) and Firefox 70.0b9 (20190923154733) on Windows 10x64, Ubuntu 18.04 and macOS 10.14.
Description
•