847626 - Work - Add "learn more" link to Geolocation info app bar notification

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

The geolocation permission prompt should include a "learn more" link like it does in desktop Firefox.  See design spec in bug 838211.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 2

[Allison Naaktgeboren :ally]

Comment on attachment 723648 [details] [diff] [review]
patch

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

r+, a couple nits (below)

Should we file a followup bug to get a test? I'm starting to wonder if we should start a testing bug, and append a comment every time we add something without a test. Might save on bugzilla load.

::: browser/metro/components/ContentPermissionPrompt.js
@@ +42,5 @@
> +  getChromeWindowForRequest: function getChromeWindowForRequest(aRequest) {
> +    return aRequest.window ?
> +      this.getChromeWindow(aRequest.window.top).wrappedJSObject :
> +      aRequest.element.ownerDocument.defaultView;
> +  },

style nit: while this is legal, please make this an if/else block. Save the ? operator for stuff that fits on one line

::: browser/metro/theme/platform.css
@@ +438,5 @@
>  }
>  
> +.notification-link {
> +  /* Make the link take up all the space before the buttons. */
> +  -moz-box-flex: 9999;

this seems like a hack, or is there really no better way? :(

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #2)
> Should we file a followup bug to get a test? I'm starting to wonder if we
> should start a testing bug, and append a comment every time we add something
> without a test. Might save on bugzilla load.

I'll set the in-testsuite? flag to put this on my dashboard.  (See also http://bugzil.la/ALL+:Metro+testsuite%3F )

> style nit: while this is legal, please make this an if/else block. Save the
> ? operator for stuff that fits on one line

Fixed.

> > +  /* Make the link take up all the space before the buttons. */
> > +  -moz-box-flex: 9999;
> 
> this seems like a hack, or is there really no better way? :(

An alternative would be to set "-moz-box-flex: 0;" on all the other other children of the <notification> element -- but I don't have a clean way to select all those children since the <spacer> has no class or ID.  :/

It didn't seem worth submitting a toolkit bug to modify the binding just so I could write this ruleset differently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/98280804365a

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98280804365a
https://hg.mozilla.org/mozilla-central/rev/10b98a87ae87