new geolocation popup: "Learn More" link is missing

VERIFIED FIXED in Firefox 4.0b7

Status

()

VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Gavin, Assigned: Margaret)

Tracking

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: popupnotification)

Attachments

(2 attachments, 5 obsolete attachments)

This is a regression from bug 398776. Need to re-add the link to the notification somehow.
Whiteboard: popupnotification
blocking2.0: ? → betaN+
(Assignee)

Comment 1

8 years ago
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)

Updated

8 years ago
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 years ago
Created attachment 469506 [details] [diff] [review]
patch
(Assignee)

Comment 4

8 years ago
Created attachment 469507 [details]
screenshot (link on bottom left)

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+
(Assignee)

Comment 6

8 years ago
Created attachment 469533 [details] [diff] [review]
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.
Attachment #469506 - Attachment is obsolete: true
Attachment #469533 - Flags: review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Attachment #469533 - Attachment is patch: true
Attachment #469533 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 7

8 years ago
Created attachment 469550 [details] [diff] [review]
patch-v3

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)
(Assignee)

Comment 8

8 years ago
Created attachment 469592 [details] [diff] [review]
patch-v4 (new geolocation-notification id)

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
Created attachment 470152 [details] [diff] [review]
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).
Attachment #469592 - Attachment is obsolete: true
Created attachment 470475 [details] [diff] [review]
patch that applies

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+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ba1849f86ade
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6

Updated

8 years ago
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 :-)
(Assignee)

Updated

8 years ago
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.