1488828 - Regression: On closed lists, page up/down works like arrow up/down

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Alex ARNAUD]

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">&nbsp;</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.

Comment 1

[Alex ARNAUD]

This regression is annoying for keyboard users especially visual-impaired users that rely only on keyboard.

Best regards,
Alex.

Comment 2

[Samuel Thibault]

I can confirm the issue with my firefox 61.0.1 on Linux, as well as with the nightly snapshot.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

David, could you please help with that? Thanks

Comment 4

[Rares Doghi, Desktop QA]

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

[Ryan VanderMeulen [:RyanVM]]

#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.

Comment 6

[Alex ARNAUD]

(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

[David Bolter [:davidb] (NeedInfo me for attention)]

Bumping my NI over to Jamie (new head of a11y).

Comment 8

[James Teh [:Jamie]]

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?

Comment 9

[James Teh [:Jamie]]

(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. :)

Comment 10

[Alex ARNAUD]

(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.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Sean, can you help find an owner for this regression?  Thanks!

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 66 but we can still take a patch for 67.

Comment 13

[Neha Kochar [:neha]]

Bulk change for all regression bugs with status-firefox67 as 'fix-optional' to be marked 'affected' for status-firefox68.

Comment 14

[Pascal Chevrel:pascalc]

Asa, this bug seems to have fallen into the cracks, can it be prioritized please? Thanks

Comment 15

[Asa Dotzler [:asa]]

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.

Comment 16

[James Teh [:Jamie]]

Attachment: Bug 1488828: For collapsed select controls in the content process, set the number of rows to the maximum instead of 1.

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

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f3fff7c1f2ba

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 20

[Jean-Philippe MENGUAL]

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

[Jean-Philippe MENGUAL]

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

[Jean-Philippe MENGUAL]

Is a non-regressino test planned to avoid this bug to come back?

Regards

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Seems like https://hg.mozilla.org/mozilla-central/rev/f3fff7c1f2ba includes a test.

Comment 24

[James Teh [:Jamie]]

Yes, the test I landed with the patch should prevent this from regressing.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta or can it ride Fx71 to release?

Comment 26

[James Teh [:Jamie]]

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.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6811da275b66

Comment 29

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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.