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...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.21
Assigned To: rsx11m
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed
fixed


Attachments
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 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 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 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 rsx11m 2013-05-26 09:04:05 PDT
Thanks for the reviews, push for comm-central please.
Comment 4 Ryan VanderMeulen [:RyanVM] 2013-05-28 10:17:27 PDT
https://hg.mozilla.org/comm-central/rev/1c04fc3a99c6
Comment 5 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 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 rsx11m 2013-05-28 10:40:00 PDT
Oops, I forgot about my own comment #2, will do.
Comment 8 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 Ryan VanderMeulen [:RyanVM] 2013-05-28 11:33:22 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/485de398bd2f

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