1689980 - Clicking the "continue" button in the import secret key dialog before selecting a file causes: Uncaught (in promise) TypeError: can't access property "length", gFiles is undefined

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Lasana Murray]

The continue button should be disabled until the user has actually selected a key.

Comment 1

[Lasana Murray]

Attachment: bug1689980.patch

Make the continue button disabled by default.

Comment 2

[Alessandro Castellani [:aleca]]

This happens only if using the menu item "Import secret keys from file" fro the OpenPGP Key Manager.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9200383 [details] [diff] [review]
bug1689980.patch

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

Thanks for finding this issue, but unfortunately this solution doesn't work as it breaks the "Add Key" subdialog.

I think the better approach would be to disable that button when the dialog loads directly on the import screen: https://searchfox.org/comm-central/rev/4eb14a03690b8d6bbe53853a33b6e09d330a05ea/mail/extensions/openpgp/content/ui/keyWizard.js#118-122

This is what we're using in the wizard when switching between sections.
`kDialog.getButton("accept").setAttribute("disabled", true);`

Comment 4

[Lasana Murray]

Attachment: bug1689980v2.patch

I disable the button closer to the code that displays this specific section.

Comment 5

[Lasana Murray]

Question: Is the plan to eventually remove the key manager from the main menu? Currently it's a little inconsistent, clicking back on this same "import secret key" screen takes you to the section used in the account settings rather than the key manager dialog.

Comment 6

[Alessandro Castellani [:aleca]]

(In reply to Lasana Murray from comment #5)

Question: Is the plan to eventually remove the key manager from the main menu? Currently it's a little inconsistent, clicking back on this same "import secret key" screen takes you to the section used in the account settings rather than the key manager dialog.

Indeed, we have bug 1662282 for it.
The plan is, most likely, to move this whole key manager into a tab.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9200650 [details] [diff] [review]
bug1689980v2.patch

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

The disabling of the button is correct, but I think we should move it in here: https://searchfox.org/comm-central/rev/f6018345454b019bd41c50da80c2bb34f042db86/mail/extensions/openpgp/content/ui/keyWizard.js#119

The wizardImportKey() method, normally has this stack trace when interacting from withing the dialog: wizardContinue() -> switchSection() -> wizardImportKey().
The "Continue" button is disabled whenever the wizardContinue() method is called, which is hooked to the Continue button itself.
Disabling the button in the wizardImportKey() is basically like disabling it twice if the user is using the dialog from the first screen.
Not an issue, but just unnecessary.

Disabling the button on init() only when the code is directly requesting the import section makes more sense.
We should also add a comment to explain why we're doing that.

Comment 8

[Lasana Murray]

Disabling the button on init() only when the code is directly requesting the
import section makes more sense.

Just a thought on this:
Wouldn't it be more idiomatic to have the accept button disabled by default and only enable it when the conditions for "continuing" are satisfied?

Comment 9

[Lasana Murray]

Attachment: bug1689980v3.patch

Move the change to the init function.

Comment 10

[Alessandro Castellani [:aleca]]

Wouldn't it be more idiomatic to have the accept button disabled by default and only enable it when the conditions for "continuing" are satisfied?

Sure, I'm okay with that.
Up to you if you want to take on the burden of updating the dialog, which is currently used by both the e2ee page as a subdialog and by the Key Manager as a regular dialog.
That's why there are some strange approaches and timeouts in it, in order to be able to use the same source in both scenarios.

Comment 11

[Alessandro Castellani [:aleca]]

Comment on attachment 9200756 [details] [diff] [review]
bug1689980v3.patch

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

Looks good, thanks.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/75a28605a427
Disable continue button by default when importing secret keys via the key manager. r=aleca

Comment 13

[Magnus Melin [:mkmelin]]

Maybe not worth uplifting.