Note: There are a few cases of duplicates in user autocompletion which are being worked on.

in Junk Settings, disable addressbook white list if adaptive junk mail control is not activated

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Account Manager
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 1 bug, {ux-consistency, ux-error-prevention})

Trunk
Thunderbird 16.0
ux-consistency, ux-error-prevention
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
In the Junk settings pane of Account manager, the "Do not mark mail as junk if the sender is in:" addressbook whitelist is always enabled.

If it is used only when the adaptive junk mail control is enabled, then it should be disabled otherwise.

Alternatively, it should only be enabled if any of "adaptive junk mail" or "trust headers set by" is enabled.

Can anybody test or confirm when this whitelist is consulted?
(Assignee)

Comment 1

5 years ago
Also, if the whitelist is dependent on the adaptive junk mail control, make it indented so the dependence is seen visually. As in the mockup in this attachment:
https://bugzilla.mozilla.org/attachment.cgi?id=612072
(Assignee)

Comment 2

5 years ago
See also comments in bug 397197.
Depends on: 397197
Summary: in Junk Settings, disable addressbok white list if adaptive junk mail control is not activated → in Junk Settings, disable addressbook white list if adaptive junk mail control is not activated
(Assignee)

Comment 3

5 years ago
The indentation was already done in bug 397197.
(Assignee)

Comment 4

5 years ago
Created attachment 618796 [details] [diff] [review]
patch

So this could do it.

However there is a glitch. Then the whilelist is disabled, the checkboxes in it do not appear disabled (just don't work). Is that some problem with the widget?
Attachment #618796 - Flags: feedback?(bwinton)
(Assignee)

Comment 5

5 years ago
Comment on attachment 618796 [details] [diff] [review]
patch

Is that an universal problem with the list widget or is this some addressbook specific implementation?
Attachment #618796 - Flags: feedback?(mconley)
(Assignee)

Comment 6

5 years ago
I should also disable the list caption "Do not automatically mark..." .
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 618796 [details] [diff] [review]
patch

Yeah, that's a glitch.  ;)

Other than that, I think I like it, but that kinda needs to be fixed.
(I took a quick look, and couldn't figure out why your disabling didn't work.)
Attachment #618796 - Flags: feedback?(bwinton) → feedback-
(Assignee)

Comment 8

5 years ago
Who can help us here if it is a problem with the widget?
(Assignee)

Comment 9

5 years ago
Created attachment 619441 [details] [diff] [review]
patch v2

So what about this? :)
It came out that checkbox is actually just an image (defined in core listbox.css and such). It seems it doesn't have css rule for the [disabled] state.
So I made a rule for our element (with opacity:0.5 which produces just the needed effect, at least on linux). But it should probably better be handled in the core css, can you contact the proper people?
Attachment #618796 - Attachment is obsolete: true
Attachment #618796 - Flags: feedback?(mconley)
Attachment #619441 - Flags: ui-review?(bwinton)
(Assignee)

Comment 10

5 years ago
Roc, bz, can you forward this to the correct people?
The problem is we have an element like this:
<listbox id="whiteListAbURI" rows="5"/>
   let abItem = document.createElement("listitem");
    abItem.setAttribute("type", "checkbox");
    abItem.setAttribute("class", "listitem-iconic");
    abItem.setAttribute("label", ab.dirName);
    abItem.setAttribute("value", ab.URI);

When we set disabled=true on it, most of the element turns gray, but the checkbox icon stays white (without change). I could override it in our theme (see patch v2), but there surely is a better way.
I don't know who maintains the core CSS. Neil Deakin maybe?
Comment on attachment 619441 [details] [diff] [review]
patch v2

That's better, but I think we also want to grey-out the text, and the border, like in the text box for "Automatically delete junk mail after [14] days".

And I kind of suspect that this is the wrong way to go, and we should figure out why setting the disabled attribute doesn't do the right thing here.

So, given those, I have to say ui-r-.
Attachment #619441 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 13

5 years ago
Bwinton, can you attach a screenshot of what is not greyed out?
I hope http://dl.dropbox.com/u/2301433/Screenshots/GreyJunk.png shows what I mean…
(Assignee)

Comment 15

5 years ago
Well, in your screenshot the whitelist background is white in the same way as the number in "Automatically delete junk mail after [14] days".
Also, the "trust junk mail headers set by" should not be greyed out, it is not dependent on Adaptive junk setting.

But I agree the whilelist entries' text should be grey (as it eh whitelist caption). I'll look into that. Maybe it is the same problem as the checkboxes not properly greying out (the whole widget disabling may be unimplemented in toolkit).
(Assignee)

Comment 16

5 years ago
Created attachment 632828 [details] [diff] [review]
patch v3

But for me on Linux, the whitelist is disabled fine, the text is grey and the background too. Just in case I attach an updated patch. Do you still see the problem? Can you try other platforms?
Attachment #619441 - Attachment is obsolete: true
Attachment #632828 - Flags: feedback?(bwinton)
(Assignee)

Updated

5 years ago
Attachment #632828 - Flags: feedback?(iann_bugzilla)
(Assignee)

Comment 17

5 years ago
Created attachment 632884 [details] [diff] [review]
patch v4 (alternative)

Alternative patch that does not just set .disabled=true on the listbox (that seems to behave differently in each theme) but also on individual list-rows (listitems?). That does work for me fine in gnomestripe without the need of any theme fixes (I had opacity 0.5 for the "checkbox" image in patch v3). So this patch has no theme changes. Does it work for you any better?
Attachment #632884 - Flags: ui-review?(bwinton)
Attachment #632884 - Flags: feedback?(iann_bugzilla)
Comment on attachment 632884 [details] [diff] [review]
patch v4 (alternative)

Works for me on OS X!
And on Windows 7!
And on Ubuntu!

Based on those, I like it.  ui-r=me!
Attachment #632884 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Updated

5 years ago
Attachment #632884 - Flags: feedback?(iann_bugzilla) → review?(iann_bugzilla)
(Assignee)

Comment 19

5 years ago
Great to hear that :)
Comment on attachment 632828 [details] [diff] [review]
patch v3

The text is still black for me with this patch.  Fortunately, I think V4 is a better way to go, so this f- is really only for consistency's sake.  ;)

Thanks,
Blake.
Attachment #632828 - Flags: feedback?(bwinton) → feedback-
(Assignee)

Comment 21

5 years ago
Yes, I have no problem going with v4 if it works for everyone.

Updated

5 years ago
Attachment #632828 - Flags: feedback?(iann_bugzilla)

Comment 22

5 years ago
Comment on attachment 632884 [details] [diff] [review]
patch v4 (alternative)

>+  // Enabled/disable individual listbox rows
Nit: full stop at end of sentence.
>+  // Setting enable/disable on the parent listbox does not seem to work
Nit: full stop at end of sentence.
r=me with that fixed.
Attachment #632884 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 638223 [details] [diff] [review]
patch v5

Thanks.
Attachment #632828 - Attachment is obsolete: true
Attachment #632884 - Attachment is obsolete: true
Attachment #638223 - Flags: review?(mconley)
Comment on attachment 638223 [details] [diff] [review]
patch v5

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

One super-tiny nit, otherwise, looks solid. Thanks aceman.

::: mailnews/base/prefs/content/am-junk.js
@@ +167,4 @@
>  }
>  
> +/**
> + * propagate changes to the server filter menu list back to

Super tiny nit - propagate should be capitalised.
Attachment #638223 - Flags: review?(mconley) → review+
(Assignee)

Comment 25

5 years ago
Created attachment 640582 [details] [diff] [review]
patch v6

Thanks.
Attachment #638223 - Attachment is obsolete: true
Attachment #640582 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0b2a814a303f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
You need to log in before you can comment on or make changes to this bug.