950373 - Up and Down Keyboard Arrows no longer work in the Restore Session frame / box

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[rojocru]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

Open Firefox in such a way that it prompts you to restore a session.


Actual results:

At the Restore Session Page, in the box below, pressing the up and down arrows on my keyboard does nothing. The keys do work, but not in that box.


Expected results:

When I press the keys, the selection should change. Pressing the up key should choose an item above the item I selected and pressing the down key should choose the next selection below.

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/a4c1961bf723
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724194643
Bad:
http://hg.mozilla.org/mozilla-central/rev/2e27eaf8ebc2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130725 Firefox/25.0 ID:20130725083141
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4c1961bf723&tochange=2e27eaf8ebc2

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/12c11406ed75
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724230740
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/949a5e125a8d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724230940
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12c11406ed75&tochange=949a5e125a8d

Regressed by:Bug 501496



I wander that mozilla do not provide such basic function automate test :(:(:(

Comment 2

[Alice0775 White]

s/wander/wonder/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, it's odd...

<tree> handles keypress event for non-printable keys. We should change them keydown event:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tree.xml#872

However, I find this odd code...
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/aboutSessionRestore.js#169
According to the comment, session restore consumes the keys which are necessary for navigating <tree> for preventing scroll of the page. All key events which is fired on descendant elements shouldn't cause page scroll...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 XUL <tree> should handle non-printable keys with keydown event handler instead of keypress event handler

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Don't consume keys which are used by <tree> for navigation in session restore UI

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=3a0d519001ea

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8371310 [details] [diff] [review]
part.1 XUL <tree> should handle non-printable keys with keydown event handler instead of keypress event handler

As I posted to dev-platform, non-printable keys should be handled in keydown event handlers. There are two reasons:

1. keypress event is now not dispatched if preceding keydown event is consumed.
2. keypress event shouldn't be fired if the key event doesn't cause text input. This is defined by DOM Level 3 Events.

So, handling non-printable keys with keydown event is smarter/safer.

Additionally, VK_ENTER is never used. So, we can remove the event handler.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8371312 [details] [diff] [review]
part.2 Don't consume keys which are used by <tree> for navigation in session restore UI

aboutSessionRestore.js shouldn't consume the key events which are handled by the <tree>. I confirmed that arrow keys in <tree> doesn't cause scroll of the page.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b57f9be43a50
https://hg.mozilla.org/integration/mozilla-inbound/rev/b724cd142b51

Comment 11

[[e-mail]]

(In reply to Masayuki Nakano from comment #4)
> However, I find this odd code...
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/content/aboutSessionRestore.js#169
> According to the comment, session restore consumes the keys which are
> necessary for navigating <tree> for preventing scroll of the page. All key
> events which is fired on descendant elements shouldn't cause page scroll...

This used to be a problem before it was fixed in bug 655004.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b57f9be43a50
https://hg.mozilla.org/mozilla-central/rev/b724cd142b51

Comment 13

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-01-08.
Verified fixed in FF 30.0a1 (2014-03-05), Win 7 x64.

Comment 14

[rojocru]

Hello. I am the one who originally posted about this bug. I am sorry if what I am asking is answered here, but everything here seems very technical.

My Questions are

1) So, this problem has been fixed?
2) If Yes, I should see the fix in Firefox 30?
3) And if yes, when will Firefox 30 be released?

Also, why couldn't this be fixed for the current version, version 27 or one before 30--i.e. 28 or 29?

And if it's not too much trouble, could someone explain in very basic layman terms what happened to cause this problem.

Comment 15

[Paul Silaghi, QA [:pauly]]

Hi Rojocru,

(In reply to rojocru from comment #14)
> 1) So, this problem has been fixed?
Yes

> 2) If Yes, I should see the fix in Firefox 30?
Yes

> 3) And if yes, when will Firefox 30 be released?
2014-06-10 https://wiki.mozilla.org/RapidRelease/Calendar
 
> Also, why couldn't this be fixed for the current version, version 27 or one
> before 30--i.e. 28 or 29?
I guess it's not important enough to be pushed to lower versions
 
> And if it's not too much trouble, could someone explain in very basic layman
> terms what happened to cause this problem.
I leave this to Masayuki

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Paul Silaghi, QA [:pauly] from comment #15)
> > Also, why couldn't this be fixed for the current version, version 27 or one
> > before 30--i.e. 28 or 29?
> I guess it's not important enough to be pushed to lower versions

Yes. And it's risky to uplift since it changes all <tree> element's behavior. So, some addons and other part of our product may have regressions.

> > And if it's not too much trouble, could someone explain in very basic layman
> > terms what happened to cause this problem.
> I leave this to Masayuki

It's hard to explain the cause of this bug without technical terms... We're changing each event behavior if it's different from other browsers and standard spec of DOM Level 3 Events. Such changes may cause regressions like this if it's implemented as unsafe or buggy approach.