Closed Bug 926473 Opened 9 years ago Closed 9 years ago

Menu label "Disable scam detection for all messages" is ambiguous, too easy to hit after bug 562048 redesign

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: steve42lawson, Assigned: rsx11m.pub)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

Receive an email that Thunderbird suspects is a "scam".


Actual results:

The following message was displayed:

"This message may be a scam."

with the following link:

"Disable scam detection for all messages"


Expected results:

The verbiage of the link is ambiguous.  In "Disable scam detection for all messages", what is the scope of "all"?  All further messages from the email address in the "From:" field, or ALL messages -- i.e. turn scam detection off for EVERY subsequent email?

If the former, then a more informative message would be: "Disable scam detection for all further messages from this email address."

If the later, then how about: "Disable scam detection for all further emails (from any address)"  or even just "Disable scam detection" [in other words, remove the ambiguous clause "for all messages" -- because it is either redundant, or it's confusing]
Summary: "Disable scam detection for 'all messages' -- ambiguous message → "Disable scam detection for all messages" -- ambiguous message
That feature was introduced with bug 653103, where the actual discussion took place in bug 623198 and went through a couple of iterations before ending up with the current link text.

The "for all further emails" clause would be wrong as the feature wouldn't be triggered for messages you've already received (and even viewed) either. It's in fact "Disabling the feature" as I suggested as text somewhere in the originating bug report. The "for all messages" was supposed to be unambiguous as for "not just the current sender" or similar.
Blocks: 653103
Component: Untriaged → Message Reader UI
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: "Disable scam detection for all messages" -- ambiguous message → Rephrase "Disable scam detection for all messages" to make it unambiguous that the feature is switched off
Version: 24 → Trunk
Attached patch Proposed label change (obsolete) — Splinter Review
Ok, so "Disable scam detection feature entirely" is the least ambiguous phrase I could come up with which has the same length as the current label. If it sounds a bit scary, that's probably not a bad idea to make the user think twice about what it's doing.

Another option might be "Don't show this warning for any message" which is more like the checkbox in modal dialogs to silence them, but that's less specific with regard to actually disabling the entire feature (which it does).
Assignee: nobody → rsx11m.pub
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #818597 - Flags: ui-review?(bwinton)
Attachment #818597 - Flags: review?(bwinton)
Comment on attachment 818597 [details] [diff] [review]
Proposed label change

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

So, this is an improvement to the current message, so it gets my r+ and ui-r+, but would you be open to some brainstorming to see if we can get an even better message before we check it in?

Thanks,
Blake.
Attachment #818597 - Flags: ui-review?(bwinton)
Attachment #818597 - Flags: ui-review+
Attachment #818597 - Flags: review?(bwinton)
Attachment #818597 - Flags: review+
Thanks, but - what do you have in mind? I had this other proposal:

(Quoting comment #2)
> Another option might be "Don't show this warning for any message" which is
> more like the checkbox in modal dialogs to silence them, but that's less
> specific with regard to actually disabling the entire feature (which it does).

Thus, something even more specific than "Disable scam detection feature entirely" in the patch or something more casual like the other option mentioned above?
For the brainstorming bit, I think anything goes, and we'll see what sounds best.  :)

"Disable scam detection feature"
"Disable this warning"
"Permanently disable this warning"
"Don't show this warning again"
"Don't ever show this warning again"
…
(In reply to Blake Winton (:bwinton) from comment #5)
> "Disable this warning"
> "Don't show this warning again"

Those two don't resolve the ambiguity "is it just for this message or the entire feature?"

> "Permanently disable this warning"
> "Don't ever show this warning again"

If we go with either of those, it should have a "for all messages" at the end. The "ever" may be too absolute though, as you can re-enable it in the preferences ("disable" reflects that you can enable it again later if you wish).

I still like "Disable" along with "feature" best, that has the least uncertainty what it does.
This is the brainstorming bit, where we come up with ideas.  The filtering and choosing between ideas should happen after we have all the ideas we can think of.  ;)
"Disable scam detection"?  - I think "feature" is implied. And if not, shouldn't it be "Disable THE scam detection feature"
(In reply to Magnus Melin from comment #8)
> "Disable scam detection"?  - I think "feature" is implied.

Not really, that's what this bug report is about. The scope of what's exactly disabled for which messages has to be clearly conveyed, which I think isn't done if you drop the "feature" part (still could be misunderstood as disabling scam detection for *this* message or *this* sender, etc.).

> And if not, shouldn't it be "Disable THE scam detection feature"

Sounds more grammatically correct. Maybe let's make it "Disable the scam detection feature entirely" to be absolutely unambiguous that the whole thing is switched off?
Another idea would be to make the menu option read "Edit scam detection preferences..." which would open Options | Security | Email scams

Note that your original patch has bitrotted since the new notifications landed.
(In reply to Magnus Melin from comment #10)
> Another idea would be to make the menu option read "Edit scam detection
> preferences..." which would open Options | Security | Email scams

That may be a bit of an overkill given that there's just a single checkbox in that tab, but on the other hand it would be useful for the user to learn where to switch it on again if he or she wants to do so at a later time.

> Note that your original patch has bitrotted since the new notifications landed.

Thanks for pointing that out, I'll have a look if we decide to stick with just a label change and decided on the final text.
After updating my trunk build I see what you mean. Bug 562048 redesigned that bar and made the buttons part of a context menu underneath the "Preferences" button.

Thus, maybe indeed making that 2nd entry "Change global settings" and opening the dialog in the right tab would be a better solution? It takes more clicks, but would be more difficult to hit accidentally than now with the current design where "this message" and "everything" are just about on top of each other.
Depends on: 562048
Blocks: mail-scam, 562048
No longer depends on: 562048
Summary: Rephrase "Disable scam detection for all messages" to make it unambiguous that the feature is switched off → Menu label "Disable scam detection for all messages" is ambiguous, too easy to hit after bug 562048 redesign
Attached patch Open the options dialog instead (obsolete) — Splinter Review
This follows the suggestion in comment #10 and replaces the option to directly disable the feature from the menu with opening the options dialog in the correct tab, thus adding another level of error prevention and also allowing better discovery of how to re-enable the feature if so desired later.

 - mailWindowOverlay.js: Just replaces setting the pref directly with
   an openOptionsDialog() call for "paneSecurity" opening in "phishingTab"
   (I kept the previous comment as it still correctly describes the purpose)

 - mailWindowOverlay.xul: related changes in the menu

 - security.xul: "id" attributes were defined only for the "Junk" tab, thus
   I've added them for all tabs (also should make add-on authors happier)

 - messenger.dtd: label and accesskey changes for related entities

This behaves slightly different between Windows and Linux, caused by the different default for browser.preferences.instantApply; on Windows, the options dialog opens modal and thus the message (and the notification bar) is updated only after the dialog is closed, whereas on Linux the dialog isn't modal and hence the window refreshed immediately. Consequently, if you uncheck the box, the notification bar will disappear after you exit the options dialog on Windows whereas it sticks around on Linux until to reload the message or open another one.

This could be solved by expanding openOptionsDialog(aPaneID, aTabID, aOtherArgs) with another parameter "aForceModal" to add ",dialog=no,modal" or similar to the feature string, thus refreshing the message only after the dialog was closed. I'm not sure if the addition to the mailCore.js function just for this purpose and a single platform would overdo it.
Attachment #818597 - Attachment is obsolete: true
Attachment #833096 - Flags: ui-review?(bwinton)
Attachment #833096 - Flags: review?(mkmelin+mozilla)
This is the version discussed in comment #13 which switches the dialog to modal mode for this specific instance even when instantApply is active, thus improving the user experience on Linux. It shouldn't make any difference on Windows.
Attachment #833150 - Flags: ui-review?(bwinton)
Attachment #833150 - Flags: review?(mkmelin+mozilla)
Comment on attachment 833150 [details] [diff] [review]
Alternative patch with modal dialog

> /**
>  * Opens the Preferences (Options) dialog.
>  *
>  * @param aPaneID     ID of prefpane to select automatically.
>  * @param aTabID      ID of tab to select on the prefpane.
>  * @param aOtherArgs  other prefpane specific arguments
>  */
>-function openOptionsDialog(aPaneID, aTabID, aOtherArgs)
>+function openOptionsDialog(aPaneID, aTabID, aOtherArgs, aForceModal)

Eh, I forgot to add the new parameter to the documentation block, but you get the idea. Maybe there is a way to cover this in "aOtherArgs" somehow as well, thus to avoid that additional argument?
Comment on attachment 833150 [details] [diff] [review]
Alternative patch with modal dialog

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

Yes, I think opening preferneces is what we want.

::: mail/base/content/mailWindowOverlay.js
@@ +3111,1 @@
>    ReloadMessage();

I don't think doing the modal thing is very nice, it also doesn't help for the case where the dialog was already open.
Maybe best to do something like

onchange="if (this.instantApply) ReloadMessage();" on the preference?

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +574,5 @@
>  
>  <!-- Phishing Button Popup -->
>  <!ENTITY phishingOptionIgnore.label "Ignore warning for this message">
>  <!ENTITY phishingOptionIgnore.accesskey "n">
> +<!ENTITY phishingOptionSettings.label "Change global settings…">

"Scam detection settings..." perhaps?
Attachment #833150 - Flags: review?(mkmelin+mozilla)
Comment on attachment 833150 [details] [diff] [review]
Alternative patch with modal dialog

(In reply to Magnus Melin from comment #16)
> onchange="if (this.instantApply) ReloadMessage();" on the preference?

I don't think this will work given that the scope of the options dialog is not within the invoking message window, thus it won't have any knowledge of the ReloadMessage() function. Even more complicated, you may have multiple windows open with messages that triggered the scam warning and you'd like to reload.

Thus, a possible way how to solve this would be a pref listener on the message window, which appears to be too much overhead for a pref that's usually toggled just once and then forgotten about. Given that there are more relevant prefs which don't have an immediate visual effect on active views when toggled, I tend to just go with attachment 833096 [details] [diff] [review] rather than trying to make the ReloadMessage() call work for all cases.
Attachment #833150 - Flags: ui-review?(bwinton)
Attached patch Proposed patch (v3) (obsolete) — Splinter Review
(In reply to Magnus Melin from comment #16)
> ::: mail/base/content/mailWindowOverlay.js
> @@ +3111,1 @@
> >    ReloadMessage();

Given that the ReloadMessage() here only makes sense if the Options dialog was opened modal, I've made that call depend on browser.preferences.instantApply being false. Otherwise, it may look awkward on Linux if the message refreshes while the Preferences dialog opens.

> ::: mail/locales/en-US/chrome/messenger/messenger.dtd
> @@ +574,5 @@
> "Scam detection settings..." perhaps?

I'd like to keep the "Change" (or "Open", "Modify", but something), thus made it "Change scam detection settings..." which is about the length of the string for the "Ignore" menuitem above it.
Attachment #833096 - Attachment is obsolete: true
Attachment #833150 - Attachment is obsolete: true
Attachment #833096 - Flags: ui-review?(bwinton)
Attachment #833096 - Flags: review?(mkmelin+mozilla)
Attachment #833349 - Flags: ui-review?(bwinton)
Attachment #833349 - Flags: review?(mkmelin+mozilla)
For the common case window.opener.ReloadMessage() should work, I think.

Edit Scam Detection Preferences... or Edit Scam Detection Options... (depending on platform) would be in line with what the popup blocker in ff does.
(In reply to Magnus Melin from comment #19)
> For the common case window.opener.ReloadMessage() should work, I think.

Thanks, I figured it out after a bit of try and error. I still need to distinguish between instantApply being true or false in order to trigger reloading of the message at the right time. This works fine in the window where the options dialog was invoked from in both modes.

> Edit Scam Detection Preferences... or Edit Scam Detection Options...
> (depending on platform)

Done, but lower case for consistency. I've followed the template in other instances, thus defining both versions in the DTD file with an #ifdef in the XUL part. I've used 'd' as access key, 'E' is used for the "Edit" menu already.
Attachment #833349 - Attachment is obsolete: true
Attachment #833349 - Flags: ui-review?(bwinton)
Attachment #833349 - Flags: review?(mkmelin+mozilla)
Attachment #833387 - Flags: ui-review?(bwinton)
Attachment #833387 - Flags: review?(mkmelin+mozilla)
Comment on attachment 833387 [details] [diff] [review]
Proposed patch (v4)

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

LGTM! r=mkmelin
Attachment #833387 - Flags: review?(mkmelin+mozilla) → review+
Blake: ui-r ping! Working on something related and would like not to bitrot :)
Comment on attachment 833387 [details] [diff] [review]
Proposed patch (v4)

ui-r=me, based on inspection.

Thanks,
Blake.
Attachment #833387 - Flags: ui-review?(bwinton) → ui-review+
Thanks for the reviews, double-checked that the patch still applies to current trunk.
Push for comm-central, please.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/71a51089b521
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.