Last Comment Bug 872133 - Focus textbox for "seconds" when checking "Show an alert" in Notifications preference pane
: Focus textbox for "seconds" when checking "Show an alert" in Notifications pr...
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
-- minor (vote)
: seamonkey2.21
Assigned To: rsx11m
Depends on:
Blocks: 856454
  Show dependency treegraph
Reported: 2013-05-14 11:06 PDT by rsx11m
Modified: 2013-05-28 11:49 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Proposed patch [pushed to c-c, comment #4] (5.46 KB, patch)
2013-05-14 11:20 PDT, rsx11m
iann_bugzilla: review+
neil: ui‑review+
iann_bugzilla: approval‑comm‑aurora+
Details | Diff | Splinter Review
Patch for comm-aurora [comment #9] (4.35 KB, patch)
2013-05-28 10:48 PDT, rsx11m
no flags Details | Diff | Splinter Review

Description User image rsx11m 2013-05-14 11:06:43 PDT
This is another follow-up to bug 856454, for the design of the extended "Show an alert for [__] seconds" option. The current implementation (a) doesn't focus the textbox for "seconds" and (b) has a separate accesskey for that label.

Per bug 867210 comment #17 (and those leading up to it), no other preference pane shows this behavior; the preferred design implies that checking the box automatically focuses the textbox, thus the accesskey is shared between those to UI elements.
Comment 1 User image rsx11m 2013-05-14 11:20:35 PDT
Created attachment 749385 [details] [diff] [review]
Proposed patch [pushed to c-c, comment #4]

This patch ports the logic from bug 867210 to focus showAlertTime when EnableAlert() is invoked by a change mail.biff.show_alert towards a "true" value.

I'm also taking care of bug 869116 for this instance while I'm there, thus graying "seconds" in sync with the "Show an alert for" label.
Comment 2 User image rsx11m 2013-05-14 11:25:42 PDT
BTW: The patch would be suitable for comm-aurora without the changes to pref-notifications.dtd (removing showAlertTimeEnd.accesskey, which isn't necessary for the modifications in pref-notifications.{js,xul} to work). Thus, once approved, I'll post a branch patch without the locale/ changes so that there won't be a different interim behavior just for the 2.20 release.
Comment 3 User image rsx11m 2013-05-26 09:04:05 PDT
Thanks for the reviews, push for comm-central please.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2013-05-28 10:17:27 PDT
Comment 5 User image rsx11m 2013-05-28 10:23:37 PDT
Comment on attachment 749385 [details] [diff] [review]
Proposed patch [pushed to c-c, comment #4]

[Approval Request Comment]
Regression caused by (bug #): Follow-up fix to bug 856454
User impact if declined: SeaMonkey 2.20 would have a different behavior / UX
Testing completed (on m-c, etc.): tested and works fine with 2.20
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Comment 6 User image Ian Neal 2013-05-28 10:33:16 PDT
Comment on attachment 749385 [details] [diff] [review]
Proposed patch [pushed to c-c, comment #4]

There is one string change, but the patch still works without that change. Please generate a patch for checkin to comm-aurora without that string change.
Comment 7 User image rsx11m 2013-05-28 10:40:00 PDT
Oops, I forgot about my own comment #2, will do.
Comment 8 User image rsx11m 2013-05-28 10:48:20 PDT
Created attachment 754900 [details] [diff] [review]
Patch for comm-aurora [comment #9]

Without string modifications in pref-notifications.dtd.
See attachment 749385 [details] [diff] [review] for approvals.
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2013-05-28 11:33:22 PDT

Note You need to log in before you can comment on or make changes to this bug.