Closed Bug 900608 Opened 7 years ago Closed 7 years ago

Guest Browsing: String changes

Categories

(Firefox for Android :: General, defect)

24 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: ibarlow, Assigned: wesj)

Details

Attachments

(1 file, 1 obsolete file)

We made some tweaks to the menu and dialog strings for guest browsing (designs: http://cl.ly/image/2C1k1V0w3U0S/o). They are below:


* Menu item for entering guest browsing * 

New Guest Session



* Dialog for entering guest browsing *

[title] Firefox will now restart

[text] The person using it will not be able to see any of your personal browsing data (like saved passwords, history or bookmarks). 

When your guest is done, their browsing data will be deleted and your session will be restored.

[buttons] Cancel / Continue



* Menu item for leaving guest browsing * 

Exit Guest Session



* Dialog for leaving guest browsing *


[title] Firefox will now restart

[text] The browsing data from this session will be deleted. 

[buttons] Cancel / Continue
Attached patch Patch (obsolete) — Splinter Review
This is one of the few times I love our crazy string setup.
Attachment #784668 - Flags: review?(margaret.leibovic)
Attached patch Patch v2Splinter Review
This renames all the entities to use guest_session instead of guest_mode.
Attachment #784668 - Attachment is obsolete: true
Attachment #784668 - Flags: review?(margaret.leibovic)
Attachment #784692 - Flags: review?(margaret.leibovic)
Attachment #784692 - Attachment is patch: true
Comment on attachment 784692 [details] [diff] [review]
Patch v2

Review of attachment 784692 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +1798,5 @@
>  
>          Resources res = getResources();
>          ps.setButtons(new String[] {
> +            res.getString(R.string.guest_session_dialog_continue),
> +            res.getString(R.string.guest_session_dialog_cancel)

Should these be revered? In ibarlow's comment he specified the buttons as...

  [buttons] Cancel / Continue

...which makes me think Cancel should come first.

But that's not hard to reverse in a follow-up, since it doesn't depend on strings.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +361,3 @@
>  
> +<!ENTITY exit_guest_session_title "&brandShortName; will now restart">
> +<!ENTITY exit_guest_session_text "The browsing data from this session will be deleted.">

Bonus points for some localization comments about when we show these strings.
Attachment #784692 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #3)

> >          Resources res = getResources();
> >          ps.setButtons(new String[] {
> > +            res.getString(R.string.guest_session_dialog_continue),
> > +            res.getString(R.string.guest_session_dialog_cancel)
> 
> Should these be revered? In ibarlow's comment he specified the buttons as...
> 
>   [buttons] Cancel / Continue
> 
> ...which makes me think Cancel should come first.

Remember that ICS and higher likes to flip the order of the buttons: "negative" | "positive"
Gingerbread did it to the other way: "positive" | "negative"

I think the way Wes has it will yield: "negative" | "positive" on ICS and higher.
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> 
> > >          Resources res = getResources();
> > >          ps.setButtons(new String[] {
> > > +            res.getString(R.string.guest_session_dialog_continue),
> > > +            res.getString(R.string.guest_session_dialog_cancel)
> > 
> > Should these be revered? In ibarlow's comment he specified the buttons as...
> > 
> >   [buttons] Cancel / Continue
> > 
> > ...which makes me think Cancel should come first.
> 
> Remember that ICS and higher likes to flip the order of the buttons:
> "negative" | "positive"
> Gingerbread did it to the other way: "positive" | "negative"
> 
> I think the way Wes has it will yield: "negative" | "positive" on ICS and
> higher.

Cool, I didn't actually look at the implementation of setButtons, just something I wanted to make us double check :)
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> 
> > >          Resources res = getResources();
> > >          ps.setButtons(new String[] {
> > > +            res.getString(R.string.guest_session_dialog_continue),
> > > +            res.getString(R.string.guest_session_dialog_cancel)
> > 
> > Should these be revered? In ibarlow's comment he specified the buttons as...
> > 
> >   [buttons] Cancel / Continue
> > 
> > ...which makes me think Cancel should come first.
> 
> Remember that ICS and higher likes to flip the order of the buttons:
> "negative" | "positive"
> Gingerbread did it to the other way: "positive" | "negative"
> 
> I think the way Wes has it will yield: "negative" | "positive" on ICS and
> higher.

Yeah. This is kinda a deficiency in using our PromptService instead of just AlertDialog.Builder, but Android has a setPositiveButton/setNegative/setNeutral. For the prompt service, I just assumed that if there was one button is was probably positive, two were probably positive and negative, and three had a neutral state. i.e. the positive option should be first when using this api.

https://hg.mozilla.org/integration/fx-team/rev/4e2605c97d4a
https://hg.mozilla.org/mozilla-central/rev/4e2605c97d4a
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Can I nitpick something here?


'The person using it ...' - What is it referring to here? The browser? This should use & brandShortName; then.
I said "it" since we just identified the subject in the dialog title (Firefox will now restart). 

But I'm open to Aaron's suggestion.
You need to log in before you can comment on or make changes to this bug.