New About window for Firefox 4

VERIFIED FIXED in Firefox 4.0b7

Status

()

Firefox
General
VERIFIED FIXED
8 years ago
5 years ago

People

(Reporter: faaborg, Assigned: Margaret)

Tracking

(Depends on: 3 bugs, {meta, ux-tone})

unspecified
Firefox 4.0b7
x86
Windows 7
meta, ux-tone
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings])

Attachments

(9 attachments, 18 obsolete attachments)

3.42 KB, application/octet-stream
Details
458.02 KB, image/png
Details
389.61 KB, image/png
Details
896.99 KB, image/jpeg
Details
112.33 KB, image/png
Details
76.20 KB, image/png
beltzner
: ui-review+
Details
78.97 KB, image/png
Details
206.82 KB, patch
Pike
: feedback+
Details | Diff | Splinter Review
133.30 KB, image/png
Details
This is a meta bug to track design and discussion of creating a new About window for Firefox 4.  Dependent bugs will include things like:

-a button for check for updates
-modifying the text to describe our mission and values (mentioning that this is open source software)
-including a link to our privacy policy
-general visual design, making sure the window has the Stephen Horlander level of quality :)
(Reporter)

Updated

8 years ago
Depends on: 520024
(Reporter)

Updated

8 years ago
Depends on: 571424
(Reporter)

Updated

8 years ago
Keywords: ux-tone
I'll take the design work on this, standing on the shoulders of giants (in this case, faaborg)
Assignee: nobody → beltzner
(Reporter)

Comment 2

8 years ago
I would be interested to see if shorlander has any ideas as well.  The common approach of just using the app icon on white (a little too large for the window) works reasonably well, but we could probably create some better graphics (perhaps reference our in-content UI textures?)
Created attachment 469774 [details]
mockup v1

First pass at the revised design, taking all dependencies into account.

Major changes: 
 - no UA string anymore (can still be obtained from about:)
 - copyright notice removed
 - credits button/function removed [1]
 - language made more open sourcey
 - took a shot at a tagline
 - added help button

[1] we haven't been keeping it up to date, and about:contributors is a better way of doing this, I think, in the long run

Not final, need to tweak it a bunch I think.

@shorlander: given this set of content, can you perhaps propose a layout? I went with a slightly wider box, and left room for a graphical element, but would love your take on it

@faaborg: the tagline/language is close, but not quite right. Give me a tweak?

@margaret: if you want to start implementing, please note that the word "contributors" should link to about:contributors, "rights" to about:rights and "privacy" to the Firefox Privacy policy. To get the "Last Updated" date, use the buildID.
Attachment #469774 - Flags: ui-review?(faaborg)
(In reply to comment #3)
> @shorlander: given this set of content, can you perhaps propose a layout? I
> went with a slightly wider box, and left room for a graphical element, but
> would love your take on it

I will take a look at this over the weekend.

Updated

8 years ago
Blocks: 591776
(Reporter)

Comment 6

8 years ago
>@faaborg: the tagline/language is close, but not quite right. Give me a tweak?

I gave this (and two other areas of text) a lot of thought over the weekend.  I want to run it by the ux team and some people from engagement, but in general I think we might want to dial up the ideological rhetoric a bit, and say a few things that wouldn't sound right coming from our competitors.  For instance start with the line:

"Firefox exists to protect a free and open Web."
No longer blocks: 591776
Depends on: 591776
(Assignee)

Comment 7

8 years ago
We also need to incorporate the "licensing information" link somehow. Thoughts on where to put that?
(Assignee)

Comment 8

8 years ago
Created attachment 470642 [details]
basic about dialog (osx w/ firefox branding)

I have the basic functionality here. Now we just need to make it pretty!
(Assignee)

Comment 9

8 years ago
Created attachment 470644 [details]
basic about dialog (windows w/ minefield branding)
(Assignee)

Comment 10

8 years ago
Created attachment 470684 [details] [diff] [review]
wip
(In reply to comment #7)
> We also need to incorporate the "licensing information" link somehow. Thoughts
> on where to put that?

Perhaps if the wee text were to read:

// Branded

Firefox and the Firefox logos are trademarks of the Mozilla Foundation. Some of the trademarks are used under license from The Charlton Company. The source code for Firefox is available under _several licenses_.

// Unbranded

The source code for Minefield is available under _several licenses_.

(In reply to comment #8)
> I have the basic functionality here. Now we just need to make it pretty!

Great work! Two comments: 
 - need to pull the version string / buildID apart and make it human readable
 - maybe we should use a link instead of a button for check for updates? the button is much more ... visible ... though.
(Assignee)

Comment 12

8 years ago
>  - need to pull the version string / buildID apart and make it human readable

Are you talking about the "version 4.0b5pre" string? I'm not sure how I would make that more human readable. Also, won't this naturally be more readable for official releases, since it will just have numbers?
(Assignee)

Comment 13

8 years ago
Created attachment 470852 [details] [diff] [review]
wip-v2

Improved update functionality from last patch.
Attachment #470684 - Attachment is obsolete: true
(In reply to comment #12)
> Are you talking about the "version 4.0b5pre" string? I'm not sure how I would
> make that more human readable. Also, won't this naturally be more readable for
> official releases, since it will just have numbers?

It will; I meant that I wanted to put emphasis on the Last Updated date, as that's more meaningful to people. I think you've got that mostly sorted in your soon-to-come update, though!
(Assignee)

Comment 15

8 years ago
Right now there's code that includes the distribution and distribution id if a
pref is set (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.js#45).

Kev, do you know if we need to keep this?
(Assignee)

Comment 16

8 years ago
Oops, forgot the cc. Kev, can you take a look at comment 15?
(Assignee)

Updated

8 years ago
Attachment #470852 - Flags: review?(dolske)
This isn't really a [meta] anymore, it's actually tracking the implementation which will take care of all of these dependencies. Also removing bug 591776 from the dependency list - it's related to, but not dependent on this fix, I think.
No longer depends on: 591776
Summary: [meta] New About window for Firefox 4 → New About window for Firefox 4
blocking2.0: --- → beta6+
Whiteboard: [strings]

Comment 18

8 years ago
(In reply to comment #17)
> Also removing bug 591776 from the dependency list - it's related to,
> but not dependent on this fix, I think.

It's a soft one-way dependency: if the UA leaves the about dialog it should be put in a better user accessible place, but if it doesn't then it should still probably go to about:support too, but doesn't have to as much. These are both blocking beta6+ now so removal from the list is fine by me.

Comment 19

8 years ago
(In reply to comment #8)
> Now we just need to make it pretty!

Couple suggestions:
1) Show a little more tail. There's more room to show more of the big logo.
2) Maybe use some of the bluey background stuffs that's been showing up on various Mozilla.com sites, i.e. some variation of:
http://www.mozilla.com/img/tignish/template/background-feature.jpg

Comment 20

8 years ago
I would like to keep the content from comment #15 if we can, or find a simple way to get the "about" information from distribution.ini elsewhere to make it easy for an individual to see they're using a customized distribution without looking at http logs or the file directory.

Comment 21

8 years ago
Comment on attachment 470852 [details] [diff] [review]
wip-v2

+  if (um.updateCount) {
+    var update = um.getUpdateAt(0);
+    var lastUpdated = formatBuildDate(update.buildID);
+    var lastUpdatedLabel = document.documentElement.getAttribute("lastupdatedlabel");
+    document.getElementById("lastUpdated").setAttribute("value",
+                            "(" + lastUpdatedLabel + lastUpdated + ")");
+  }

This won't work for l10n, you'll need to probably make the formatting go through a .properties entry. 

Just me, if I read the code correct, "Last updated" refers to the build ID date? Confusing to me. I assume that the real build id is still accessible through about:support?
Attachment #470852 - Flags: feedback-
(Assignee)

Comment 22

8 years ago
(In reply to comment #21)
> Comment on attachment 470852 [details] [diff] [review]
> wip-v2
> 
> +  if (um.updateCount) {
> +    var update = um.getUpdateAt(0);
> +    var lastUpdated = formatBuildDate(update.buildID);
> +    var lastUpdatedLabel =
> document.documentElement.getAttribute("lastupdatedlabel");
> +    document.getElementById("lastUpdated").setAttribute("value",
> +                            "(" + lastUpdatedLabel + lastUpdated + ")");
> +  }
> 
> This won't work for l10n, you'll need to probably make the formatting go
> through a .properties entry. 

Okay, I'll fix that. It does look pretty hacky :)

> Just me, if I read the code correct, "Last updated" refers to the build ID
> date? Confusing to me. I assume that the real build id is still accessible
> through about:support?

Initially I implemented it as the the date when the last update was applied, but beltzner decided it would be more useful to have the build date for the last update, since that's more sensible for troubleshooting.
(Assignee)

Comment 23

8 years ago
Created attachment 471308 [details] [diff] [review]
wip-v3 (localization fixes)
Attachment #470852 - Attachment is obsolete: true
Attachment #471308 - Flags: feedback?(l10n)
Attachment #470852 - Flags: review?(dolske)
(Assignee)

Updated

8 years ago
Attachment #471308 - Flags: review?(dolske)

Comment 24

8 years ago
Comment on attachment 471308 [details] [diff] [review]
wip-v3 (localization fixes)

Looks good, just two comments for the sake of completeness.

I guess the slightly different use of trademarkInfo shouldn't kill us here, though you should probably update the localization note in http://mxr.mozilla.org/mozilla-central/source/other-licenses/branding/firefox/locales/en-US/brand.dtd#5.

I'm only a little concerned that the order of the link to about:rights and about:privacy is fixed, that could only cause an issue if there's a language in which beginning and end of a sentence have drastically different priorities than the little they have in English. Which wouldn't be all that bad, IMHO, as it's really not that much of a prioritization in English either way.
Attachment #471308 - Flags: feedback?(l10n) → feedback+
(Assignee)

Comment 25

8 years ago
Created attachment 471326 [details]
updated screenshot (osx w/ firefox branding)
Attachment #470642 - Attachment is obsolete: true
Created attachment 471331 [details]
Potential about window text

Here's some different text, with the same layout as beltzner's first mockup and margaret's patch.

This text is more overtly ideological, attempting to position us relative to our competitors and accurately represent our unique perspective on critical issues such as end user rights, privacy, and net neutrality.

The alternate design is for if we absolutely must use the text "privacy policy" to link our privacy policy.

Comment 27

8 years ago
Comment on attachment 471331 [details]
Potential about window text

Both versions look beautiful! :D

I think the version with the 3 links on the bottom looks more clean as it has 2 fewer lines of text in the paragraph on the right.

Could the "Join Us" link be expanded to "Join the Mozilla team" or something, please? It looks out of place and sounds odd by itself.
Liz: regarding this attachment https://bugzilla.mozilla.org/attachment.cgi?id=471331 can you confirm if the hyperlink "ensure your rights and protect your privacy" is descriptive enough to comply with providing users access to our privacy policy?

I know Julie has some concerns, but I think this is very clearly aligned with our organization's overall values (even if in certain cases like feedback we do in fact allow users to provide information that they know will be made public).

Comment 29

8 years ago
I am not sure if I'm looking at the right version but I see this:

"Firefox is free and open source software provided by the non-profit Mozilla Foundation to make the Web a better place, and made possible by our global community of contributors.
As a user you have certain rights, and are entitled to your privacy."

A few things here:  Firefox is provided by the corporation, not the foundation.  Saying users have rights and are entitled to privacy can set the wrong expectations.  Some things we don't share, some things are public and users expect that.  

We could say:

"Firefox is free and open source software provided by Mozilla Corporation to make the Web a better place and made possible by our global community of contributors.
As a user your privacy matters.  We value your privacy too.  Our approach to your privacy is explained here. [link to PP]"
Julie, my question was about this version of the text: https://bugzilla.mozilla.org/attachment.cgi?id=471331

Comment 31

8 years ago
I like the version you just posted. Observations/suggestions:

-drop net neutrality language not b/c we don't support it, but b/c there's so much more complexity to it and it seems odd in this contex. Wouldn't this also be more appropriate for other content tools like first run or start page, website.  

-The messages regarding privacy and rights are somewhat obscure. If I were looking for licensing info or privacy policy, it doesn't immediately convey the message. thus, could you do something more simple like including the words/links at the bottom. 

[Licensing Information] [Privacy Policy] [About Rights]

-could you include something to the effect of: "The Mozilla Foundation is a non-profit, public interest organization committed to make the make the Web a better place." or some similar statement that got to the public interest nature of our project?
(In reply to comment #29)
> A few things here:  Firefox is provided by the corporation, not the foundation.
>  Saying users have rights and are entitled to privacy can set the wrong
> expectations.  Some things we don't share, some things are public and users
> expect that.  
That's not always true though, right?  I'm thinking of situations where a Linux distribution ships Firefox or someone produces a branded build (no modifications).

Comment 33

8 years ago
Is there really a need for the "..." after Check For Updates?

Comment 34

8 years ago
I don't have any brilliant ideas for the wording about our mission, but I agree with Harvey that the 3 links at the bottom are more helpful to users looking for those things.

What happened to the link to Help? I thought that was a good idea.

Comment 35

8 years ago
(In reply to comment #34)

It's a submenu item in the FF button.
https://bug583386.bugzilla.mozilla.org/attachment.cgi?id=465479
Taz: yes, there is a need, as it launches another dialog.

Liz: we say in the about:rights infobar that Firefox is offered by the Mozilla Foundation, which is the sole owner of the Corporation and the owner of the trademark.

Alex: I think this language is overly aggressive. We need a middle ground.

Margaret & reviewers: please focus on the code for now, we'll work the strings separately :)
(Assignee)

Comment 37

8 years ago
(In reply to comment #34)
> What happened to the link to Help? I thought that was a good idea.

Faaborg, what did happen to help in the new mock-ups? See bug 591013.
(Assignee)

Comment 38

8 years ago
Do we need to have an "OK" button for windows? If so, the current design would have to change.
(In reply to comment #22)
> Initially I implemented it as the the date when the last update was applied,
> but beltzner decided it would be more useful to have the build date for the
> last update, since that's more sensible for troubleshooting.

Would that change then necessitate a string change to avoid confusing a normal user? A user who knows they last update yesterday may be confused to see a potentially much earlier date. 

Perhaps "Firefox build date" instead?
(Assignee)

Comment 40

8 years ago
Created attachment 471606 [details] [diff] [review]
wip-v4
Attachment #471308 - Attachment is obsolete: true
Attachment #471606 - Flags: review?(dolske)
Attachment #471308 - Flags: review?(dolske)
(Assignee)

Comment 41

8 years ago
Created attachment 471609 [details]
wip-v4 screenshot (osx w/ firefox branding)

(Note: Don't pay too much attention to the exact language. It is still being worked out.) 

I decided not to put links in the text if they available at the bottom of the dialog because the redundancy seemed confusing.

Also, where should I get the large Firefox logo image? Should I be using an image for the "Firefox" in the upper left-hand corner, and if so, where should I get that?
Attachment #471326 - Attachment is obsolete: true
(Assignee)

Comment 42

8 years ago
Created attachment 471628 [details] [diff] [review]
patch

I did a lot of code clean-up here to get the patch into a more review-able state. Now all it should really need is some string/image/css love.
Attachment #471606 - Attachment is obsolete: true
Attachment #471628 - Flags: review?(dolske)
Attachment #471606 - Flags: review?(dolske)
Comment on attachment 471628 [details] [diff] [review]
patch

>diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul

...

> #ifdef XP_MACOSX
>         inwindowmenu="false"
>-        buttons="extra2"
>         align="end"
> #else
>         title="&aboutDialog.title;"
>-        buttons="accept,extra2"

I know it is like that already, but I am curious why there is no window title on Mac.
(In reply to comment #43)
> I know it is like that already, but I am curious why there is no window title
> on Mac.

From a quick scan of my OSX apps, that just seems to be de rigeur.
Assignee: beltzner → margaret.leibovic

Comment 45

8 years ago
Back to the help question - maybe a 4th button could be added at the bottom, either "Help" or "Technical Support."
Created attachment 472890 [details]
Revised text

revised text, first paragraph is about Firefox helping the world, second paragraph is about Firefox helping you personally:

--------------------
Firefox is created by Mozilla, a global community of contributors who are working together to build a better Internet.  Firefox exists to defend the open Web so that it remains a public resource for the free expression of ideas and unrestricted technological innovation.

As a non-profit organization we are leading a global movement to empower people everywhere with the tools necessary to take control of their online lives.  We believe in ensuring your rights and protecting your privacy.

Join Us
--------------------

Comment 47

8 years ago
My suggestion for a slightly modified version. I think it's more accurate to say Mozilla exists... rather than Firefox exists... I removed the last sentence. Since we're going to have the buttons at the bottom, I don't think it's needed and we had some concerns about the wording.

-------------------------------------------
Firefox is created by Mozilla, a global community of contributors who are
working together to build a better Internet.  

Mozilla exists to defend the open Web so that it remains a public resource for the free expression of ideas and unrestricted technological innovation. As a non-profit organization we are leading a global movement to empower people
everywhere with the tools necessary to take control of their online lives.

Join Us
----------------------------------
(Adding Chelsea; if we're starting to reference/link to mozilla.org here, we should get their lead messaging person in on the mix!)

Comments on the text, which I largely like in the form indicated in comment 47:

 - assuming "Mozilla" will link to www.mozilla.org
 - assuming "Join Us" will link to www.mozilla.org/contribute
 - doesn't mention that Firefox is free
 - doesn't mention that Firefox is open
 - doesn't match against mission perfectly

Addressing those comments, and drawing text from www.mozilla.org

-------------------------------------

Firefox is designed and built by _Mozilla_, a global community of contributors working together to make a the Internet better. Firefox is completely free and open. You can _use it_ in any way you want.

_Mozilla_ believes that the Internet should be open, public, and accessible by everyone without restriction. As a non-profit organization we define success as building a better communities and a better Internet, not benefiting our shareholders. 

Sound interesting? Why not _get involved_?

---------------------------------------

where,
  _Mozilla_ == www.mozilla.org
  _use it_ == about:rights
  _get involved_ == www.mozilla.org/contribute

I like the idea of having the links across the bottom as indicated in attachment 472890 [details] - let's definitely do that.

I also like the reduced visual weight (small text, lighter colour) for the trademark notification.

@Chelsea: you OK with this?

@Margaret: I think we can drop the requirement for a link to "Help" from this window (will update bug 591013) since this is already in the "Help" menu for most systems (OSX being the exception, what can you do) and it no longer really fits with the design.
(In reply to comment #48)
some edits, in the glaring light of the morning:

---------------------------

This software is completely free and open, you can _use it_ however you want.

Firefox is designed by _Mozilla_, a global community working together to make the Internet better. We believe that the Internet should be open, public, and accessible by everyone without restriction. 

Sound interesting? Why not _get involved_?

---------------------------

 - removed bit about shareholders, as it is misleading without the subsequent (in fact we don't have any shareholders!)
 - shortened, because brevity is the soul of wit, and German is the source of long strings
 - unburied the lede

As an added bonus, we can make the unbranded version the exact same as this one, and just drop the text about trademark attributions.
The links are pointing to the right place from the Foundation perspective (moz.org and /contribute) and the copy is fine as well (thank you for removing shareholders - there be dragons).
The first line here should probably match bug 571584, so adding that dependency.
Depends on: 571584

Comment 52

8 years ago
is there any reason why the About window needs to be a dialog box? Why couldn't it just open a mozilla.org webpage in a new tab, similar to the first run or start or Help page? 

That way there'd be much more space for content, it would fit with the rest of Firefox, and it would be part of the web.
Moving this from a dialog box to a tab is out of scope for this release.

Updated

8 years ago
Depends on: 589444
No longer depends on: 571584
No longer depends on: 589444
Based on today's meeting, text for the left hand side of the dialog:

---------------------------

Firefox is designed by _Mozilla_, a global community working together to make
the Internet better. We believe that the Internet should be open, public, and
accessible by everyone without any restrictions.

Sound interesting? _Get involved_!

---------------------------

Short and sweet.
(Assignee)

Comment 55

8 years ago
Created attachment 473292 [details] [diff] [review]
patch (with final strings)

Waiting on shorlander for design work, but this patch has the text from comment 54.
Attachment #471628 - Attachment is obsolete: true
Attachment #473292 - Flags: review?(dolske)
Attachment #471628 - Flags: review?(dolske)
> Major changes: 
>  - credits button/function removed [1]

> [1] we haven't been keeping it up to date, and about:contributors is a better
> way of doing this, I think, in the long run

fwiw, if you are referring to about:credits, it is far less updated than the credits in the about dialog, just look around for some name like dietrich, me, unfocused, shawn, and so on
I've filed Bug 594788 for the about:credits issue.
Created attachment 473598 [details]
About Layout and Visual Style Ideas

(In reply to comment #55)
> Waiting on shorlander for design work

Did a few layouts and tried to incorporate our proposed in-content style (http://blog.stephenhorlander.com/2010/06/01/in-content-ui-visual-unification/) as suggested by faaborg in comment 2. Although basic white works as well.

I think the current layout is good I would just suggest a few tweaks.

General:
* Firefox/Minefield icon should go on the left. Aside from being mostly standard having it on the wrong side of the text alignment looks odd.

* It would be nice to get the trademark text out of the way at the bottom. Not sure if there is some legal requirement about having it in close proximity to the logo?

Specific:
A) I think this works the best. Large icon on the left, main text on the right, links along the bottom and the trademark text out of the way at the very bottom. Slightly shorter window.

B) Same as A with the trademark text under the smaller logo.

C) Stacked text and links vertically. I like the extra links in a horizontal space at the bottom but if they are being localized is that enough space?

D) Smaller window with a less dominant icon.

E) Cropped icon and segmented elements (icon/wordmark/update) (about text) (links) (trademark stuff)

F) Mostly a throw away :) Centered layout.

Comment 59

8 years ago
The trademark statement doesn't need to be in close proximity to the logo, but it does need to be legible, so shouldn't be too faint to easily read.
Nice work, Stephen. I think A or C work best. So if your preference is for A, let's do that. As Liz said, no requirement for the trademark statement to be near the logo, and I think there's enough room for localization.

Margaret: the only differences between branded and unbranded builds should be:

branded = use Firefox logo, include trademark statement
unbranded = use Minefield logo, do not include trademark statement

(of course, %brandShortName should take care of the Firefox/Minefield thing)

Comment 61

8 years ago
A+ for A! :)

Typos: v.4.06bpre used instead of v.4.0b6pre, not that it matters in a mockup.

Question however, do we really need or want the leading "v."?
(In reply to comment #61)
> Question however, do we really need or want the leading "v."?
If we do, please make it localizable. "v.4.0b6" doesn't work for us, but "v4.06" does.

Comment 63

8 years ago
I personally think the leading "v." is unnecessary and could confuse users who already have trouble understanding version numbers. I suggest dropping it please.
Happy to drop the "v", sure.

Comment 65

8 years ago
An explicit usage of the word "version" would make sense. Would probably want to bump the "last updated" parenthetical down to a new line to fit it cleanly, however, just the plain version number is probably also fine.
(Assignee)

Comment 66

8 years ago
Created attachment 473674 [details] [diff] [review]
patch (with updated styling)
Attachment #473292 - Attachment is obsolete: true
Attachment #473674 - Flags: review?(dolske)
Attachment #473292 - Flags: review?(dolske)
(Assignee)

Comment 67

8 years ago
Created attachment 473676 [details]
screenshot (firefox, osx)
Attachment #470644 - Attachment is obsolete: true
Attachment #471609 - Attachment is obsolete: true

Comment 68

8 years ago
Can you make the trademark statement a shade darker?
(In reply to comment #65)
> An explicit usage of the word "version" would make sense. Would probably want
> to bump the "last updated" parenthetical down to a new line to fit it cleanly,
> however, just the plain version number is probably also fine.

No, thank you. We'll drop the "v" and leave "Last Updated" where it is.

(In reply to comment #68)
> Can you make the trademark statement a shade darker?

If we must. Is that aesthetic advice or legal advice? I'm pretty happy with it as it is. Noticable, but not at all emphasized.

Comment 70

8 years ago
(In reply to comment #69)
> No, thank you. We'll drop the "v" and leave "Last Updated" where it is.

Sounds good to me.

> (In reply to comment #68)
> > Can you make the trademark statement a shade darker?
> 
> If we must. Is that aesthetic advice or legal advice? I'm pretty happy with it
> as it is. Noticable, but not at all emphasized.

It's basically required clutter; keeping it light is better looking, in my opinion.
Margaret: sorry nobody answered it earlier, but the answer to your bug 585475 comment 12 question is pretty much "um, yeah, you're going to have to leave the menuitem around, in a Mac ifdef, and leave all the code around too."

The way the Widget: Cocoa menuitem moving works is that it finds the existing menuitem in the wrong menu, by id, hides it, remembers what the click action is, and then when the one it puts in the Firefox menu gets clicked, calls that action. So there has to be a menuitem with the id "checkForUpdates" or there will be no update menuitem in the Firefox menu, and there has to be a checkForUpdates() in scope for it to call.

Also, you're leaving some orphan CSS, in browser.css's #checkForUpdates[loading="true"].
We're being careful not to introduce a color profile right?  The screenshot has the firefox looking sunburned, but I'm assuming that's because the screenshot itself has the device profile of Margaret's laptop LCD.
The PNG in attachment 473676 [details] has a color profile. The supplied icon did not.
Depends on: 306018
What are you opinions on the fact that the Apple HIG explicitly recommends against text links in about windows?

"If you want to give the user a convenient way to visit your website or contact your company from your About window, be sure to create buttons that accomplish these tasks. Do not provide a clickable URL or email address because it is not necessarily clear that there is an action associated with it."

http://developer.apple.com/library/mac/#documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGWindows/XHIGWindows.html#//apple_ref/doc/uid/20000961-DontLinkElementID_1812

Comment 75

8 years ago
(In reply to comment #74)
Standard styled hyperlinks in a browser's dialog are quite clear. ;)
(In reply to comment #66)
> Created attachment 473674 [details] [diff] [review]
> patch (with updated styling)

I am of the opinion that the "Get Involved!" link should include the exclamation point.

I also think that "by" (after "accessible") should be changed to "to":
* open to everyone
* public to everyone
* accessible to everyone

And perhaps a comma after "everyone"?

(In reply to comment #69)
> (In reply to comment #65)
> > An explicit usage of the word "version" would make sense. Would probably want
> > to bump the "last updated" parenthetical down to a new line to fit it cleanly,
> > however, just the plain version number is probably also fine.
> 
> No, thank you. We'll drop the "v" and leave "Last Updated" where it is.

I second the proposal to move "Last Updated" down to its own line. I also think that it should lose the parentheses, gain a colon, and be more associated with the "Check for Updates..." button than the version number.

It might also be wise to include the build date/ID in there somewhere. (Unless I'm misinterpreting what the "Last Updated" date represents?)
(In reply to comment #76)
> I am of the opinion that the "Get Involved!" link should include the
> exclamation point.

I can't say that I hold a strong opinion either way; up to Margaret.

> I also think that "by" (after "accessible") should be changed to "to":
> * open to everyone
> * public to everyone
> * accessible to everyone

Good change; Margaret, please make it so.

> And perhaps a comma after "everyone"?

No, that would be a comma splice.

> I second the proposal to move "Last Updated" down to its own line. I also 

The answer was no before, and remains no.

However, on looking at it again, the "Last Updated" text isn't really needed. Margaret, sorry for the last minute change(s) but it should read:

Firefox
4.0.4 (released March 14, 2011)

> It might also be wise to include the build date/ID in there somewhere. (Unless
> I'm misinterpreting what the "Last Updated" date represents?)

The answer was no before, and remains no.
(In reply to comment #77)
> > And perhaps a comma after "everyone"?
> 
> No, that would be a comma splice.

No it wouldn't. Comma splices join two otherwise complete sentences (independent clauses) with a comma, creating a run-on.

The comma in question here would properly delineate a parenthetical clause. (And I'm changing my suggestion from a question to a request.)

> > I second the proposal to move "Last Updated" down to its own line. I also 
> 
> The answer was no before, and remains no.
> 
> However, on looking at it again, the "Last Updated" text isn't really needed.
> Margaret, sorry for the last minute change(s) but it should read:
> 
> Firefox
> 4.0.4 (released March 14, 2011)
> 
> > It might also be wise to include the build date/ID in there somewhere. (Unless
> > I'm misinterpreting what the "Last Updated" date represents?)
> 
> The answer was no before, and remains no.

I do much like better the use of "released", especially with the lowercase "r". But I wonder: Why are the other suggested changes so steadfastly "no"?
Are we going to stick with the string 'Last updated' even though it will display the build date, not the date the update was applied?
Luke, please read the previous bug comments (or at least the past two or three!) before commenting.
(Assignee)

Comment 81

8 years ago
Created attachment 474735 [details] [diff] [review]
patch v4

I addressed comment 71 by putting back the "Check for updates..." help menuitem with a Mac ifdef around it. I also put the utility functions back in utilityOverlay.js.

I also responded to some of the string feedback (I'll attach a screenshot).
Attachment #473674 - Attachment is obsolete: true
Attachment #474735 - Flags: review?(dolske)
Attachment #473674 - Flags: review?(dolske)
(Assignee)

Comment 82

8 years ago
Created attachment 474736 [details]
patch v4 screenshot
Comment on attachment 474736 [details]
patch v4 screenshot

This is fine from a UX perspective on strings and layout. No more changes needed or will be accepted.
Attachment #474736 - Flags: ui-review+
(Assignee)

Updated

8 years ago
Attachment #474735 - Flags: review?(dolske) → review?(gavin.sharp)
Comment on attachment 474735 [details] [diff] [review]
patch v4

General comments:
- Also need to remove (credits.xhtml)
  - talked to beltzner, probably need to figure out some way to expose about:credits (seems to have gotten lost along the way in string changes)
- about-logo.png and about-wordmark.png need to be added to "unofficial" branding (need non-bomb versions of the minefield images)
- tabbing through links causes them to jump around (:active or :focused styling messed up?)
- the addition of "aboutDialog" prefix to all IDs seems unnecessary (and redundant)

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js

> function init(aEvent)

>-  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>-                        .getService(Components.interfaces.nsIPrefBranch);
>-
>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].
>+              getService(Components.interfaces.nsIPrefBranch);

If you're going to change this, might as well change it to use Services.jsm.

> #ifdef XP_MACOSX
>   // it may not be sized at this point, and we need its width to calculate its position
>   window.sizeToContent();
>   window.moveTo((screen.availWidth / 2) - (window.outerWidth / 2), screen.availHeight / 5);
> #endif
>+
>+#ifdef MOZ_UPDATER
>+  initUpdates();
>+#endif

Seems like this should happen before the sizeToContent() call above.

>+function initUpdates()

Hmm, would be really nice to share this code with the code in buildHelpMenu that is was copied from... Can you refactor to make that possible?

>diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul

> <dialog xmlns:html="http://www.w3.org/1999/xhtml"

> #ifdef XP_MACOSX

>         align="end"

I think this is no longer needed.

>+        buttons=",">

Hmm, this is a bit strange. I wonder whether this should actually be a <dialog> at all... Does buttons="" work (I think it should).

>+        <description id="aboutDialog_version">
>+#expand <label class="version-label" value="__MOZ_APP_VERSION__"/> <label class="version-label" id="aboutDialog_released"/>

Not sure what the l10n implications are of doing hardcoding this order. I think you should have a single "version" label that is programmatically set, and modify the string to include the version:

aboutVersion=%1$S (released %2$S) (include an L10N note explaining the values of the variables)

You can use Services.appinfo.version (I confirmed that it's always the same as __MOZ_APP_VERSION__), and remove the MOZ_APP_VERSION define in /browser/base/Makefile.in.

>+        <label id="aboutDialog_distribution"/>
>+        <label id="aboutDialog_distributionId"/>

Have you tested what things look like with these set?

>+#ifdef MOZ_UPDATER
>+        <hbox>
>+          <button id="aboutDialog_checkForUpdates" oncommand="checkForUpdates();" align="start"/>

This only works on Mac, because only it includes browserMountPoints (which includes utilityOverlay.js). I guess you could unconditionally include utilityOverlay, but maybe it would be cleaner to move the update-related code into a JS module that both can use.

>+        <description class="text-blurb">
>+          &contribute.start;<label class="text-link" href="http://www.mozilla.org/contribute">&contribute.getInvolvedLink;</label>
>+        </description>

Probably would be a good idea to provide a contribute.end (even if it's empty in en-US), in case some locales require the ability to put non-linked text after the link.

>+        <label class="text-link bottom-link" href="http://www.mozilla.com/legal/privacy/">&bottomLinks.privacy;</label>    

nit: remove trailing whitespace here

>+  <stringbundleset id="aboutDialog_stringbundleset">
>+    <stringbundle id="aboutDialog_browserBundle" src="chrome://browser/locale/browser.properties"/>
>+  </stringbundleset>

Just use Services.strings.createBundle in the JS (less XBL overhead - might not be relevant if you refactor the update code).

>diff --git a/browser/base/content/baseMenuOverlay.xul b/browser/base/content/baseMenuOverlay.xul

>+#ifdef XP_MACOSX
>         <menuseparator id="updateSeparator"/>
> #ifdef MOZ_UPDATER
>         <menuitem id="checkForUpdates"
>                   label="&updateCmd.label;"
>                   class="menuitem-iconic"
>                   oncommand="checkForUpdates();"/>
> #endif
>+#endif
>         <menuseparator id="aboutSeparator"/>

Looks like this has an existing bug (will show two separators when MOZ_UPDATER isn't defined) - fix it while you're here by moving the MOZ_UPDATER ifdef up one line?
Attachment #474735 - Flags: review?(gavin.sharp) → review-

Comment 85

8 years ago
(In reply to comment #84)
...
> >+        <description id="aboutDialog_version">
> >+#expand <label class="version-label" value="__MOZ_APP_VERSION__"/> <label class="version-label" id="aboutDialog_released"/>
> 
> Not sure what the l10n implications are of doing hardcoding this order. I think
> you should have a single "version" label that is programmatically set, and
> modify the string to include the version:
> 
> aboutVersion=%1$S (released %2$S) (include an L10N note explaining the values
> of the variables)
> 
> You can use Services.appinfo.version (I confirmed that it's always the same as
> __MOZ_APP_VERSION__), and remove the MOZ_APP_VERSION define in
> /browser/base/Makefile.in.

I looked at this before, and my take away was that this is not a regression. Given that version number and release date are styled differently, putting them into one string with markup may be worse.
(Assignee)

Comment 86

8 years ago
Created attachment 474907 [details] [diff] [review]
patch v5
Attachment #474735 - Attachment is obsolete: true
Attachment #474907 - Flags: review?(gavin.sharp)
(Assignee)

Comment 87

8 years ago
(In reply to comment #84)

> - Also need to remove (credits.xhtml)
>   - talked to beltzner, probably need to figure out some way to expose
> about:credits (seems to have gotten lost along the way in string changes)

Should I file a new bug for this or try to get it in this patch?

> >diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul
> 
> > <dialog xmlns:html="http://www.w3.org/1999/xhtml"
> 
> > #ifdef XP_MACOSX
> 
> >         align="end"
> 
> I think this is no longer needed.
> 
> >+        buttons=",">
> 
> Hmm, this is a bit strange. I wonder whether this should actually be a <dialog>
> at all... Does buttons="" work (I think it should).

It's weird, but you do have to do buttons=",". I'm not sure why, but it doesn't work otherwise.

> >+        <description id="aboutDialog_version">
> >+#expand <label class="version-label" value="__MOZ_APP_VERSION__"/> <label class="version-label" id="aboutDialog_released"/>
> 
> Not sure what the l10n implications are of doing hardcoding this order. I think
> you should have a single "version" label that is programmatically set, and
> modify the string to include the version:
> 
> aboutVersion=%1$S (released %2$S) (include an L10N note explaining the values
> of the variables)
> 
> You can use Services.appinfo.version (I confirmed that it's always the same as
> __MOZ_APP_VERSION__), and remove the MOZ_APP_VERSION define in
> /browser/base/Makefile.in.

I didn't change this because of Pike's comment (comment 85).

> >+        <label id="aboutDialog_distribution"/>
> >+        <label id="aboutDialog_distributionId"/>
> 
> Have you tested what things look like with these set?

I have now! I'll attach a screenshot :)

> >+#ifdef MOZ_UPDATER
> >+        <hbox>
> >+          <button id="aboutDialog_checkForUpdates" oncommand="checkForUpdates();" align="start"/>
> 
> This only works on Mac, because only it includes browserMountPoints (which
> includes utilityOverlay.js). I guess you could unconditionally include
> utilityOverlay, but maybe it would be cleaner to move the update-related code
> into a JS module that both can use.

I just included utilityOverlay.js for now.
(Assignee)

Comment 88

8 years ago
Created attachment 474908 [details]
screenshot with distro text
(Assignee)

Comment 89

8 years ago
Created attachment 474912 [details] [diff] [review]
patch v6

Whoops, I forgot to add the new images to the jar file in browser/branding/unofficial/content.
Attachment #474907 - Attachment is obsolete: true
Attachment #474912 - Flags: review?(gavin.sharp)
Attachment #474907 - Flags: review?(gavin.sharp)
(In reply to comment #87)
> (In reply to comment #84)

> It's weird, but you do have to do buttons=",". I'm not sure why, but it doesn't
> work otherwise.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/dialog.xml#289

if aButtons is empty it does not parse it, it should probably check if aButtons === null || aButtons === undefined. you could probably pass a not existing string like buttons="none" and I guess that would work around the ugliness.
PRobably this should be a panel rather than a dialog though.
Comment on attachment 474912 [details] [diff] [review]
patch v6

General comments:
- about-wordmark should actually be "Mozilla Developer Preview" for "unofficial" branding (not a big deal though, can file followup)
- have you tested RTL? ForceRTL can help with that: https://addons.mozilla.org/en-US/firefox/addon/7438/
- has this had any Linux/Windows testing? I can help with that tomorrow.

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js

This doesn't explicitly import Services.jsm, but works because of utilityOverlay. I'd still kind of like to avoid the include of utilityOverlay by factoring out the update related code from it into a JS module (in a followup), so can you just do the explicit import() here to avoid needing to make that change later (and make it more obvious that it's happening)?

>+function initUpdates()

>+  var sdf =
>+      Components.classes["@mozilla.org/intl/scriptabledateformat;1"].
>+      getService(Components.interfaces.nsIScriptableDateFormat);

Hmm, I thought these were intended to be createInstance()d, but I suppose it doesn't matter since it doesn't seem to keep any persistent state (nsScriptableDateFormat.cpp).

>+    var update = um.getUpdateAt(0);
>+    var buildDate = sdf.FormatDateTime("", sdf.dateFormatLong,
>+                                       sdf.timeFormatNone,
>+                                       update.buildID.substring(0, 4),
>+                                       update.buildID.substring(4, 6),
>+                                       update.buildID.substring(6, 8),
>+                                       update.buildID.substring(8, 10),
>+                                       update.buildID.substring(10, 12),
>+                                       update.buildID.substring(12, 14));

nit: use let instead of var, and make the variable hold the buildID rather than the update object (updateBuildID)

>diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul

> <dialog xmlns:html="http://www.w3.org/1999/xhtml"

>+        buttons=",">

We should just make this a <window> and omit this attribute. To maintain the Esc-to-close behavior, you can add a:

<keyset>
  <key key="VK_ESCAPE" oncommand="window.close();"/>
</keyset>

>diff --git a/browser/base/content/credits.xhtml b/browser/base/content/credits.xhtml

>-    <!ENTITY % creditsDTD SYSTEM "chrome://browser/locale/credits.dtd">
>-      <img src="chrome://branding/content/aboutCredits.png" />
>-      <img src="chrome://branding/content/aboutFooter.png" />

These are all unused now.

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>+function setupCheckForUpdates(aElement)

Just keep the parameter name as "checkForUpdates" to reduce the diff churn.

>-#else
>-  // Needed by safebrowsing for inserting its menuitem so just hide it
>-  document.getElementById("updateSeparator").hidden = true;

I was wrong about this - safeBrowsing uses updateSeparator as an anchor for positioning its menu item, so we can't remove it for no-updater builds - need to undo that ifdef change I suggested.

>+    aElement.removeAttribute("loading");

I wonder whether we should have a throbber on the button to reflect the "loading" state... Have you tested actually going through an update cycle to make sure this UI handles it all correctly (i.e. looks OK while an update is downloading)? Rob Strong can probably help with that (he has update tests set up, IIRC), or we could just be sure to test it after it lands.

>diff --git a/browser/locales/en-US/chrome/browser/aboutDialog.dtd b/browser/locales/en-US/chrome/browser/aboutDialog.dtd

>+<!ENTITY bottomLinks.license        "Licensing Information">
>+<!ENTITY bottomLinks.rights         "End User Rights">
>+<!ENTITY bottomLinks.privacy        "Privacy Policy">

Might want an L10N note explaining that these are link titles, and mentioning their targets (about:license, about:rights, etc.).

Still a couple of things I want to check, so no r+ yet, but thought I'd post these right away so that you can respond to them.
(Assignee)

Comment 92

8 years ago
(In reply to comment #91)

> - have you tested RTL? ForceRTL can help with that:
> https://addons.mozilla.org/en-US/firefox/addon/7438/

It looks good, except for the fact that the wordmark is aligned to the left instead of the right. It's a background image, so I tried setting background-position: -moz-start, but that didn't do anything. Thoughts on how to fix this?

> - has this had any Linux/Windows testing? I can help with that tomorrow.

I did some Windows testing with an earlier patch, but I haven't tested the final patch on Linux or Windows.

> >-#else
> >-  // Needed by safebrowsing for inserting its menuitem so just hide it
> >-  document.getElementById("updateSeparator").hidden = true;
> 
> I was wrong about this - safeBrowsing uses updateSeparator as an anchor for
> positioning its menu item, so we can't remove it for no-updater builds - need
> to undo that ifdef change I suggested.

Should I also keep it outside of the Mac ifdef then? I can hide it in utilityOverlay.js, but it's kind of awkward because there are two ifdefs, and I would want to hide it in the else case for both of them.

> >+    aElement.removeAttribute("loading");
> 
> I wonder whether we should have a throbber on the button to reflect the
> "loading" state... Have you tested actually going through an update cycle to
> make sure this UI handles it all correctly (i.e. looks OK while an update is
> downloading)? Rob Strong can probably help with that (he has update tests set
> up, IIRC), or we could just be sure to test it after it lands.

I did test it with a test update from Rob, and it looks right. However, the test update downloads really quickly, so I didn't see the loading state. Clicking the button brings up the update dialog, though, and that lets the user know what's going on.
(Assignee)

Comment 93

8 years ago
Created attachment 475024 [details] [diff] [review]
patch v7
Attachment #474912 - Attachment is obsolete: true
Attachment #475024 - Flags: review?(gavin.sharp)
Attachment #474912 - Flags: review?(gavin.sharp)
(Assignee)

Comment 94

8 years ago
Created attachment 475027 [details] [diff] [review]
patch v8

Oops, wrong comment syntax in dtd file.
Attachment #475024 - Attachment is obsolete: true
Attachment #475027 - Flags: review?(gavin.sharp)
Attachment #475024 - Flags: review?(gavin.sharp)
Comment on attachment 475027 [details] [diff] [review]
patch v8

I've tested the latest patch on Windows 7, it works and looks correct.

Just a couple comments but it's personal taste mostly:
- I find the below grey part having a too subtle difference from the top white part. On my screen it's barely noticeable.
- I find a bit strange that each link opens a new separate window, I was expecting them to open tabs in my last window. But looks like we are not the only ones doing so.

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js

>+// Services = object with smart getters for common XPCOM services
>+Components.utils.import("resource://gre/modules/Services.jsm");

the comment does not look useful

> function init(aEvent)

>-      var distroVersion = prefs.getCharPref("distribution.version");
>-      var distroAbout = prefs.getComplexValue("distribution.about",
>+      var distroVersion = Services.prefs.getCharPref("distribution.version");
>+      var distroAbout = Services.prefs.getComplexValue("distribution.about",
>         Components.interfaces.nsISupportsString);
>   

there is some trailing space here, could be worth removing them while touching this code

>       var distroField = document.getElementById("distribution");
>       distroField.value = distroAbout;
>       distroField.style.display = "block";
>     

and here as well

>+function initUpdates()
>+  var um =
>+      Components.classes["@mozilla.org/updates/update-manager;1"].
>+      getService(Components.interfaces.nsIUpdateManager);
>+  var sdf =
>+      Components.classes["@mozilla.org/intl/scriptabledateformat;1"].
>+      getService(Components.interfaces.nsIScriptableDateFormat);

the limit is 80 chars still, and looks like these would not go over it, so why nl after the "="? it looks ugly
(ps: ah I see this code has been copied from utilityOverlay, if Gavin is not fine with fixing ugliness in both places,
I'd suggest to at least avoid importing it in new code)

>diff --git a/browser/locales/en-US/chrome/browser/aboutDialog.dtd b/browser/locales/en-US/chrome/browser/aboutDialog.dtd

>+<!ENTITY contribute.start           "Sound interesting? ">

stupid question. I'm not English so it's really a question, should not this be "Sounds interesting?"

Comment 96

8 years ago
Comment on attachment 475027 [details] [diff] [review]
patch v8

To copy the discussion from irc:


One thing to watch out for when testing on non-mac:

The long dateformat comment in http://mxr.mozilla.org/mozilla-central/source/intl/locale/idl/nsIScriptableDateFormat.idl#87 indicates that this may leave us with the weekday showing in the OS locale (not the fx locale) on at least some platforms. I wouldn't mind to just parse numbers out of the buildID and to use those directly.
(In reply to comment #96)
> the fx locale) on at least some platforms. I wouldn't mind to just parse
> numbers out of the buildID and to use those directly.

But I would. Do we know which locales don't have matching OS locales? Can we do something where "if we know it'll work, show month names, if not, show fugly numbers?"

Comment 98

8 years ago
This is about using a German build on an English OS, or vice versa.

This has nothing to do with matchOS or something.

Also, it's unclear what non-mac platforms actually return using the current code, as the comment describing the date format indicates something that the mac doesn't return (i.e., it claims to include the weekday). If that's just for datetime and not for just date, no idea. We'll have to test and see what we get.
Comment on attachment 475027 [details] [diff] [review]
patch v8

- it seems like link focus styling is broken on windows/linux
- RTL fix (based on feedback from ehsan): http://pastebin.mozilla.org/789057

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js

>+function initUpdates()

>+    let buildID = um.getUpdateAt(0).buildID;
>+    let buildDate = sdf.FormatDateTime("", sdf.dateFormatLong,
>+                                       sdf.timeFormatNone,
>+                                       buildID.substring(0, 4),
>+                                       buildID.substring(4, 6),
>+                                       buildID.substring(6, 8),
>+                                       buildID.substring(8, 10),
>+                                       buildID.substring(10, 12),
>+                                       buildID.substring(12, 14));

We'll need to figure out a better solution here... It really sucks that nsIScriptableDateFormat is so sucky :(

>diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul

>+  <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>

>+    <key key="VK_ESCAPE" oncommand="window.close();"/>

Oops, mistake in my example code - this needs to use keycode="VK_ESCAPE" to work.

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>+function setupCheckForUpdates(checkForUpdates)

This function tries to use document.getElementById("bundle_browser"), which fails on non-Mac. I guess you should refactor it to take a reference to a stringBundle, and have the utilityOverlay caller pass strings.stringBundle (will also require some changes to getStringWithUpdateName to use the nsIStringBundle API rather than the <stringbundle> one, e.g. s/getFormattedString/formatStringFromName/)
Attachment #475027 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 100

8 years ago
Created attachment 475180 [details] [diff] [review]
patch v9

This should (hopefully) address all the concerns. I can file a follow-up bug for the localized "released" date.
Attachment #475027 - Attachment is obsolete: true
Attachment #475180 - Flags: review?(gavin.sharp)
Comment on attachment 475180 [details] [diff] [review]
patch v9

>diff --git a/browser/base/content/baseMenuOverlay.xul b/browser/base/content/baseMenuOverlay.xul

>+#ifdef XP_MACOSX
>         <menuitem id="checkForUpdates"
>+#endif

This means that you can remove the #checkForUpdates[loading="true"] rules in browser.css for winstripe/gnomestripe.

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

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

var browserBundle = document.getElementById("bundle_browser").stringBundle;

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+# About Firefox Dialog
>+# LOCALIZATION NOTE (aboutdialog.released): %1$S = year, %2$S = month, 
>+# %3$S = day

Mention that these are just numbers (rather than names), and that they're zero-padded (for day/month)?

Doing it this way now removes our ability to change this after string freeze - if you kept things as just "%s = date", we could potentially change the format later. Though I guess that could potentially have some l10n impact anyways? Get Axel to make the call, I guess?

>+aboutdialog.released=(released %2$S-%3$S-%1$S)

ISO 8601 is the one true date format! (grumble, I guess this is en-*US*...)

>diff --git a/browser/branding/unofficial/locales/en-US/brand.dtd b/browser/branding/unofficial/locales/en-US/brand.dtd

> <!-- LOCALIZATION NOTE (releaseBaseURL): The about: page appends __MOZ_APP_VERSION__.html, e.g. 2.0.html -->
> <!ENTITY  releaseBaseURL        "http://www.mozilla.org/projects/devpreview/releases/">

Hate to scope creep, but can you remove this while you're at it? It's unused (was forgotten in bug 348076).

r=me with those addressed. Yay!
Attachment #475180 - Flags: review?(gavin.sharp) → review+

Comment 102

8 years ago
I'm not a friend of re-doing this after b7 for fx4 wrt dates. Also, we should expose the order of the date parts to l10n. No idea how we'd do this better without l10n impact, too.

I'd suggest to go with this.
You'd suggest we go with ... what?
(Assignee)

Comment 104

8 years ago
Created attachment 475211 [details] [diff] [review]
final patch
Attachment #475180 - Attachment is obsolete: true

Comment 105

8 years ago
(In reply to comment #103)> You'd suggest we go with ... what?Expose the date format to 1lon.

Comment 106

8 years ago
Comment on attachment 475211 [details] [diff] [review]
final patch

With one like this.

Possible nit, you could just strip a leading '0' for day and month to make it look more humane.

I'm open to follow up bugs for Firefox 4.next to make our date formatting not suck, too.
Attachment #475211 - Flags: feedback+
Ship it. Margaret - accurate to say this is checkin-needed now?
Attachment #469774 - Attachment is obsolete: true
Attachment #469774 - Flags: ui-review?(faaborg)
Following my comment 7 on bug 571424. (sorry again for dup)
It could be nice to add a link on "We believe that the internet should be open, public and accessible by everyone without any restrictions." or on a following "Learn more..." link to a web page as an entrance door to the free software/open source world.
I first think about the Organic one : http://www.mozilla.com/en/firefox/organic/ (or maybe to http://www.mozilla-europe.org/en/firefox/organic/ as it more localized, aka,try french on both, only the last one work right).
(In reply to comment #108)
My previous proposal depend on the (ln10) string freeze, so we might consider it before this deadline. (just a late but important thought)

Comment 110

8 years ago
I'm not in favor of creeping the scope more, also, the "organic" messaging isn't working equally well around the globe.

Comment 111

8 years ago
I'm also not comfortable with the language proposed in comment 108, I think it goes too far.
(Assignee)

Comment 112

8 years ago
There's no more room for changes in the language in this bug. I would say that anyone who wants to change the language should file a follow-up bug, but a lot of discussion went into reaching the current language, so I don't think it's going to change for Firefox 4.
(Assignee)

Comment 113

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a1d900b2c5a4
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Bustage fix: http://hg.mozilla.org/mozilla-central/rev/9a2b165409b6

This removed "about.png" from the branding dir, which is what about:logo is tied to (see nsAboutRedirector.cpp). A handful of tests all refer to this (some seem to care about it's size, sigh), and I suppose we should avoid breaking other apps, so I just relanded the old about.png images.

Honestly, I'm not even sure why about:logo is around, we should probably just kill it off at some point. But not as a bustage fix. :)

Updated

8 years ago
Blocks: 596486
(In reply to comment #95)
> - I find the below grey part having a too subtle difference from the top white
> part. On my screen it's barely noticeable.

Yeah, probably worth a followup.

> - I find a bit strange that each link opens a new separate window

That's bug 263433.

> stupid question. I'm not English so it's really a question, should not this be
> "Sounds interesting?"

It's a contraction of "does this sound interesting?", so no :)

Updated

8 years ago
Depends on: 263433
(In reply to comment #114)
> Honestly, I'm not even sure why about:logo is around, we should probably just
> kill it off at some point. But not as a bustage fix. :)
Thunderbird and SeaMonkey still use about:logo and Sunbird and Camino used to use it although I'm not sure whether they still do (they would if they used Toolkit's about: page for instance.)
Blocks: 596535
Added a following bug 596535 : Add a link in About window to make Firefox
Mobile more discoverable for Firefox Desktop users.
No longer blocks: 596535

Updated

8 years ago
Depends on: 596562

Updated

8 years ago
Blocks: 596813
Depends on: 596905

Updated

8 years ago
Depends on: 597012

Comment 118

8 years ago
Verified fixed.  Looks and works great!

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 4.0b7

Updated

8 years ago
No longer depends on: 597528

Comment 119

8 years ago
Created attachment 476646 [details]
screenshot with checked-in patch

Beautiful!

Comment 120

8 years ago
(In reply to comment #119)
> Created attachment 476646 [details]
> screenshot with checked-in patch
> 
> Beautiful!

Indeed, quite a wonderful improvement. :)

However, it looks like Mac may be in the Linux camp with respect to the missing released date. If so, please note it in bug 597012.
The trademark text doesn't show up on Windows Minefield.

Comment 122

8 years ago
(In reply to comment #121)
> The trademark text doesn't show up on Windows Minefield.

That is because the Minefield logo is not trademarked. Trunk builds are meant to be completely free, and since the Firefox logo is copyrighted...

Comment 123

8 years ago
Which means that Jesse's screenshot is an odd combination of Firefox branding with a Minefield version, by the way. (and why I'm not 100% sure if Mac gets bug 597012 or not)
My point is that we regressed bug 569057 with this change since it was added back to About:Dialog recently and I would agree Minefield is also not a Charlton Trademark, but Firefox is.
(In reply to comment #124)
> My point is that we regressed bug 569057 with this change since it was added
> back to About:Dialog recently and I would agree Minefield is also not a
> Charlton Trademark, but Firefox is.

I don't understand that at all. "regressed bug 569057" would mean that the Charlton text doesn't appear in Firefox-branded builds. Is that what you're saying happens? It not being displayed in Minefield builds isn't a bug, it's intentional.
(In reply to comment #125)
> (In reply to comment #124)
> > My point is that we regressed bug 569057 with this change since it was added
> > back to About:Dialog recently and I would agree Minefield is also not a
> > Charlton Trademark, but Firefox is.
> 
> I don't understand that at all. "regressed bug 569057" would mean that the
> Charlton text doesn't appear in Firefox-branded builds. Is that what you're
> saying happens? It not being displayed in Minefield builds isn't a bug, it's
> intentional.

Well it was in Minefield till this bug was fixed, because it keeps getting taken out.  If it shows up in the next release build then we're good, but I cannot test that. None of the screenshots or comments mention it shouldn't be build for non branded builds as bug 569057 put it back in because it keeps getting taken out.

Can you confirm the patch that went in will show up in Firefox-branded builds?
I'm pretty sure that the Charlton text was never included for Minefield builds from the trunk. (It was included for branch unbranded builds given the hack we used to add it back there, though.)

My local branded build shows it just fine: http://grab.by/6t3E

(Email me if you want to discuss this further - we don't need to continue spamming all the nice people watching this bug.)
(In reply to comment #127)
> I'm pretty sure that the Charlton text was never included for Minefield builds
> from the trunk. (It was included for branch unbranded builds given the hack we
> used to add it back there, though.)
 
Just wanted to clear the air here, I double checked a few builds before and after that bug, your right, sorry, I was wrong, thinking I seen it in the dialog too before with the Minefield logo.

> My local branded build shows it just fine: http://grab.by/6t3E

Thanks for the update.
(In reply to comment #122)
> (In reply to comment #121)
> > The trademark text doesn't show up on Windows Minefield.
> 
> That is because the Minefield logo is not trademarked. Trunk builds are meant
> to be completely free, and since the Firefox logo is copyrighted...

Then the trademark area of the dialog should be collapsed, either by making the "content" longer of making the whole dialog shorter. The big, empty space at the bottom of the dialog doesn't look good, and it gives the false impression that something should be there.
(In reply to comment #129)
> (In reply to comment #122)
> > (In reply to comment #121)
> > > The trademark text doesn't show up on Windows Minefield.
> > 
> > That is because the Minefield logo is not trademarked. Trunk builds are meant
> > to be completely free, and since the Firefox logo is copyrighted...
> 
> Then the trademark area of the dialog should be collapsed, either by making the
> "content" longer of making the whole dialog shorter. The big, empty space at
> the bottom of the dialog doesn't look good, and it gives the false impression
> that something should be there.

We've wandered beyond the meat of fixing this bug - if you have follow ups to file, by all means, but let's stop spamming the 50 people on this cc list, please?
Depends on: 598860
If I may ask, why bottom strip is styled in general Windows style? It breaks consistency with Win7.
We'll fix that for windows (I think that's the OS X style being carried over at the moment)
Shouldn't the style rules for the about window live in the skin provider instead of content? This makes things difficult for themers, as they have to specifically override those rules.

Comment 134

8 years ago
(In reply to comment #133)
> Shouldn't the style rules for the about window live in the skin provider
> instead of content? This makes things difficult for themers, as they have to
> specifically override those rules.

The about dialog isn't supposed to be re-themed much. In fact, AMO has a policy of telling theme and extension developers to leave the about dialog alone.
(In reply to comment #134)
> 
> The about dialog isn't supposed to be re-themed much. In fact, AMO has a policy
> of telling theme and extension developers to leave the about dialog alone.

I can agree with this regarding to the branding images, but not for the general appearance of it. So I think we could maintain the rules for branding styles on content and move the other rules to the skin provider.

Notice that since this window imports the global rules, it indeed is themed and follow specifically rules from the used theme like, e.g., background and font colors.

Maybe I wouldn't touch it if it doesn't break things on my theme. Just as an example, I usually have rules for buttons where I specify padding. The aboutDialog.css overrides these rules and I need to create a new global rule just for the buttons on this window. I also needed to use the important flag since increasing the specificity using a child selector didn't work on this case. I guess other themers can also experience similar annoyances.

Also leaving this rules on content doesn't really avoid the window to be themed, just makes it more difficult and at risk of breaking things on other themes than the default.

Updated

8 years ago
Depends on: 600568

Updated

8 years ago
Depends on: 600569

Updated

8 years ago
No longer depends on: 600568, 600569

Updated

8 years ago
Blocks: 601049
I have few suggestions:
1. Can we move about window into DH pop-up, please?
2. Ca we have acces directly from FXB? Something like question mark inside FXB on the left side.
Depends on: 606651

Updated

7 years ago
Blocks: 624941
Depends on: 626413

Updated

7 years ago
Blocks: 618522

Updated

7 years ago
Depends on: 646473

Updated

7 years ago
Depends on: 649732

Updated

7 years ago
Blocks: 536108

Updated

6 years ago
Depends on: 737215
You need to log in before you can comment on or make changes to this bug.