certificate import success message looks like an error (warning icon by alert() function)

RESOLVED FIXED in Firefox 55

Status

()

Core
Security: PSM
P1
trivial
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: georgezhu, Assigned: keeler)

Tracking

({ux-consistency})

Trunk
mozilla55
ux-consistency
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8707491 [details]
success message

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151210085006

Steps to reproduce:

1) Go to preferences->advanced->certificates->view certificates->your certificates
2) Press Import...
3) Select a valid SSL client certificate


Actual results:

Importing succeeds, but for a moment it looks like it fails. Because the success message contains a big exclamation mark that's usually only seen in error or warning messages(see attached image). I've been importing some client side certificates for the browser and for Thunderbird(same code) on 2 computers, and on the fourth import I still got a brief scare on seeing the success message.


Expected results:

The message should not look like an error. Probably the easiest would be to remove the exclamation mark, or replace it with something more fitting.

Comment 1

2 years ago
https://dxr.mozilla.org/mozilla-central/search?q=SuccessfulP12Restore
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Component: Untriaged → Security: UI
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Whiteboard: [good first bug]
Hello, I would like to take this up as my first bug. Can someone guide me how to proceed please. I would like to know the repo it is related to. Any pointers would be appreciated. Thanks!

Comment 3

2 years ago
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Hello,

I've cloned the gecko-dev repo and then used mach build and mach run to have the nightly version. Wanted to know if I'm on the right track. I've also analyzed the two files here: https://dxr.mozilla.org/mozilla-central/search?q=SuccessfulP12Restore but not able to understand how that exclamation mark is coming. 

We're considering everything as errors here and then using switch case to know the exact type (SuccessfulP12Restore, in this case), but I can't find where we're setting up that exclamation mark.

I would need some more help. Also I don't see any mentor on this bug.

Comment 5

2 years ago
You are basically correct, but submit a patch must be used Mercurial (https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial), either a workflow (https://developer.mozilla.org/en-US/docs/Mozilla/Git).


Good First Bugs and Mentored bugs is two different categories in my opinion, so there is no formal mentor here. per https://developer.mozilla.org/en-US/docs/Introduction#Find_a_bug_we%27ve_identified_as_a_good_fit_for_new_contributors.


ShowAlertFromStringBundle > ShowAlertWithConstructedString > prompter->Alert > 
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPromptService#alert_example

Ah, I don't know how to find and specify a moderate icon for it, sorry.
Component: Security: UI → Theme
Product: Core → Firefox
Summary: certificate import success message looks like an error → certificate import success message looks like an error (warning icon by alert() function)
Whiteboard: [good first bug]

Comment 6

2 years ago
http://stackoverflow.com/questions/5806465/javascript-change-alert-icon
http://stackoverflow.com/questions/6853936/javascript-remove-warning-symbol-in-popup
http://forums.asp.net/t/961205.aspx
......
Keywords: ux-consistency
Whiteboard: [DUPEME][wontfix?]
Hello, thanks for the info you provided for submitting patch through mercurial as well as git. 

So, this means essentially we've to integrate an external JS library or this bug won't fix it at all?

Comment 8

2 years ago
I guess this bug are difficult to fix, unless we have a base changes, or a new interface. However, I don't have experience in the side.
(Assignee)

Updated

9 months ago
Assignee: nobody → dkeeler
Component: Theme → Security: PSM
Priority: -- → P1
Product: Firefox → Core
Whiteboard: [DUPEME][wontfix?] → [psm-assigned]
Version: 43 Branch → Trunk
Comment hidden (mozreview-request)

Comment 10

9 months ago
mozreview-review
Comment on attachment 8843491 [details]
bug 1239344 - remove error alert for successful PKCS12 operations

https://reviewboard.mozilla.org/r/117214/#review118912

Looks good.

We can probably also take the opportunity to update test_certDB_import_pkcs12.js so that it tests alerts are only shown for failure cases, but I can take care of that if you prefer.

::: security/manager/ssl/nsPKCS12Blob.cpp:569
(Diff revision 1)
> +  nsAutoString message;
> +  if (NS_FAILED(GetPIPNSSBundleString(msgID, message))) {
> +    return;
> +  }
> +
> +  prompter->Alert(nullptr, message.get());

We should explicitly ignore the result using `Unused <<`.
Attachment #8843491 - Flags: review?(cykesiopka.bmo) → review+

Updated

9 months ago
Duplicate of this bug: 1067260
Comment hidden (mozreview-request)
(Assignee)

Comment 13

9 months ago
mozreview-review-reply
Comment on attachment 8843491 [details]
bug 1239344 - remove error alert for successful PKCS12 operations

https://reviewboard.mozilla.org/r/117214/#review118912

Thanks! I went ahead and added a few testcases - what do you think?

> We should explicitly ignore the result using `Unused <<`.

Good call.
(Assignee)

Comment 14

9 months ago
(ni? so you see comment 13)
Flags: needinfo?(cykesiopka.bmo)

Comment 15

9 months ago
Cool, looks good.
Flags: needinfo?(cykesiopka.bmo)
(Assignee)

Comment 16

9 months ago
Thanks! Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22cc2bab4f76

Comment 17

9 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58ae9ceae825
remove error alert for successful PKCS12 operations r=Cykesiopka

Comment 18

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58ae9ceae825
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.