599480 - When the about window is opened the app menu's check for updates menuitem is a noop

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Robert Strong (they/them - no direct email)]

Bug 596813 made it so the about window contains a minimal version of the app update ui. On Mac OS X the app menu still contains a check for updates which attempts to open the app update ui but I intentionally added checks in the app update service to prevent this from happening since the two ui's would fight over being listeners for different actions, etc.

So, the app menu's check for updates menuitem should either be removed or focus the about window when it is opened.

Comment 1

[Robert Strong (they/them - no direct email)]

This should block IMO

Comment 2

[[not reading bugmail]]

Doesn't this conflict with bug 306018 that was fixed recently?  I think they didn't want OSX to use the about dialog per bug 585475 comments 11-13.

Comment 3

[Robert Strong (they/them - no direct email)]

It doesn't conflict if UX choose to just focus the about window when it is opened. As for standard locations, the location that is the most standard for apps to provide a check for updates on Windows is under Help so I don't see why it can't be decided by UX for it to different on Mac as it was decided that it should be different on Windows.

Another option would be to disable app update in the about window on Mac OS X.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Should focus the About Window, IMO.

Comment 5

[:Margaret Leibovic]

Attachment: patch

This patch opens the about dialog instead of the update dialog. It seems to do the right thing, but I haven't tested it with a test update available.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 484031 [details] [diff] [review]
patch

Should be fine with this approach

Comment 7

[Justin Dolske [:Dolske]]

Hmm, I've got a general concern about doing this... When the user explicitly selects Check For Update, it's a little confusing to have a giant, wordy About dialog pop up -- of which the update related portion is a small and subtle part. Doubly so because the "About Firefox" and "Check For Update" menuitems are right next to each other, so people might think they misclicked.

Having a separate dialog seems better in that respect. We could just make Check For Updates focus the About window only in the rare event that it's already open (and keep the existing behavior otherwise). I suppose opening the About window would similarly need to raise the update check window if it happens to have already been opened?

If that's too hacky, it might be OK to go with the current patch if we also improve the About window to make the status of updates more obvious. Maybe that's a good idea anyway. Specifically:

  * Keep the checking message+throbber for a minimum of 2 seconds, so the user has a chance to see it's actually doing something (ie, it draws their attention). I've noticed on fast connections the check completes so fast I'm not sure if it's just caching the status or actually doing something.

  * Provide some context (colored?) for the "Apply Update" when present. EG "A newer version is available! [Apply Update]".

Comment 8

[Robert Strong (they/them - no direct email)]

I personally don't see why it is ok to remove Check for Updates... from the help menu on Windows and it isn't ok to remove it from the app menu on Mac OS X.

Comment 9

[Justin Dolske [:Dolske]]

Yeah, per discussion, I think we should just remove the Check For Updates menu on OS X too. Sad to do now that we've finally got it in the right place, but meh. Also, note that neither Chrome nor Safari have a Check For Updates menuitem there.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #7)
> Hmm, I've got a general concern about doing this... When the user explicitly
> selects Check For Update, it's a little confusing to have a giant, wordy About
> dialog pop up -- of which the update related portion is a small and subtle
> part. Doubly so because the "About Firefox" and "Check For Update" menuitems
> are right next to each other, so people might think they misclicked.

That's a good point, and one that I hadn't considered. Likely also the reason why they took it out of Chromium for OSX. The other reason, I suspect, is because they just expect Chrome to automatically update.

Meh, I guess I'm fine with getting rid of the menuItem. We'll have to update all our docs to say "Go to About Firefox" to check for updates. w/e.

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 484031 [details] [diff] [review]
patch

Clearing review, needs patch that just removes the menuitem per last comment.

Comment 12

[:Margaret Leibovic]

Attachment: patch v2

This patch gets rid of the check for updates menuitem. I reverted the changes from bug 306018. I also had to change some references to the updateSeparator element, since I got rid of that.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

We should probably just leave the nsMenuBarX code in for other apps that may want to keep the menu item.

Comment 14

[:Margaret Leibovic]

Attachment: patch v3

New patch that keeps the widget code.

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 485869 [details] [diff] [review]
patch v3

Not sure if there are any tests exercising this code, might want to just run this through try unless you've already checked.

Comment 17

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/726e585dd529

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101106 Firefox/4.0b8pre

Updated the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=9940