573536 - new geolocation popup: "Learn More" link is missing

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

This is a regression from bug 398776. Need to re-add the link to the notification somehow.

Comment 1

[:Margaret Leibovic]

Is there an example of what this should look like?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I think we can just add it to the end of the current message for now. I guess if the button was aligned to the bottom right, putting the link on the bottom left could work too. I think we want to do this by overriding the base popupnotification binding, which should give us enough flexibility to do either.

Comment 3

[:Margaret Leibovic]

Attachment: patch

Comment 4

[:Margaret Leibovic]

Attachment: screenshot (link on bottom left)

With theme changes from bug 577931 applied.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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

This looks great!

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+  <binding id="geolocation-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">

>+        let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");

You can just use gNavigatorBundle.

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

>+#geolocation .text-link[anonid="learnmore"] {

Probably better to just give this a unique class (geolocation-text-link?)

This needs styling on linux and windows too, presumably?

Comment 6

[:Margaret Leibovic]

Attachment: patch-v2

Updated patch with feedback from comment 5.

I added some positioning css to the gnomestripe and winstripe themes, but I don't have environments to test them in. Additional polish may be necessary, such as changing the color of the link, but that depends on how the styling turns out after bug 577927 is resolved.

Comment 7

[:Margaret Leibovic]

Attachment: patch-v3

Fixed winstripe css. Tested on windows 7.

Comment 8

[:Margaret Leibovic]

Attachment: patch-v4 (new geolocation-notification id)

Fixed geolocation notification id to work after the patch for bug 591026 lands.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 469592 [details] [diff] [review]
patch-v4 (new geolocation-notification id)

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+  <binding id="geolocation-notification" extends="chrome://global/content/bindings/notification.xml#popup-notification">
>+    <content>

Hmm, do you not need the align="start"? I forget why I had to add that to the base binding... It looks like this fixes the button alignment though, so maybe we should get rid of it on the base binding too?

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

>+.geolocation-text-link {
>+  color: #ffffff;
>+  -moz-padding-start: 13px;
>+  padding-top: 10px;
>+}

I think we should probably move the color: rule to the #notification-popup block, and get rid of the .popup-notification-description rule, but we can do that in a followup if you want (has the potential to affect other stuff).

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 469550 [details] [diff] [review]
patch-v3

(I guess we actually want this one given bug 591026)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 469592 [details] [diff] [review]
patch-v4 (new geolocation-notification id)

nope, this one - bug 591026 landed after all :)

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

I took the liberty of addressing my comments and updating to deal with bug 591026's new patch, since I had this applied. I had to adjust the Mac styling to avoid the padding from making the button too tall (which breaks native styling).

Removing the specific .geolocation-text-link styling on Mac didn't work, since the geolocation blue link styling is more specific.

I haven't tested this on Windows or Linux - I wonder whether the padding needs adjustment there too (not sure what platforms you tested).

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch that applies

Not sure what happened, but the last patch completely failed to apply on trunk. This one works.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/ba1849f86ade

Comment 15

[[e-mail]]

You didn't actually remove the XXX comment from nsBrowserGlue.js, which is fortunate as it made it easier for me to find this bug :-)

Comment 16

[Simona Badau, Desktop QA]

Verified on:
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110405 Firefox/4.2a1pre