Closed Bug 573536 Opened 11 years ago Closed 11 years ago

new geolocation popup: "Learn More" link is missing

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Gavin, Assigned: Margaret)

References

Details

(Whiteboard: popupnotification)

Attachments

(2 files, 5 obsolete files)

This is a regression from bug 398776. Need to re-add the link to the notification somehow.
Whiteboard: popupnotification
blocking2.0: ? → betaN+
Is there an example of what this should look like?
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.
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
With theme changes from bug 577931 applied.
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?
Attachment #469506 - Flags: feedback+
Attached patch patch-v2 (obsolete) — Splinter Review
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.
Attachment #469506 - Attachment is obsolete: true
Attachment #469533 - Flags: review?(gavin.sharp)
Attachment #469533 - Attachment is patch: true
Attachment #469533 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch-v3 (obsolete) — Splinter Review
Fixed winstripe css. Tested on windows 7.
Attachment #469533 - Attachment is obsolete: true
Attachment #469550 - Flags: review?(gavin.sharp)
Attachment #469533 - Flags: review?(gavin.sharp)
Fixed geolocation notification id to work after the patch for bug 591026 lands.
Attachment #469550 - Attachment is obsolete: true
Attachment #469592 - Flags: review?(gavin.sharp)
Attachment #469550 - Flags: review?(gavin.sharp)
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).
Attachment #469592 - Flags: review?(gavin.sharp) → review+
Comment on attachment 469550 [details] [diff] [review]
patch-v3

(I guess we actually want this one given bug 591026)
Attachment #469550 - Flags: review+
Attachment #469592 - Attachment is obsolete: true
Attachment #469550 - Attachment is obsolete: false
Comment on attachment 469592 [details] [diff] [review]
patch-v4 (new geolocation-notification id)

nope, this one - bug 591026 landed after all :)
Attachment #469592 - Attachment is obsolete: false
Attachment #469550 - Attachment is obsolete: true
Attached patch updated patch (obsolete) — Splinter Review
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).
Attachment #469592 - Attachment is obsolete: true
Not sure what happened, but the last patch completely failed to apply on trunk. This one works.
Attachment #470152 - Attachment is obsolete: true
Attachment #470475 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/ba1849f86ade
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Blocks: 593512
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 :-)
Blocks: 646858
Verified on:
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110405 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Blocks: 595810
No longer blocks: 593512
You need to log in before you can comment on or make changes to this bug.