Closed Bug 368266 Opened 14 years ago Closed 7 years ago

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

Categories

(MailNews Core :: Feed Reader, enhancement)

x86
All
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: alfred.peng, Assigned: alta88)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
I think this is is intended. See e.g. bug 183419 comment 34. 
Assignee: mscott → nobody
UI man.
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.
Component: RSS → Feed Reader
Product: Thunderbird → MailNews Core
Duplicate of this bug: 529457
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.
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(josiah)
Flags: needinfo?(bwinton)
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.
Flags: needinfo?(josiah)
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.
Attached patch closebutton.patch β€” β€” Splinter Review
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.
Assignee: nobody → alta88
Attachment #8367958 - Flags: review?(josiah)
(I think you've got the info you needed, Alta.  ;)
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(bwinton)
Comment on attachment 8367958 [details] [diff] [review]
closebutton.patch

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

Looks fine to me. Thanks!
Attachment #8367958 - Flags: review?(josiah) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/704cbe92153e
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch closebutton2.patch (obsolete) β€” β€” Splinter Review
Attachment #8369489 - Flags: review+
(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.
Keywords: checkin-needed
Attached patch closebutton2.patch β€” β€” Splinter Review
Attachment #8369489 - Attachment is obsolete: true
Attachment #8369494 - Flags: review+
Comment on attachment 8369494 [details] [diff] [review]
closebutton2.patch

rs=me for SeaMonkey
Attachment #8369494 - Flags: review+
https://hg.mozilla.org/comm-central/rev/5c89bc7e77e9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Duplicate of this bug: 975324
Duplicate of this bug: 1006056
Duplicate of this bug: 1007815
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.
Attachment #8369494 - Flags: approval-comm-release?
Flags: needinfo?(bugspam.Callek)
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. :-(
Attachment #8369494 - Flags: approval-comm-release?
Flags: needinfo?(bugspam.Callek)
Duplicate of this bug: 1028089
Duplicate of this bug: 1028821
Duplicate of this bug: 1044557
Duplicate of this bug: 1048060
You need to log in before you can comment on or make changes to this bug.