Closed Bug 488218 Opened 11 years ago Closed 11 years ago

geolocation ui changes

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [icon-3.1] [icon-needed])

Attachments

(7 files, 10 obsolete files)

6.43 KB, patch
Details | Diff | Splinter Review
22.19 KB, image/png
beltzner
: ui-review+
Details
22.91 KB, image/png
Details
21.32 KB, image/png
Details
947 bytes, image/png
Details
960 bytes, image/png
Details
18.03 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
Adding "more info" and "remember this decision" functionality
Attached image screenshot (obsolete) —
Comment on attachment 372518 [details] [diff] [review]
possible impl. requiring no string changes

the calls to setPagePermission should be uri's not strings.  fixed locally.
Axel: this is one potential approach; as the patch says, it's doing an icky thing by pulling in from other bundles, so I wanted to get your thoughts on it early. Not a consideration for b4, so there's no deadline pressure in the immediate sense.
Doug: I think I'd prefer an "Always" button as opposed to a checkbox, as otherwise we get a real UI mishmash in there. This might be a good opportunity, as well, to use the fact that infobars don't always have to be 1-line high. Maybe something like:

%s wants to know your location (Tell them) (Don't tell them) (Always tell them)
                                                                _Learn more..._

Also, we should probably be using the same formatting as the XPI install infobar does for the site name, just showing the domain (so people.mozilla.org) not the full URL.
Mike:

Using a "always button", you can not express the state where the user never wants to be asked against for a particulate site.

I agree about the multi-line thing.  my xul-css fu failed and/or i didn't want to change the notification xbl to make this easier.

I was thinking of:

%s wants to know your location.                          _Learn more..._
          (Tell them) (Don't tell them)    [] Remember this

Also, i think XPIs did it wrong. :-) For example, sites.google.com/baduser/ would appear as "sites.google.com".  (Maybe this horse has both left the barn and is dead.)


Also, maybe this should be a b4 consideration.  It provides a big usability win with little risk.
After speaking with Axel, he's pretty sure that we'll be able to get the localization on these strings post b4, and his preferred approach is to:

 - get a patch with hardcoded en-US strings for b4
 - file a follow up bug to switch to localized strings for post-b4

(In reply to comment #6)
> Using a "always button", you can not express the state where the user never
> wants to be asked against for a particulate site.

D'oh, good point.

> I agree about the multi-line thing.  my xul-css fu failed and/or i didn't want
> to change the notification xbl to make this easier.
> 
> I was thinking of:
> 
> %s wants to know your location.                          _Learn more..._
>           (Tell them) (Don't tell them)    [] Remember this

I don't think I like the center align, and would rather that the _Learn more_ be aligned under the thing that you'll learn more about, so:

%s wants to know your location    (Tell them) (Don't tell them)  [ ] Remember
_Learn more..._

Need to think of a better string than "Remember" though. Maybe "Remember for this site" like with passwords?

> Also, i think XPIs did it wrong. :-) For example, sites.google.com/baduser/
> would appear as "sites.google.com".  (Maybe this horse has both left the barn
> and is dead.)

That's really sites.google.com's responsibility, IMO. The rest of our site-specific preferences are by eTLD, and I would assume that this would be as well, right? Also, I really don't think that the majority of users will understand that fullURL is different from eTLD for the purposes of sharing their location.

cc: faaborg for his input/thoughts; alex, quick turnaround here, so get your licks in quickly!
gavin, can you give me a hand here?  I had a bit of xulpain trying to get two lines in the notification hbox.
>cc: faaborg for his input/thoughts; alex, quick turnaround here, so get your
>licks in quickly!

For some reason our notification bars don't usually follow the normal conventions for dialog buttons, but I think the rationale still makes sense.

Action button: describe the action on the button itself so the user can make a choice without having to read anything else, so "print" instead of "ok," or in this case: "send location"

Cancel button: should always be "cancel" (as opposed to "don't print" or "don't send location"), so it is easy to find, and you can click it without having to read anything else or even know what the bar/dialog was asking about in the first place.

Going beyond a single line might break some themeing on OS X, so best to check to be sure (we would need to file a follow up bug to fix that).  Also, on OS X you can't use Aqua style controls in chrome on OS X (the checkbox), we can re purpose the HUD style controls used in the bookmarks contextual dialog:

http://mxr.mozilla.org/seamonkey/source/browser/themes/pinstripe/browser/hud-style-check-box-empty.png

http://mxr.mozilla.org/seamonkey/source/browser/themes/pinstripe/browser/hud-style-check-box-checked.png

In general I think it would be better to try to keep this on a single line, matching all of the other infobars in the app.
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #372517 - Attachment is obsolete: true
Attachment #372518 - Attachment is obsolete: true
Attachment #372771 - Flags: review?
Attached image screenshot (with patch v.1) (obsolete) —
actually, the text in the notification bar should just read "Learn More.".
learn more shouldn't be yellow, and should be underlined to indicate it behaves like a hyperlink.
Comment on attachment 372772 [details]
screenshot (with patch v.1)

WHy yellow instead of blue? If it behaves like a web link, it should look like a web link, IMO, though I suppose we can play with styling after b4 so I'm not too concerned. If we're putting it all on one line, though, I think that it needs to be in (parens) though, so:

%s blah blah. (Learn more...)

"Remember this decision" should be "Remember for this site".

I would really (really) rather that the page address be eTLD, as mentioned.
new patch+screenshot coming up.
Attached image screenshot (with patch v.2) (obsolete) —
Attachment #372771 - Attachment is obsolete: true
Attachment #372772 - Attachment is obsolete: true
Attachment #372771 - Flags: review?
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #372785 - Flags: review?
1. using eTLD in the notification box
2. no more yellow.  using the right class name to get the URL color


This is alot simpler patch.  Turns out I am hitting some sort of xbl bug in Minefield that is preventing me from placing the "Learn more" text after the message description (if you look a the first screenshot, you will see that the bright yellow text was immediately after the message text). The same code works fine in 1.9.1.

Does having the "learn more" link where it is placed in screenshot v.2 acceptable?  I am not sure it is -- and maybe we should have it placed right in 1.9.1, and figure out what is wrong on the trunk later.
Comment on attachment 372784 [details]
screenshot (with patch v.2)

OIC why blue doesn't work. Right. On grey. I should have thought of that.

This is looking much better. Not sure why the (Learn more...) moved all the way to the right (I was fine with where it was in the previous screenshot). Let's try:

 - moving "(Learn more...)" back to being right after "%s wants to know your location"

 - making "(Learn more...)" white, but underlined, and the same smaller text size
Attached patch patch v.3Splinter Review
Attachment #372784 - Attachment is obsolete: true
Attachment #372785 - Attachment is obsolete: true
Attachment #372785 - Flags: review?
same code.  minefield doesn't love me.
That's ... strange? Gavin, any idea why the string gets placed differently in Minefield?
Comment on attachment 372787 [details]
screenshot (with patch v.3) in Shiretoko

oh, sorry, uir+ with the nit that the "Learn more..." should be underlined (but not the parenthesis - I know - I'm a bitch)
If we are going for one line, then this should be the same height as other infobars on OS X.  Also, can you duplicate the icon you are display and rename it so we can do a drop in of a more relevant icon? (it looks like I'm even more of a bitch)
@bitches.  :-) I agree with all of your comments.  Just wondering are these things b4 blockers or things we need to clean up after the code freeze for b4. I am sort of bullish on pushing something a bit unpolished but has the required elements -- with a promise that I will fix up the mess.

mike, localization of "(text)" becomes 3 strings doesn't it?  does it matter?

alex, np on the icon.  I am not doing anything to change the size of the infobar.  Maybe it is because of the checkbox? Gavin, got ideas?
this has an underline under the learn more link, but it doesn't have the parenthesis.  Just wondering if this would work, or does having the parens help?
Attached patch patch v.4 (obsolete) — Splinter Review
integrated mfinkle's irc comments

1) now works on both Minefield and 1.9.1  (According to mfinkle, trying to add a child into the anonymous elements may be undefined).  Now we just add our label to the description element.  A side benefit is that we don't have to mess around with the spacer element.

2) i copied the Info.png to Geo.png and update the url we use.

3) underlining works.
Attachment #372809 - Flags: review?
Attachment #372809 - Flags: review? → review?(gavin.sharp)
Whiteboard: [icon-3.1] [icon-needed]
Windows icons are above, icons for OS X and Linux will likely only be ready after Beta 4
This patch is fine from a UI standpoint for now; we can do fixups after the b4 cutoff.
Comment on attachment 372809 [details] [diff] [review]
patch v.4

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>+        link.style.color = "white";

Ugh. Could you use a system color there (preferably the same as we're using for the normal text) or make this a class and style it via CSS to be white on Mac only and standard text-link elsewhere? I don't think plain "white" will look good on all OS/theme combinations we support.
Comment on attachment 372809 [details] [diff] [review]
patch v.4

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>   prompt: function(request) {
> 
>+    var prefService = Components.classes["@mozilla.org/content-pref/service;1"]
>+                                .getService(Components.interfaces.nsIContentPrefService);

Could use Cc/Ci throughout this function.

>+              callback: function(bar) {
>+                if (bar.lastChild.checked)
>+                  setPagePermission(request.requestingURI, true);
>+                request.allow(); 

Use "notification" rather than "bar", and add a comment explaining that the lastChild is the checkbox you added (applies to both functions)? Actually, it would be better to use bar.ownerDocument.getElementById("rememberChoice"), I think.

>-                                         "chrome://browser/skin/Info.png",
>+                                                      "chrome://browser/skin/Geo.png",

Missing a jar.mn addition for Geo.png?

>+      function geolocation_hacks_to_notification () {

>+        var link = newBar.ownerDocument.createElementNS(XULNS, "label");
>+        link.setAttribute("value", "Learn More..."); /* xxx hardcoded english us */
>+        link.addEventListener("click", function() requestingWindow.open("http://www.mozilla.com/geolocation") , false); /* xxx hardcoded english us */

>+        // This works, but the blue is hard to read over the grey background of the notification
>+        // link.className = "text-link";
>+        link.style.textDecoration = "underline";
>+        link.style.color = "white";

I'd prefer to use the text-link binding and override the color styling... though need to make sure it works on all platforms since the notification bar backgrounds vary as Kairo mentions. It would also allow you to just set href="http://www.mozilla.com/geolocation" rather than adding an onclick handler (and pick up keyboard accessibility at the same time).

>+      requestingWindow.setTimeout(geolocation_hacks_to_notification, 0);

I think you want getChromeWindow(requestingWindow).wrappedJSObject (i.e. add a chromeWindow temporary above where "tabbrowser" is defined) rather than "requestingWindow", which as far as I can tell is a content window and can go away (despite that being unlikely).
Attached patch patch v.5 (obsolete) — Splinter Review
incorporates gavin's comments
incorporates alex's icons
Attachment #372809 - Attachment is obsolete: true
Attachment #372892 - Flags: review?
Attachment #372809 - Flags: review?(gavin.sharp)
The setTimeout issue doesn't seem to have been addressed, and it looks like you unintentionally changed the icon reference back to Info.png?
Attached patch patch v.6 (obsolete) — Splinter Review
Attachment #372892 - Attachment is obsolete: true
Attachment #372892 - Flags: review?
Attachment #372912 - Flags: review?(gavin.sharp)
Comment on attachment 372912 [details] [diff] [review]
patch v.6

>+        link.href="http://www.mozilla.com/geolocation";

This should be /firefox/geolocation, and follow the same pattern as the phishing protection stuff, like in http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#681
Attached patch patch v.7 (obsolete) — Splinter Review
incorporates beltzner's feedback.
Attachment #372912 - Attachment is obsolete: true
Attachment #372962 - Flags: review?
Attachment #372912 - Flags: review?(gavin.sharp)
Attachment #372962 - Flags: review? → review+
Comment on attachment 372962 [details] [diff] [review]
patch v.7

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>       var buttons = [{

>+            callback: function(notification) {
>+              if (notification.ownerDocument.getElementById("rememberChoice").checked)
>+                setPagePermission(request.requestingURI, false);
>+              request.cancel()
>+              },

Indentation isn't quite right here.

>+      getChromeWindow(requestingWindow).wrappedJSObject.setTimeout(geolocation_hacks_to_notification, 0);

Should avoid calling getChromeWindow twice by holding it in a local variable earlier in this function (where "tabbrowser" is declared).
Attached patch patch v.8Splinter Review
includes last comments.
Attachment #372962 - Attachment is obsolete: true
Comment on attachment 372970 [details] [diff] [review]
patch v.8

a191=beltzner
Attachment #372970 - Flags: approval1.9.1+
see 488218 for fixing the l10n issues.
(In reply to comment #43)
> see 488218 for fixing the l10n issues.

What did you really mean?
Duplicate of this bug: 463235
marking fixed.  please open new bugs for new issues.

alex, do you need me to file new bugs for the icon's on mac and linux?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 372970 [details] [diff] [review]
patch v.8

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
...
>-// FAQ URL
>+// FAQ URLs
> pref("browser.safebrowsing.warning.infoURL", "http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/phishing-protection/");
>+pref("browser.geolocation.warning.infoURL", "http://%LOCALE%.www.mozilla.com/%LOCALE%/firefox/geolocation/");

This is 404 now. (I tried http://en-US.www.mozilla.com/en-US/firefox/geolocation/). Is there separate bug ID about this issue?
@bart bug 488497
>alex, do you need me to file new bugs for the icon's on mac and linux?

I've filed bug 489252 and bug 489253
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.