Closed Bug 1443771 Opened 6 years ago Closed 6 years ago

Change Firefox about dialogue visual on ESR

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox60 blocking fixed
firefox61 --- fixed

People

(Reporter: RT, Assigned: amin)

References

Details

Attachments

(4 files)

Firefox Quantum on ESR about.png
72.53 KB, image/png
Details
firefox-enterprise-about-dialog.jpg
476.69 KB, image/jpeg
Details
With my code change
208.16 KB, image/png
Details
Bug 1443771 - Add ESR name to the about dialog. It is not translated.
59 bytes, text/x-review-board-request
Felipe
: review+
jcristau
: approval-mozilla-beta+
Details 
The current ESR about dialogue visual (https://dxr.mozilla.org/comm-esr52/source/mozilla/browser/branding/official/content/about-wordmark.png) does not align with current release Firefox Quantum branding and after discussions with product and content marketing the recommendation is to update the visual as follows:
- 'Firefox Quantum', with subcopy of "Extended Support Release" underneath (see attached)

Notes:
- This has to be done on the ESR tree, not central
- The visual will be provided on this bug
- This does not require string changes
Amin, can you please help us with producing the visual?
Flags: needinfo?(aalhazwani)
Tracked as blocking for 60/ESR60 to ensure this gets landed in a timely manner and is closely monitored by relman team.
status-firefox60: --- → affected
tracking-firefox60: --- → blocking
(In reply to Romain Testard [:RT] from comment #1)
> Amin, can you please help us with producing the visual?

Here you can find the SVG version of the logo https://design.firefox.com/photon/visuals/product-identity-assets.html#firefox. For size and placement please refer to the Firefox Release about dialog.

Do we have any update on the name? Once we have a decision there I can also prepare the SVG for that as well!
Flags: needinfo?(aalhazwani)
Amin, I am assigning this bug to you, feel free to reassign to whoever is supposed to land this change.
Assignee: nobody → aalhazwani
Status: NEW → ASSIGNED
Priority: -- → P1
About dialog for Firefox Enterprise, Romain could you assign this bug to who is going to implement it? Thanks!

The assets are currently hosted here: https://dxr.mozilla.org/mozilla-central/source/browser/branding/official/content

- about-logo.png
- about-logo@2x.png
- about-woodmark.svg

Would be possible to also have the about-logo as an SVG? You can find the SVG file here https://design.firefox.com/photon/visuals/product-identity-assets.html#firefox
Flags: needinfo?(rtestard)
Thanks Amin
Panos, can you please help assign this?
Also NI Felipe in case he's able to look into it.
Flags: needinfo?(rtestard)
Flags: needinfo?(past)
Flags: needinfo?(felipc)
This is Justin's turf.
Flags: needinfo?(past) → needinfo?(dolske)
Blocks: 1433173
I can't commit to this right now as my main focus is getting the policies implemented. However, the biggest chunk of work here will be from UX to produce the new image; once that's done, replacing the old one can probably be done very quickly by any front-end engineer.
Flags: needinfo?(felipc)
Note that the main holdup here is the art and the art would have to be redone anyway (because it uses the old Firefox logo).

Who would be on the hook to redo the art work? I think Stephen Horlander did the Quantum work.
Flags: needinfo?(shorlander)
Hi there, as I explained in comment#5 all visual assets are already available on m-c.
Flags: needinfo?(shorlander)
so then are we going to add the Extended Support Release part as text?
If possible, yes.
Attached image With my code change — 
Here's what it looks like with the patch I'm about to attach.

I went with 125% over existing font.
Updated patch adds a margin on the right so it looks correct in RTL languages.

And before folks ask, I checked all the major languages of https://www.mozilla.org/en-US/firefox/organizations/all/.

Extended Support Release is NOT translated, so this is not a string for translation.

We'll also need to remove the hack from the ESR branch when we do this.
Flags: needinfo?(dolske)
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

https://reviewboard.mozilla.org/r/227880/#review234050

The code looks to me, but a couple of things before going ahead:

- You should check with Flod about having this string as an untranslatable string, and the desire to uplift this to beta

- I had heard that there was some sort of hack during the build process of ESR that replaced the (now) "Firefox Quantum" image with the older "Firefox ESR" image. We should make sure that won't be a problem, and check if there are any images to be removed from the tree and specially from being shipped but unused.

::: browser/base/content/aboutDialog.css:34
(Diff revision 2)
> +#esr {
> +  font-weight: bold;
> +  font-size: 125%;
> +  margin-top: 10px;
> +  margin-left: 0;
> +  margin-right: 0;

if this `margin-right: 0` was done to support RTL languages, you should instead use `margin-inline-start: 0`, which applies as margin-left on LTR and margin-right on RTL.
Attachment #8959016 - Flags: review?(felipc) → review+
flod said:

"the string should be hard coded in the XUL/HTML template, or JS code. If you expose it, it will be translated for sure"

As far as the ESR hack, I plan to open a bug against release engineering. There was some hack that was done in 38 that has been carried forward.

> if this `margin-right: 0` was done to support RTL languages, you should instead use `margin-inline-start: 0`, which applies as margin-left on LTR and margin-right on RTL.

Good point. I was copying the other code.
I've changed my tack on this a little. I made release generic in the XUL and set the title via JS. That way this is more general than just the ESR case.
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

Requesting Jared's feedback on the new patch since he seems to be the goto person for about changes.
Attachment #8959016 - Flags: feedback?
Mike: the feedback flag here is blank...
Flags: needinfo?(mozilla)
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

doh! Also doing a needinfo.
Flags: needinfo?(mozilla) → needinfo?(jaws)
Attachment #8959016 - Flags: feedback? → feedback?(jaws)
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

https://reviewboard.mozilla.org/r/227880/#review234842

::: browser/base/content/aboutDialog.js:83
(Diff revision 4)
>          currentChannelText.hidden = true;
>    }
>  
> +  if (AppConstants.MOZ_UPDATE_CHANNEL == "esr") {
> +    let release = document.getElementById("release");
> +    release.textContent = "Extended Support Release";

Can you add a comment here to say that this is explicitly not localizable?

> I've changed my tack on this a little. I made release generic in the XUL and set the title via JS. That way this is more general than just the ESR case.

Since this isn't used outside of ESR I'm not sure what we're gaining by setting the string via JS. I'd prefer to keep the string in the markup, and then here we would just make it visible.
Attachment #8959016 - Flags: review+
Flags: needinfo?(jaws)
Attachment #8959016 - Flags: feedback?(jaws)
> Since this isn't used outside of ESR I'm not sure what we're gaining by setting the string via JS. I'd prefer to keep the string in the markup, and then here we would just make it visible.

I'm picturing a world where we might have other custom releases and use this string. But I can do it thr other way if you think that's better.
Blocks: 1447399
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

https://reviewboard.mozilla.org/r/227880/#review234842

> Can you add a comment here to say that this is explicitly not localizable?
> 
> > I've changed my tack on this a little. I made release generic in the XUL and set the title via JS. That way this is more general than just the ESR case.
> 
> Since this isn't used outside of ESR I'm not sure what we're gaining by setting the string via JS. I'd prefer to keep the string in the markup, and then here we would just make it visible.

Comment added and code moved.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/0fd93dddaf2c
Add ESR name to the about dialog. It is not translated. r=Felipe,jaws
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

Approval Request Comment
[Feature/Bug causing the regression]: ESR identifier
[User impact if declined]: Unable to identify ESR
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: No. Can't until we have an ESR beta
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: no
[Why is the change risky/not risky?]: Only affects ESR
[String changes made/needed]: None
Attachment #8959016 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/0fd93dddaf2c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

about dialog update for esr60, beta60+
Attachment #8959016 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

https://reviewboard.mozilla.org/r/227880/#review248812

::: browser/base/content/aboutDialog.js:81
(Diff revision 5)
>      channelLabel.value = UpdateUtils.UpdateChannel;
>      if (/^release($|\-)/.test(channelLabel.value))
>          currentChannelText.hidden = true;
>    }
>  
> +  if (AppConstants.MOZ_UPDATE_CHANNEL == "esr") {

This is an abuse of the MOZ_UPDATE_CHANNEL variable. This makes downstream not show the string because their update channel is "default".
Comment on attachment 8959016 [details]
Bug 1443771 - Add ESR name to the about dialog. It is not translated.

https://reviewboard.mozilla.org/r/227880/#review248812

> This is an abuse of the MOZ_UPDATE_CHANNEL variable. This makes downstream not show the string because their update channel is "default".

I'm not sure what you mean by downstream? I thought the default update channel was only used for private builds, never in release.
downstream meaning Linux distros.
(In reply to Julien Cristau [:jcristau] from comment #36)
> downstream meaning Linux distros.

Unfortunately there is no other way to identify the ESR except for the update channel.

Can you think of something we missed?
Adding needinfo for this question. How do you suggest we identify the ESR?

Do we know how the distros are setting the correct build flags for building ESR?
Flags: needinfo?(mh+mozilla)
There was bug 1432737, but it was backed out :(
ESR used to have a different branding content, did that go away?
Flags: needinfo?(mh+mozilla)
> ESR used to have a different branding content, did that go away?

Yes. We were going to have to redo the logo anyway (for Quantum) and we thought just adding text would be easier.

I didn't realize that the reason they went with an image is so it could be easily checked into the branch and downstream folks wouldn't have to worry about it.

This is going to come up with other things too (policy and autoconfig) so we're probably going to need an easy way to identify ESR anyway.

I wonder if bug 1432737 will make it in eventually?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: