Closed Bug 742503 Opened 9 years ago Closed 8 years ago

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

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 16.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: ux-consistency, ux-error-prevention)

Attachments

(1 file, 5 obsolete files)

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?
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
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
The indentation was already done in bug 397197.
Attached patch patch (obsolete) — Splinter 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?
Attachment #618796 - Flags: feedback?(bwinton)
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)
I should also disable the list caption "Do not automatically mark..." .
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-
Who can help us here if it is a problem with the widget?
Attached patch patch v2 (obsolete) — Splinter Review
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)
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-
Bwinton, can you attach a screenshot of what is not greyed out?
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).
Attached patch patch v3 (obsolete) — Splinter Review
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)
Attachment #632828 - Flags: feedback?(iann_bugzilla)
Attached patch patch v4 (alternative) (obsolete) — Splinter Review
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+
Attachment #632884 - Flags: feedback?(iann_bugzilla) → review?(iann_bugzilla)
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-
Yes, I have no problem going with v4 if it works for everyone.
Attachment #632828 - Flags: feedback?(iann_bugzilla)
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+
Attached patch patch v5 (obsolete) — Splinter Review
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+
Attached patch patch v6Splinter Review
Thanks.
Attachment #638223 - Attachment is obsolete: true
Attachment #640582 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0b2a814a303f
Status: ASSIGNED → RESOLVED
Closed: 8 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.