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

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[:aceman]

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

[:aceman]

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

Comment 2

[:aceman]

See also comments in bug 397197.

Comment 3

[:aceman]

The indentation was already done in bug 397197.

Comment 4

[:aceman]

Attachment: 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?

Comment 5

[:aceman]

Comment on attachment 618796 [details] [diff] [review]
patch

Is that an universal problem with the list widget or is this some addressbook specific implementation?

Comment 6

[:aceman]

I should also disable the list caption "Do not automatically mark..." .

Comment 7

[Blake Winton (:bwinton) (:☕️)]

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.)

Comment 8

[:aceman]

Who can help us here if it is a problem with the widget?

Comment 9

[:aceman]

Attachment: 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

[:aceman]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know who maintains the core CSS. Neil Deakin maybe?

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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

[:aceman]

Bwinton, can you attach a screenshot of what is not greyed out?

Comment 14

[Blake Winton (:bwinton) (:☕️)]

I hope http://dl.dropbox.com/u/2301433/Screenshots/GreyJunk.png shows what I mean…

Comment 15

[:aceman]

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

[:aceman]

Attachment: 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

[:aceman]

Attachment: 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

[Blake Winton (:bwinton) (:☕️)]

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

[:aceman]

Great to hear that :)

Comment 20

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 21

[:aceman]

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

Comment 22

[Ian Neal]

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

[:aceman]

Attachment: patch v5

Thanks.

Comment 24

[Mike Conley (: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.

Comment 25

[:aceman]

Attachment: patch v6

Thanks.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0b2a814a303f