368266 - The "RSS subscription" dialog should have an "OK" button

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Description

[Alfred Peng]

Thunderbird version 3 alpha 1 (20070115) on OpenSolaris.

Bring up Account Settings, select the RSS account, then click the Manage Subscriptions button.

Actual result:
The users have to click right-up conner close button to close.

Expected result:
There should have an "OK" button.

Comment 1

[Magnus Melin [:mkmelin]]

I think this is is intended. See e.g. bug 183419 comment 34. 

Comment 2

[Wayne Mery (:wsmwk)]

UI man.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

This sounds like a windows vs. mac thing.  I believe the Mac HIG recommends against a close button for dialogs, in favour of the button in the title bar.  However windows and linux/gnome recommend explicit close/ok buttons for most dialogs.

Comment 5

[alta88]

This bug should either be closed or fixed. I lean toward adding a Close button, on the same line as the other 5, with a flex between them and Close.

Comment 6

[Josiah Bruner [:jsbruner]]

Yes, we should add a close button.

Windows and OS X definitely require a close button, separate from the default window controls. In fact, even OS X does in this case.

As seen from: https://developer.apple.com/library/mac/documentation/userexperience/conceptual/applehiguidelines/Windows/Windows.html

Only "panels" (popup windows that float on top) and document windows, do not require a close button. Dialogs and Alerts on the other hand do. Technically this window is a document window as the Cocoa APIs define it, though we use it more like a dialog ourselves.

Therefore I see no reason to special case OS X. Let's just add a "Done" button to the right of the existing buttons.

Comment 7

[Richard Marti (:Paenglab)]

I'm also okay with a close button, although we have already a lot of buttons on one line in this small (in default) dialog.

Comment 8

[alta88]

Attachment: closebutton.patch

It's not ideal given the existing button row, but a second row would be worse.  The text is 'Close', as this implies neither update/apply (like Done or Ok) nor undo/cancel.

The newsgroup subscribe dialog has Ok and Cancel in both win and linux.  The options/preference dialog has Ok and Cancel on win and Close on linux, fwiw.  Seems Ok and Cancel aren't right on instant apply dialogs.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

(I think you've got the info you needed, Alta.  ;)

Comment 10

[Josiah Bruner [:jsbruner]]

Comment on attachment 8367958 [details] [diff] [review]
closebutton.patch

Review of attachment 8367958 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/704cbe92153e

Comment 12

[Philip Chee]

File Not Found

The file chrome://messenger/locale/preferences/permissions.dtd cannot be found. Please check the location and try again.&f=regular

    Could the item have been renamed, removed, or relocated?
    Is there a spelling, capitalization, or other typographical error in the address?
    Do you have sufficient access permissions to the requested item?

Comment 13

[Philip Chee]

1. You shouldn't re-purpose strings from some other random .DTD since the semantics might be different in languages that aren't English.

2. Instead add an entity to feed-subscriptions.dtd (Both Thunderbird and SeaMonkey please). Your assistance in this matter is greatly appreciated.

Comment 14

[alta88]

Attachment: closebutton2.patch

Comment 15

[alta88]

(In reply to Philip Chee from comment #13)
> 1. You shouldn't re-purpose strings from some other random .DTD since the
> semantics might be different in languages that aren't English.
> 

True, just not in this case.

> 2. Instead add an entity to feed-subscriptions.dtd (Both Thunderbird and
> SeaMonkey please). Your assistance in this matter is greatly appreciated.

Yes, duplication can't be avoided unfortunately.  Sorry for the oversight.

Comment 16

[alta88]

Attachment: closebutton2.patch

Comment 17

[Philip Chee]

Comment on attachment 8369494 [details] [diff] [review]
closebutton2.patch

rs=me for SeaMonkey

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5c89bc7e77e9

Comment 22

[rsx11m]

Comment on attachment 8369494 [details] [diff] [review]
closebutton2.patch

This follow-up patch made it for the 30.0 release cycle (currently comm-beta) but barely missed the 29.0 cut-off, hence quite a few duplicates have been accumulated for SM 2.26. If this should go into a 2.26.1 release it'll need to land on comm-release as well.

Comment 23

[rsx11m]

Comment on attachment 8369494 [details] [diff] [review]
closebutton2.patch

>+++ b/suite/locales/en-US/chrome/mailnews/newsblog/feed-subscriptions.dtd
>+<!ENTITY button.close.label          "Close">

Eh, this has a string change (thanks Ratty!), thus cancelling requests. :-(