Closed
Bug 488218
Opened 15 years ago
Closed 15 years ago
geolocation ui changes
Categories
(Firefox :: General, defect)
Firefox
General
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
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Comment 2•15 years ago
|
||
Assignee | ||
Comment 3•15 years ago
|
||
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.
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 6•15 years ago
|
||
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.
Comment 7•15 years ago
|
||
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!
Assignee | ||
Comment 8•15 years ago
|
||
gavin, can you give me a hand here? I had a bit of xulpain trying to get two lines in the notification hbox.
Comment 9•15 years ago
|
||
>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.
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #372517 -
Attachment is obsolete: true
Attachment #372518 -
Attachment is obsolete: true
Attachment #372771 -
Flags: review?
Assignee | ||
Comment 11•15 years ago
|
||
Assignee | ||
Comment 12•15 years ago
|
||
actually, the text in the notification bar should just read "Learn More.".
Comment 13•15 years ago
|
||
learn more shouldn't be yellow, and should be underlined to indicate it behaves like a hyperlink.
Comment 14•15 years ago
|
||
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.
Assignee | ||
Comment 15•15 years ago
|
||
new patch+screenshot coming up.
Assignee | ||
Comment 16•15 years ago
|
||
Attachment #372771 -
Attachment is obsolete: true
Attachment #372772 -
Attachment is obsolete: true
Attachment #372771 -
Flags: review?
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #372785 -
Flags: review?
Assignee | ||
Comment 18•15 years ago
|
||
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 19•15 years ago
|
||
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
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #372784 -
Attachment is obsolete: true
Attachment #372785 -
Attachment is obsolete: true
Attachment #372785 -
Flags: review?
Assignee | ||
Comment 21•15 years ago
|
||
Assignee | ||
Comment 22•15 years ago
|
||
same code. minefield doesn't love me.
Comment 23•15 years ago
|
||
That's ... strange? Gavin, any idea why the string gets placed differently in Minefield?
Updated•15 years ago
|
Attachment #372787 -
Flags: ui-review+
Comment 24•15 years ago
|
||
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)
Comment 25•15 years ago
|
||
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)
Assignee | ||
Comment 26•15 years ago
|
||
@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?
Assignee | ||
Comment 27•15 years ago
|
||
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?
Assignee | ||
Comment 28•15 years ago
|
||
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?
Assignee | ||
Updated•15 years ago
|
Attachment #372809 -
Flags: review? → review?(gavin.sharp)
Updated•15 years ago
|
Whiteboard: [icon-3.1] [icon-needed]
Comment 29•15 years ago
|
||
Comment 30•15 years ago
|
||
Comment 31•15 years ago
|
||
Windows icons are above, icons for OS X and Linux will likely only be ready after Beta 4
Comment 32•15 years ago
|
||
This patch is fine from a UI standpoint for now; we can do fixups after the b4 cutoff.
Comment 33•15 years ago
|
||
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 34•15 years ago
|
||
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).
Assignee | ||
Comment 35•15 years ago
|
||
incorporates gavin's comments incorporates alex's icons
Attachment #372809 -
Attachment is obsolete: true
Attachment #372892 -
Flags: review?
Attachment #372809 -
Flags: review?(gavin.sharp)
Comment 36•15 years ago
|
||
The setTimeout issue doesn't seem to have been addressed, and it looks like you unintentionally changed the icon reference back to Info.png?
Assignee | ||
Comment 37•15 years ago
|
||
Attachment #372892 -
Attachment is obsolete: true
Attachment #372892 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #372912 -
Flags: review?(gavin.sharp)
Comment 38•15 years ago
|
||
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
Assignee | ||
Comment 39•15 years ago
|
||
incorporates beltzner's feedback.
Attachment #372912 -
Attachment is obsolete: true
Attachment #372962 -
Flags: review?
Attachment #372912 -
Flags: review?(gavin.sharp)
Updated•15 years ago
|
Attachment #372962 -
Flags: review? → review+
Comment 40•15 years ago
|
||
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).
Assignee | ||
Comment 41•15 years ago
|
||
includes last comments.
Attachment #372962 -
Attachment is obsolete: true
Comment 42•15 years ago
|
||
Comment on attachment 372970 [details] [diff] [review] patch v.8 a191=beltzner
Attachment #372970 -
Flags: approval1.9.1+
Assignee | ||
Comment 43•15 years ago
|
||
see 488218 for fixing the l10n issues.
Comment 44•15 years ago
|
||
(In reply to comment #43) > see 488218 for fixing the l10n issues. What did you really mean?
Comment 45•15 years ago
|
||
bug 488574
Assignee | ||
Comment 47•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Comment 48•15 years ago
|
||
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?
Assignee | ||
Comment 49•15 years ago
|
||
@bart bug 488497
Comment 50•15 years ago
|
||
>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
Assignee | ||
Updated•15 years ago
|
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•