Closed
Bug 690022
Opened 13 years ago
Closed 13 years ago
Android back (escape) key should close the locale picker
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 10
People
(Reporter: mbrubeck, Assigned: mbrubeck)
References
Details
(Keywords: polish, Whiteboard: [pushed])
Attachments
(2 files, 1 obsolete file)
2.27 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce: 1. Open the Fennec preferences 2. Press the language button to open the locale picker 3. Press the system "back" button (on Android) or "escape" key (desktop) Expected results: Locale picker closes. Actual results: Nothing happens.
Attachment #563124 -
Flags: review?(wjohnston)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has patch]
Comment 1•13 years ago
|
||
Comment on attachment 563124 [details] [diff] [review] patch Review of attachment 563124 [details] [diff] [review]: ----------------------------------------------------------------- So I'm a bit worried about how this will work/feel with the different panes in the locale picker. For startup, we have an "intro" pane that will say "Loading" while we check if there are any locales on AMO that will work for your system and "Continue In English"/"Select a different language" if we can't find one. Then there's the main picker, and then there's the "Installing" one. I think maybe we put a switch statement in here instead (yes, I've somehow screwed up the case on pickerpage)? switch(this.selectedPanel) { case this.pickerpage: this.cancelPicker(); // we can move the cancelInstall in here, although I'm not sure its needed... cancelPicker will close the window if this is not startup being shown at startup case this.mainPage: this.closeWindow(); // this will open the browser window during startup... don't think we want that? case this.installerPage: this.cancelInstall(); this.showPicker(); } I'm not 100% sure what sort of granularity people expect out of back. But I think this will work.
Attachment #563124 -
Flags: review?(wjohnston) → review-
Assignee | ||
Comment 2•13 years ago
|
||
Addresses review comments.
Attachment #563124 -
Attachment is obsolete: true
Attachment #564319 -
Flags: review?(wjohnston)
Updated•13 years ago
|
Attachment #564319 -
Flags: review?(wjohnston) → review+
Comment 3•13 years ago
|
||
I'll land my tests for this code today. You mind adding a test?
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #3) > I'll land my tests for this code today. You mind adding a test? Sure, I can add a test.
Whiteboard: [has patch] → [has patch][needs tests]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has patch][needs tests] → [has patch]
Assignee | ||
Comment 5•13 years ago
|
||
I tried to keep this test as simple as possible, so it just pokes LocaleUI directly to test starting from the various pages.
Attachment #565311 -
Flags: review?(wjohnston)
Comment 6•13 years ago
|
||
Comment on attachment 565311 [details] [diff] [review] test Review of attachment 565311 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with simple :) Just keep me from breaking it again
Attachment #565311 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 7•13 years ago
|
||
Folded the patches into one changeset and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/73aa31834297
Flags: in-testsuite+
Whiteboard: [has patch] → [pushed]
Target Milestone: --- → Firefox 10
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/73aa31834297
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 9•13 years ago
|
||
This issue is verified fixed on build: Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111009 Firefox/10.0a1 Fennec/10.0a1 Device: HTC Desire OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•