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)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 10

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Keywords: polish, Whiteboard: [pushed])

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — 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)
Whiteboard: [has patch]
Blocks: 689706
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-
Attached patch patch v2Splinter Review
Addresses review comments.
Attachment #563124 - Attachment is obsolete: true
Attachment #564319 - Flags: review?(wjohnston)
Attachment #564319 - Flags: review?(wjohnston) → review+
I'll land my tests for this code today. You mind adding a test?
(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]
Whiteboard: [has patch][needs tests] → [has patch]
Attached patch testSplinter Review
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/73aa31834297
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.

Attachment

General

Created:
Updated:
Size: