739757 - Audit all the popups

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Our doorhangers are a bit all over the place, so we should make them consistent. Consistent in design, in language, in interaction, etc

Let's use this bug to iron out the differences and make them more awesome.

Comment 1

[:Margaret Leibovic]

This should help you find all the doorhanger popups:
http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/&string=doorhanger.show

Comment 2

[Aaron Train [:aaronmt]]

Soft-block 1.0? Or future tracking?

Comment 3

[Ian Barlow (:ibarlow)]

I don't think this should soft block 1.0, I predict a lot of string changes to get this right.

Comment 4

[Ian Barlow (:ibarlow)]

Just a note to self to account for an "always share geolocation for this site" option, as is being discussed in bug 729485

Comment 5

[:Margaret Leibovic]

Ian, this was bothering me yesterday, so I made an etherpad that describes the current popups:
https://etherpad.mozilla.org/fennec-popups

We should go through the list to decide what messages/buttons/checkboxes we want, and I can whip up some patches!

Comment 6

[:Margaret Leibovic]

Ian, ping? If you can take a look at that etherpad and make some decisions about what our notifications should look like, this could be a good mentor bug.

Comment 7

[:Margaret Leibovic]

Attachment: "Remember" text awkwardly wrapped

I discovered that the "Remember" button text for password notifications on my tablet wraps awkwardly.

If we get rid of "Not Now" buttons, this issue would be fixed :)

I vote that we only ever have two buttons in a popup, and that we use checkboxes for the always/never functionality.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

(In reply to Margaret Leibovic [:margaret] from comment #7)
that is bug 715572

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Margaret Leibovic [:margaret] from comment #6)
> Ian, ping? If you can take a look at that etherpad and make some decisions
> about what our notifications should look like, this could be a good mentor
> bug.

Yup, I actually just moved this back up our UX priority list last week. Eric and I are looking into our menu situation, we should have some updates for you soon!

Comment 10

[Sriram Ramasubramanian [:sriram]]

(For anyone working on this) Please define it under themes as a style like: http://developer.android.com/reference/android/R.attr.html#dialogTheme
Please refer to the android source code's attrs.xml to find what all can be configured here. This is a better to add it.

Comment 11

[:Margaret Leibovic]

I filed this bug about just sorting out the string for messages/buttons for all the popups to make them feel consistent, so I don't think there will be theme work involved in this. We should do that part of the doorhanger revamp in a separate bug.

Comment 12

[:Margaret Leibovic]

I want to pick this back up to organize the doorhanger notification messages/buttons (Sriram is working on updating their styles separately).

Ian, maybe we can sync up about this at the work week.

Comment 13

[Aaron Train [:aaronmt]]

I saw Sriram post a mockup of them redone yesterday which will fix this bug

Comment 14

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #13)
> I saw Sriram post a mockup of them redone yesterday which will fix this bug

That is for the styling, not for the wording (see previous comment). That should happen in a different bug.

Comment 15

[:Margaret Leibovic]

Attachment: (Part 1) Update strings for offline apps/indexedDB notifications

I updated the strings for a bunch of notifications according to what ibarlow and I worked out on this etherpad: https://etherpad.mozilla.org/fennec-popups

I still need to find a good indexedDB test page, since I think we now only prompt for increasing a site's data quota. For offline apps I used http://html5demos.com/offlineapp.

Comment 16

[:Margaret Leibovic]

Attachment: (Part 2) Update strings for content permissions prompts

Test pages for content permissions perms:
http://people.mozilla.com/~mleibovic/test/geo.html
https://developer.mozilla.org/samples/domref/desktopnotification.html

I also forgot to mention that I made sure to update the site settings dialog to use these new strings (since a lot of them are shared).

Comment 17

[:Margaret Leibovic]

Attachment: (Part 3) Update click to play plugin strings to use "activate" instead of "play"

Test page: http://people.mozilla.org/~mwargers/tests/plugins/flash/flashembed_invisible.html

(Note: your device needs to have Flash installed to test)

Comment 18

[:Margaret Leibovic]

Attachment: (Part 4) Update popup blocked notifications

Test page: http://www.popuptest.com/

Comment 19

[:Margaret Leibovic]

Attachment: (Part 5) Update telemetry prompt strings

This doesn't have a test page, since it's our own prompt, but this is a tiny string swap.

Comment 20

[:Margaret Leibovic]

Attachment: (Part 6) Update login manager strings

I also got rid of some strings/things we weren't using in here.

I just used twitter's login page to test this.

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12b6443f6bed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5239c5771d
https://hg.mozilla.org/integration/mozilla-inbound/rev/17ab91e3a0c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/38c9642dfab0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3637227e2fee
https://hg.mozilla.org/integration/mozilla-inbound/rev/14893b300874

Comment 22

[:Margaret Leibovic]

Ugh, testDoorhanger depends on the current strings/buttons. I'm going to need to update that. Backed this out for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f185ed5f165d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6be3e974c65
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1bfaab3957
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e2839de3f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6418f191dbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/42845185ca07

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I don't know how you normally do backouts, but whatever you did resulted in your last cset being empty, which of course created even more bustage. Please use qbackout in the future.

Backed out for real this time:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b7c3c50226

Comment 24

[:Margaret Leibovic]

Attachment: (Part 7) Update testDoorHanger to work with new strings

I factored out the strings in the test while I was in here, and I had to s/clickOnText/clickOnButton/ because the button text now also appears in the message text, and this was causing us to click on the wrong thing (that was annoying for me to figure out!).

Pushed to try to make sure everything works out:
https://tbpl.mozilla.org/?tree=Try&rev=2132afbe4c89

Comment 25

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 723688 [details] [diff] [review]
(Part 7) Update testDoorHanger to work with new strings

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

This looks great! ...but it fails on try.

::: mobile/android/base/tests/testDoorHanger.java.in
@@ +47,5 @@
>  
>          // Test "Share" button hides the notification
>          mSolo.clickOnCheckBox(0);
> +        mSolo.clickOnButton(GEO_ALLOW);
> +        mAsserter.is(mSolo.searchText(GEO_MESSAGE), false, "Geolocation doorhanger has been hidden");

Seems to be failing here. Do we need a short pause after clicking on the button, to avoid finding the doorhanger from the previous step? Or is GEO_MESSAGE located somewhere else??

Comment 26

[:Margaret Leibovic]

Oops, I drafted a comment but forgot to post it. I accidentally pushed to try without a qref, so it's my old patch that was failing :)

The correct try push is passing:
https://tbpl.mozilla.org/?tree=Try&rev=d7dccce5254a

Comment 27

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 723688 [details] [diff] [review]
(Part 7) Update testDoorHanger to work with new strings

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

That's awesome then!

Comment 28

[:Margaret Leibovic]

I folded the patches together to land this time:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ec9c5b8da4

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/36ec9c5b8da4

Comment 30

[Francesco Lodolo [:flod]]

That's a huge amount of strings to digest ;-)

    2.17 +popup.warning=%S prevented this site from opening a pop-up window. Would you like to show it?
    2.18 +popup.warningMultiple=%S prevented this site from opening %S pop-up windows. Would you like to show them?

Why did you use this form instead of the usual plural form? Isn't it supported on Android?

https://developer.mozilla.org/en/docs/Localization_and_Plurals

Comment 31

[:Margaret Leibovic]

(In reply to Francesco Lodolo [:flod] from comment #30)
> That's a huge amount of strings to digest ;-)
> 
>     2.17 +popup.warning=%S prevented this site from opening a pop-up window.
> Would you like to show it?
>     2.18 +popup.warningMultiple=%S prevented this site from opening %S
> pop-up windows. Would you like to show them?
> 
> Why did you use this form instead of the usual plural form? Isn't it
> supported on Android?
> 
> https://developer.mozilla.org/en/docs/Localization_and_Plurals

Yeah, sorry about all the string changes :/

Regarding plural form, it looks like this was copied over from desktop's implementation, which doesn't use plural form:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#428

That code in ancient, so that may have happened before we had our current plural form support. We could file a bug to update that, if that would make things easier for localizers.

Comment 32

[Francesco Lodolo [:flod]]

(In reply to :Margaret Leibovic from comment #31)
> That code in ancient, so that may have happened before we had our current
> plural form support. We could file a bug to update that, if that would make
> things easier for localizers.

CCing Axel, I personally think it would be a good thing but I want to be sure I'm not missing some technical limit.

Comment 33

[Axel Hecht [:Pike]]

Seems both impls are in js, so they should use the plural forms, filing a new bug sounds like a good idea.