Closed Bug 950373 Opened 8 years ago Closed 8 years ago

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

Categories

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

25 Branch
x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox26 --- affected
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
firefox30 --- verified
firefox-esr24 --- unaffected

People

(Reporter: rojocru, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Session Restore
Ever confirmed: true
Keywords: regression
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 :(:(:(
Blocks: 501496
Severity: normal → major
Component: Session Restore → Event Handling
Product: Firefox → Core
s/wander/wonder/
Duplicate of this bug: 967945
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 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.
Attachment #8371310 - Flags: review?(enndeakin)
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.
Attachment #8371312 - Flags: review?(enndeakin)
Attachment #8371310 - Flags: review?(enndeakin) → review+
Attachment #8371312 - Flags: review?(enndeakin) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/b57f9be43a50
https://hg.mozilla.org/mozilla-central/rev/b724cd142b51
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Reproduced in Nightly 2014-01-08.
Verified fixed in FF 30.0a1 (2014-03-05), Win 7 x64.
Status: RESOLVED → VERIFIED
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.
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
(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.
Duplicate of this bug: 987083
Depends on: 990512
Depends on: 1007680
No longer depends on: 990512
Depends on: 1247476
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.