Last Comment Bug 742503 - in Junk Settings, disable addressbook white list if adaptive junk mail control is not activated
: in Junk Settings, disable addressbook white list if adaptive junk mail contro...
: ux-consistency, ux-error-prevention
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
-- enhancement (vote)
: Thunderbird 16.0
Assigned To: :aceman
Depends on: 397197
Blocks: 729147
  Show dependency treegraph
Reported: 2012-04-04 13:09 PDT by :aceman
Modified: 2012-07-10 14:16 PDT (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (4.41 KB, patch)
2012-04-26 14:00 PDT, :aceman
bwinton: feedback-
Details | Diff | Splinter Review
patch v2 (8.88 KB, patch)
2012-04-29 13:33 PDT, :aceman
bwinton: ui‑review-
Details | Diff | Splinter Review
patch v3 (10.29 KB, patch)
2012-06-13 13:06 PDT, :aceman
bwinton: feedback-
Details | Diff | Splinter Review
patch v4 (alternative) (7.93 KB, patch)
2012-06-13 14:32 PDT, :aceman
iann_bugzilla: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v5 (7.95 KB, patch)
2012-07-01 13:52 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v6 (7.95 KB, patch)
2012-07-10 06:53 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image :aceman 2012-04-04 13:09:28 PDT
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?
Comment 1 User image :aceman 2012-04-04 13:12:02 PDT
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:
Comment 2 User image :aceman 2012-04-05 00:02:09 PDT
See also comments in bug 397197.
Comment 3 User image :aceman 2012-04-25 08:15:24 PDT
The indentation was already done in bug 397197.
Comment 4 User image :aceman 2012-04-26 14:00:55 PDT
Created attachment 618796 [details] [diff] [review]

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?
Comment 5 User image :aceman 2012-04-26 14:06:00 PDT
Comment on attachment 618796 [details] [diff] [review]

Is that an universal problem with the list widget or is this some addressbook specific implementation?
Comment 6 User image :aceman 2012-04-27 03:15:12 PDT
I should also disable the list caption "Do not automatically mark..." .
Comment 7 User image Blake Winton (:bwinton) (:☕️) 2012-04-27 12:03:36 PDT
Comment on attachment 618796 [details] [diff] [review]

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.)
Comment 8 User image :aceman 2012-04-29 06:44:24 PDT
Who can help us here if it is a problem with the widget?
Comment 9 User image :aceman 2012-04-29 13:33:43 PDT
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?
Comment 10 User image :aceman 2012-04-29 13:40:33 PDT
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.
Comment 11 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-29 15:31:07 PDT
I don't know who maintains the core CSS. Neil Deakin maybe?
Comment 12 User image Blake Winton (:bwinton) (:☕️) 2012-05-01 07:28:10 PDT
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-.
Comment 13 User image :aceman 2012-05-12 11:19:47 PDT
Bwinton, can you attach a screenshot of what is not greyed out?
Comment 14 User image Blake Winton (:bwinton) (:☕️) 2012-06-13 11:41:12 PDT
I hope shows what I mean…
Comment 15 User image :aceman 2012-06-13 12:25:03 PDT
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).
Comment 16 User image :aceman 2012-06-13 13:06:03 PDT
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?
Comment 17 User image :aceman 2012-06-13 14:32:23 PDT
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?
Comment 18 User image Blake Winton (:bwinton) (:☕️) 2012-06-14 14:30:18 PDT
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!
Comment 19 User image :aceman 2012-06-15 06:22:54 PDT
Great to hear that :)
Comment 20 User image Blake Winton (:bwinton) (:☕️) 2012-06-21 11:49:22 PDT
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.  ;)

Comment 21 User image :aceman 2012-06-21 12:46:29 PDT
Yes, I have no problem going with v4 if it works for everyone.
Comment 22 User image Ian Neal 2012-07-01 13:40:59 PDT
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.
Comment 23 User image :aceman 2012-07-01 13:52:08 PDT
Created attachment 638223 [details] [diff] [review]
patch v5

Comment 24 User image Mike Conley (:mconley) 2012-07-06 10:20:09 PDT
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.
Comment 25 User image :aceman 2012-07-10 06:53:59 PDT
Created attachment 640582 [details] [diff] [review]
patch v6

Comment 26 User image Ryan VanderMeulen [:RyanVM] 2012-07-10 14:16:55 PDT

Note You need to log in before you can comment on or make changes to this bug.