The Updates pane should include the current version of Firefox and whether the browser is up-to-date

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: jaws, Assigned: timdream)

Tracking

(Blocks: 3 bugs)

55 Branch
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 4 obsolete attachments)

Created attachment 8858235 [details]
Screenshot of spec

See spec: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167550

This functionality should be copied from the About dialog. As part of this work we should make a shared module so that the code doesn't get duplicated.
(Reporter)

Updated

3 months ago
Blocks: 1324168
No longer blocks: 1335907
Hi Evan, can you pick this up? We might need to add a string for this ("Version %S") and I'd like to get this added sooner rather than later since it introduces a new string.
Flags: needinfo?(evan)

Comment 2

3 months ago
Hi Jared,

Since the Project Photon is our team's top priority task, I'm working on Photon-related bugs. We might need to confirm this bug is included in the Photon scope before I work on that. And this is Cindy's call.

Cindy, do you know is this included in the Photon scope?

Thanks.
Flags: needinfo?(evan) → needinfo?(chsiang)

Comment 3

3 months ago
Hi Evan,
Yes this one is within the scope. I've added the whiteboard tag to track it.
Flags: needinfo?(chsiang)
Whiteboard: [photon-preference]
(Reporter)

Updated

3 months ago
Blocks: 1356269
(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #0)
> Created attachment 8858235 [details]
> Screenshot of spec
> 
> See spec: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167550
> 
> This functionality should be copied from the About dialog. As part of this
> work we should make a shared module so that the code doesn't get duplicated.

Does this also replicate the update checking and "Apply update" button?

IIRC there were some old limitations on having this stuff displayed in multiple places (e.g. having multiple about:prefs tabs open, or both this page and the About dialog). I can't recall exactly what the issue was or if it still apply... rstrong, you remember this?
Flags: needinfo?(robert.strong.bugs)
The info in the about window is not entirely accurate since when that was implemented it was decided that it should state that the browser is up to date when there was an update check failure. The "up to date' message is determined by the update check not finding an update whether an error occurred during the check or not.

There are issues with performing update checks simultaneously from the about window and in the toolkit app update UI simultaneously since the UI's wouldn't show the same state.
Flags: needinfo?(robert.strong.bugs)
Also, when there are repeated consecutive failures we do inform the client. This handles the case where the client is unable to contact the update server for whatever reason without unduly alarming them by notifying them when it is a transient error.

Comment 7

3 months ago
this bug should also include an "update firefox" button if the current version is not the latest. tina will add specs here.
Flags: needinfo?(thsieh)

Updated

3 months ago
Priority: -- → P1
From my end of things I'm focused on making updates as silent and reliable as possible. It should be possible to duplicate the code from the Firefox about dialog to add this functionality for the few users that would get value out of this.
Comment 5 is a bit troubling. I don't think it's feasible to refactor aboutDialog-appUpdater.js in the fx55 cycle to allow multiple UIs update at the same time. If we could accept that as a post-55 follow-up we should not try to get this UI into fx55. Cindy, what do you think?
Flags: needinfo?(chsiang)
Target Milestone: --- → Firefox 55

Comment 10

3 months ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> Comment 5 is a bit troubling. I don't think it's feasible to refactor
> aboutDialog-appUpdater.js in the fx55 cycle to allow multiple UIs update at
> the same time. If we could accept that as a post-55 follow-up we should not
> try to get this UI into fx55. Cindy, what do you think?

I think it's reasonable to push it to after 55.
Flags: needinfo?(chsiang)
Tina,

For the release version, on the about dialog we will have a "What's New" link followed by the version number.

Also if the Firefox is a distribution build, we will show the distribution and distribution ID on the second line. See attachment 8572733 [details] from bug 1139509.

Do you want to show the "what's new" link? Do you want to show, and what's the proposed style of, distribution desc and ID here?
Assignee: nobody → timdream
(Tentatively assign to myself. Might ask other to grab if occupied)
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Created attachment 8862394 [details]
With "Check for updates" button
Flags: needinfo?(thsieh)
Attachment #8862394 - Flags: ui-review?(thsieh)
Created attachment 8862395 [details]
Up-to-date label
Attachment #8862395 - Flags: ui-review?(thsieh)
Created attachment 8862396 [details]
With distribution description and id

I found another screenshot on distribution label: https://www.soeren-hentzschel.at/wp-content/uploads/2012/07/firefox-funnelcake.png
Attachment #8862396 - Flags: ui-review?(thsieh)
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

I used the existing XUL components to put the labels without the spec. Tina should check if they are alright. Jared, this patch is on top of bug 1330121 and it ... works. I wonder if you would like to duplicate tests to test the updater also, or rename the aboutDialog-appUpdater.js. I'll update the patch accordingly.
Attachment #8862392 - Flags: feedback?(jaws)
Comment hidden (mozreview-request)
Created attachment 8862400 [details]
With what's new link
Attachment #8862400 - Flags: ui-review?(thsieh)
Attachment #8862396 - Attachment filename: Screen Shot 2017-04-27 at 7.55.58 PM.png → With distribution description and id
Attachment #8862396 - Attachment description: Screen Shot 2017-04-27 at 7.55.58 PM.png → With distribution description and id
Attachment #8862396 - Attachment filename: With distribution description and id → Screen Shot 2017-04-27 at 7.55.58 PM.png
Attachment #8862395 - Attachment description: Screen Shot 2017-04-27 at 7.56.36 PM.png → Up-to-date label
Attachment #8862394 - Attachment description: Screen Shot 2017-04-27 at 7.56.59 PM.png → With "Check for updates" button
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

Update style on the release note link.
Attachment #8862392 - Flags: feedback?(jaws)
(Reporter)

Comment 21

3 months ago
mozreview-review
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

https://reviewboard.mozilla.org/r/134340/#review137454

feedback+ from me, looks good!

::: browser/components/preferences/in-content/preferences.xul:40
(Diff revision 2)
>  <!ENTITY % contentDTD SYSTEM "chrome://browser/locale/preferences/content.dtd">
>  <!ENTITY % applicationsDTD SYSTEM
>    "chrome://browser/locale/preferences/applications.dtd">
>  <!ENTITY % advancedDTD SYSTEM
>    "chrome://browser/locale/preferences/advanced.dtd">
> +<!ENTITY % aboutDialogDTD SYSTEM "chrome://browser/locale/aboutDialog.dtd" >

Please add a note in aboutDialog.dtd that these strings are used from two different places. Flod, is this usage OK with you?
(regarding question in comment 21)
Flags: needinfo?(francesco.lodolo)
(Reporter)

Updated

3 months ago
Attachment #8862392 - Flags: feedback?(jaws) → feedback+

Updated

3 months ago
QA Contact: hani.yacoub
Comment hidden (obsolete)
Blocks: 1330121
No longer blocks: 1330121
... and bug 1330121 needs to land first.
Depends on: 1330121
No longer depends on: 1357348
(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #22)
> (regarding question in comment 21)

Thanks for flagging. I've checked and it's over 20 strings that we would need to port, so I'm OK with reusing them, especially considering how tricky they are (some locales fail to grasp the concept of 3 concatenated strings).

There's one concern though, and that's accesskeys (3 of them).

As Jared said, please add a comment to aboutDialog.dtd explaining that some strings are reused in the new preference panel.

Also add a similar comment here, explaining that strings from aboutDialog.dtd are displayed in the same section, and that might create accesskey conflicts
https://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/preferences/advanced.dtd#l86
Flags: needinfo?(francesco.lodolo)
Comment hidden (mozreview-request)

Comment 27

3 months ago
mozreview-review
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

https://reviewboard.mozilla.org/r/134340/#review137582

::: commit-message-5765e:3
(Diff revisions 2 - 3)
> -Bug 1356507 - Show version and updater in the preferences update pane.
> +Bug 1356507 - Show version and updater in the preferences update pane. r=jaws
>  
> -This quick patch includes unmodified aboutDialog-appUpdater.js into
> +This change ncludes unmodified aboutDialog-appUpdater.js into

typo: ncludes

::: browser/locales/en-US/chrome/browser/aboutDialog.dtd:7
(Diff revision 3)
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/.  -->
>  <!ENTITY aboutDialog.title          "About &brandFullName;">
>  
> +<!-- LOCALIZATION NOTE (update.*):
> +# Thse strings are used being used in the update pane of preferences also;

various typos: "These strings are also used in the update pane of preferences."

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:88
(Diff revision 3)
>  
>  <!ENTITY updateTab.label                 "Update">
>  
> +<!-- LOCALIZATION NOTE (updateApplication.label):
> +  Strings from aboutDialog.dtd are displayed in this section of the preferences.
> +  Please be aware of accesskey conflicts.

Maybe "Please check for possible accesskey conflicts".
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

3 months ago
Flags: qe-verify+
(Reporter)

Comment 30

3 months ago
mozreview-review
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

https://reviewboard.mozilla.org/r/134340/#review137740
Attachment #8862392 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Is this ready to land now?
Flags: needinfo?(timdream)
(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #32)
> Is this ready to land now?

I am waiting for Tina to confirm the UI (comment 11).
Flags: needinfo?(timdream)
Update: Tina and I had a short conversation about this.

1. She would like to have the distribution labels removed.

2. She have problem with the update buttons, which the new pref design need it to be on the right. However that means we will need a new copy string on the left side, which will take even longer to finalize. I will wait for her to make a decision (including the option of leaving the patch as-is).
Tim, thanks for the updates! :)

I'll ask the copywriter if we can have her recommendation for the new string. Since the schedule is tight, I'm OK with just moving the button to the right hand side without a new string. I see the new string is related to the content work for Photon, so let's add the new string no later than v57.

BTW, Can we add the "Version" in front of the version number as how the spec looks like?
I'm not sure why the distribution labels would be removed and there is no reasoning given in the bug. The distribution labels are helpful if someone is trying to report an issue with Firefox, though I guess that is what about:support is for. We show the distribution labels on the About dialog though, I don't think this bug is where we should make the decision to stop showing them. I would prefer the discussion about hiding the distribution labels be moved to a new bug, and we keep the About dialog and the Updates pane consistent with each other.

Moving the button to the right means we should always show the button there.

@Tim / @Tina, to prevent having to create new strings right now, can we always show status-related messages as text on the left side, and then actions that require a button on the right side? When a status-related message is unavailable (such as prior to checking for updates), we should just show nothing on the left side. We can also disable the [Check for updates] button while we are checking for updates, downloading an update, or applying an update.

For example...

Before checking for updates:
<blank string>                 [Check for updates]

While checking for updates:
Checking for updates...        [Check for updates (disabled)]

After update has been applied:
<blank string>                 [Restart to update Firefox]
Hey Jared,
That makes sense to me. The reason why I suggested hiding the distribution labels was because it looked hard to understand. But I agree that we should be consistent with what Firefox shows in the About dialog. I believe that we can solve the issue by UX/Visual, so let's file a new bug for fixing the complex version details.
Comment hidden (mozreview-request)
Attachment #8862394 - Attachment is obsolete: true
Attachment #8862394 - Flags: ui-review?(thsieh)
Attachment #8862395 - Attachment is obsolete: true
Attachment #8862395 - Flags: ui-review?(thsieh)
Attachment #8862396 - Attachment is obsolete: true
Attachment #8862396 - Flags: ui-review?(thsieh)
Attachment #8862400 - Attachment is obsolete: true
Attachment #8862400 - Flags: ui-review?(thsieh)
The latest revision moves the updater UI mark up and add disabled buttons per comment 36. I've also add the missing space b/t labels and the throbber. Let's go ahead and land this without waiting for further review from :jaws, given that he is away.
Keywords: checkin-needed
Blocks: 1361640

Comment 40

3 months ago
mozreview-review
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

https://reviewboard.mozilla.org/r/134340/#review138756

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:89
(Diff revisions 6 - 7)
>  <!-- LOCALIZATION NOTE (updateApplication.label):
>    Strings from aboutDialog.dtd are displayed in this section of the preferences.
>    Please check for possible accesskey conflicts.
>  -->
>  <!ENTITY updateApplication.label         "&brandShortName; Updates">
> +<!ENTITY updateApplication.version       "Version ">

Is there a screenshot for the latest iteration?

You're concatenating this with another string, which means you need 
a) a localization note explaining it, so localizers will know why the space is needed
b) a "post" string after the version number, that will remain empty in English but not necessarily in other languages.
Created attachment 8864011 [details]
Screenshots.zip

As asked by Tina, attaching screenshots of the patch to land (and bug 1361640) for her to plan future works.
I will need to address comment 40.
Keywords: checkin-needed
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 44

3 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2768a8779068
Show version and updater in the preferences update pane. r=jaws
Keywords: checkin-needed
Backed out for eslint failure:

https://hg.mozilla.org/integration/autoland/rev/f97d552d92a0529919e9f9957ee5e92dae1d3484

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2768a87790681efc0fe64df26c363ef7381759a6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=96165665&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/preferences/in-content/advanced.js:72:7 | 'gAppUpdater' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/preferences/in-content/advanced.js:72:25 | 'appUpdater' is not defined. (no-undef
Flags: needinfo?(timdream)
Comment hidden (mozreview-request)
Sorry. Should be good now.
Flags: needinfo?(timdream)

Comment 48

3 months ago
mozreview-review
Comment on attachment 8862392 [details]
Bug 1356507 - Show version and updater in the preferences update pane.

https://reviewboard.mozilla.org/r/134340/#review138784

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:89
(Diff revision 9)
> +<!-- LOCALIZATION NOTE (updateApplication.label):
> +  Strings from aboutDialog.dtd are displayed in this section of the preferences.
> +  Please check for possible accesskey conflicts.
> +-->
>  <!ENTITY updateApplication.label         "&brandShortName; Updates">
> +<!-- LOCALIZATION NOTE (updateApplication.version.pre): include a trailing space as needed -->

Better

# LOCALIZATION NOTE (updateApplication.version.*): updateApplication.version.pre
# is followed by a version number, keep the trailing space or replace it with a
# different character as needed. updateApplication.version.post is displayed
# after the version number, and is empty on purpose for English. You can use it
# if required by your language.
Comment hidden (mozreview-request)
There are also test failures like this for the previous push: https://treeherder.mozilla.org/logviewer.html#?job_id=96179485&repo=autoland
Flags: needinfo?(timdream)
Thanks for pointing that out. Yeah I can produce that locally although I don't really know what cause it. It's going to take some effort to fix that XPCOM thing. I hope I could find some way to workaround it.
Flags: needinfo?(timdream)
See Also: → bug 1361929
Comment hidden (mozreview-request)
Created attachment 8864380 [details]
Screen Shot - Funnelcake UI.png

As asked by Tina.
Blocks: 1361947

Comment 54

3 months ago
mozreview-review
Comment on attachment 8864377 [details]
Bug 1356507 - Workaround defineLazyServiceGetter() as described in bug 1361929,

https://reviewboard.mozilla.org/r/136056/#review139132

Try looks green, let's try landing it again!
Attachment #8864377 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed

Comment 55

3 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28a9741054a2
Show version and updater in the preferences update pane. r=jaws
https://hg.mozilla.org/integration/autoland/rev/7b179818adc2
Workaround defineLazyServiceGetter() as described in bug 1361929, r=mikedeboer
Keywords: checkin-needed

Comment 56

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28a9741054a2
https://hg.mozilla.org/mozilla-central/rev/7b179818adc2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
See Also: → bug 1364653

Comment 57

2 months ago
This bug  was about  to "Updates pane should include current version of Firefox and whether the browser is up-to-date". 

I have seen the feature being implemented with latest Nightly 55.0a1 on Windows 8.1 (64 bit).

This bug's fix is now verified in Latest Nightly.

Build ID : 20170517030204
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday 20170517]
This bug  was about "including the current version of Firefox and whether the browser is up-to-date
in the Updates pane". I have seen the feature being implemented with latest Nightly on Ubuntu 16.04 LTS !

This bug's fix is now verified in Latest Nightly!

Build ID   :  20170518100156
User Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170517]
 As per Comment 57 & Comment 58, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.