Android back (escape) key should close the locale picker

VERIFIED FIXED in Firefox 10

Status

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

(Blocks: 1 bug, {polish})

Trunk
Firefox 10
All
Android
polish
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

(Whiteboard: [pushed])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 563124 [details] [diff] [review]
patch

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

7 years ago
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-
(Assignee)

Comment 2

7 years ago
Created attachment 564319 [details] [diff] [review]
patch v2

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?
(Assignee)

Comment 4

7 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

7 years ago
Whiteboard: [has patch][needs tests] → [has patch]
(Assignee)

Comment 5

7 years ago
Created attachment 565311 [details] [diff] [review]
test

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+
(Assignee)

Comment 7

7 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
https://hg.mozilla.org/mozilla-central/rev/73aa31834297
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.