78274 - Convert profile manager to use <dialog>

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect, P4)

Description

[sairuh (rarely reading bugmail)]

like bug 78272, i thought a bug for this existed, but couldn't find one. dup as
needed...

anyhow, tabbing btwn the buttons in the Profile Manager and Manager User
Profiles dialogs shifts the visual focus [highlight], but it does not activate.
in other words, hitting enter [or escape] will not do anything when a button has
focus.

side note: if an item in the profile list has focus [selected and highlighted],
only the enter key will work [which will launch the profile]; hitting the escape
key will not dismiss the dialog [exit].

Comment 1

[Paul Chen]

nav triage team:

Reassigning to ccarlen, changing component to profile manager frontend

Comment 2

[Judson Valeski]

-> vishy

Comment 3

[Viswanath Ramachandran]

nav triage: my team is not going to be able to do this in mozilla0.9.2, does
someone want to take it? 

Comment 4

[Peter Trudelle]

Looks like we are not updating the initial default command button in response to
tabbing.  When focus is tabbed to another command button, bold outline still
stays on original default button, but should be updated. ->bryner, clearing target.

Comment 5

[Brian Ryner (not reading)]

This is because the profile manager has not yet been converted to use <dialog>.
 Updating summary and reassigning.

Comment 6

[Blake Ross]

Just another dialog, dup of bug 102637.

*** This bug has been marked as a duplicate of 102637 ***

Comment 7

[K'Trina Medina]

Verified as a dupe of bug 102637 

Comment 8

[[e-mail]]

Reopening because I have a patch.

Comment 9

[[e-mail]]

Attachment: Proposed patch

Comment 10

[piers]

Neil, several months since the patch was created - does the patch still work, 
and if so can anyone else sr? (Or am i missing something here?)

(Adding you as cc)

Comment 11

[sairuh (rarely reading bugmail)]

jag, has the patch bitrotted at all, or is it okay?

Comment 12

[timeless]

Comment on attachment 109235 [details] [diff] [review]
Proposed patch

it still applies

Comment 13

[Alec Flett]

Comment on attachment 109235 [details] [diff] [review]
Proposed patch

sr=alecf

Comment 14

[timeless]

.

Comment 15

[[e-mail]]

Fix checked in.

Comment 16

[John Morrison]

I see an error in the console on linux: 

  "chome://communicator/content/profileManager.js line 284: start has no 
   properties".

where start is 'start = document.getElementById('ok');'. Is this something
left behind?

Comment 17

[Conrad Carlen (not reading bugmail)]

This swapped the positions of the Start/Cancel buttons on Mac. Very disturbing -
the default button on Mac is always in the lower right, which it's no longer.

Comment 18

[[e-mail]]

Backed out because of issues

Comment 19

[[e-mail]]

Attachment: work in progress

Comment 20

[[e-mail]]

Attachment: The One

Comment 21

[[e-mail]]

Attachment: v1.1

Forgot some CSS :-[

Comment 22

[jag (Peter Annema)]

Nice big patch. I'm looking forward to reviewing this over a big mug of home
brewed coffee tomorrow.

Comment 23

[jag (Peter Annema)]

Comment on attachment 151485 [details] [diff] [review]
v1.1

>Index: profile/resources/locale/en-US/profileManager.properties

Perhaps we should put quotes around the name of the button in the intro and
switch text?

Maybe some other day...

>Index: themes/classic/global/mac/dialog.css
>Index: themes/classic/global/win/dialog.css
>Index: themes/modern/global/dialog.css

Why do the margins need to be adjusted? Not saying this is wrong, just trying
to understand.

>@@ -236,22 +237,6 @@
>-          // calling window.close() while an oncommand event
>-          // call is on the stack fails to close the window, 
>-          // so we need to do this ugly setTimeout hack
>-          window.setTimeout(
>-            function(aDlgType) {
>-              document.documentElement._reallyDoButtonCommand(aDlgType);
>-            },
>-            0, aDlgType);
>-        ]]>
>-        </body>
>-      </method>
>-      
>-      <method name="_reallyDoButtonCommand">
>-        <parameter name="aDlgType"/>
>-        <body>
>-        <![CDATA[

Is this part of the patch or some other bug? I know that hack was necessary at
some point. Did we fix whatever bug was causing that?

Looks good to me, r=jag

Comment 24

[[e-mail]]

(In reply to comment #23)
>>Index: themes/classic/global/mac/dialog.css
>>Index: themes/classic/global/win/dialog.css
>>Index: themes/modern/global/dialog.css
>Why do the margins need to be adjusted? Not saying this is wrong, just
>trying to understand.
We need to compensate for the default padding on <dialog> in order to be able to
show the <dialogheader class="header-large"> in the correct position.

>>-      <method name="_reallyDoButtonCommand">
><snip>
>Is this part of the patch or some other bug? I know that hack was necessary
>at some point. Did we fix whatever bug was causing that?
Apparently so. I tried both windows and linux, and got someone with a Mac to try
it on the mac, and it doesn't crash. I'm not to clear on the profile code but
switching profile doesn't work too well from a timeout. I tried debugging it but
it wouldn't break anywhere obvious. But I never did like that hack ;-)

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 151485 [details] [diff] [review]
v1.1

sr=sspitzer

Comment 26

[[e-mail]]

Fix checked in.

Comment 27

[Stefan Borggraefe]

Hmm, according to

  http://www.mozilla.org/projects/ui/accessibility/accesskey.html

Ok and Cancel buttons shouldn't have accesskeys, but this patch introduced the
attributes buttonaccesskeycancel and buttonaccesskeyaccept.

Comment 28

[[e-mail]]

(In reply to comment #27)
> Hmm, according to
> 
>   http://www.mozilla.org/projects/ui/accessibility/accesskey.html
> 
> Ok and Cancel buttons shouldn't have accesskeys, but this patch introduced the
> attributes buttonaccesskeycancel and buttonaccesskeyaccept.

Oh, that was only for compatibility with ff, nobody actually uses it ;-)