Closed Bug 739757 Opened 8 years ago Closed 7 years ago

Audit all the popups

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox13 --- affected
firefox14 --- affected

People

(Reporter: ibarlow, Assigned: Margaret)

References

Details

Attachments

(8 files)

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.
This should help you find all the doorhanger popups:
http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/&string=doorhanger.show
OS: Mac OS X → Android
Hardware: x86 → ARM
Soft-block 1.0? Or future tracking?
I don't think this should soft block 1.0, I predict a lot of string changes to get this right.
Just a note to self to account for an "always share geolocation for this site" option, as is being discussed in bug 729485
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!
Assignee: nobody → 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.
Version: Firefox 14 → Trunk
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.
(In reply to Margaret Leibovic [:margaret] from comment #7)
that is bug 715572
(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!
(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.
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.
Assignee: margaret.leibovic → nobody
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.
Assignee: nobody → margaret.leibovic
I saw Sriram post a mockup of them redone yesterday which will fix this bug
(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.
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.
Attachment #721191 - Flags: review?(mark.finkle)
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).
Attachment #721194 - Flags: review?(mark.finkle)
This doesn't have a test page, since it's our own prompt, but this is a tiny string swap.
Attachment #721197 - Flags: review?(mark.finkle)
I also got rid of some strings/things we weren't using in here.

I just used twitter's login page to test this.
Attachment #721198 - Flags: review?(mark.finkle)
Attachment #721196 - Flags: review?(mark.finkle)
Attachment #721191 - Flags: review?(mark.finkle) → review+
Attachment #721194 - Flags: review?(mark.finkle) → review+
Attachment #721195 - Flags: review?(mark.finkle) → review+
Attachment #721196 - Flags: review?(mark.finkle) → review+
Attachment #721197 - Flags: review?(mark.finkle) → review+
Attachment #721198 - Flags: review?(mark.finkle) → review+
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
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
Attachment #723688 - Flags: review?(gbrown)
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??
Attachment #723688 - Flags: review?(gbrown) → review-
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 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!
Attachment #723688 - Flags: review- → review+
I folded the patches together to land this time:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ec9c5b8da4
https://hg.mozilla.org/mozilla-central/rev/36ec9c5b8da4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
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
(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.
(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.
Seems both impls are in js, so they should use the plural forms, filing a new bug sounds like a good idea.
Duplicate of this bug: 715572
You need to log in before you can comment on or make changes to this bug.