593795 - Need warning for moving cards via the message header if the cards exist in a list

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Mark Banner (:standard8)]

Follow-up to bug 456167.

We need a warning if cards exist in a mailing list and the user then attempts to move the card to a different address book.

Currently, that moving will cause the link to the mailing list to be lost without warning.

Comment 1

[Mark Banner (:standard8)]

The full fix for this would be to make mailing lists capable of referencing contacts in other address books, but that's a bit complicated for 3.3.

Therefore the option suggested by Blake and myself is this:

- If the card is contained within a mailing list (within that address book), then disable the address book switching option on the Edit Contact pop-up within the message header pane.

- Add a small bit of text below the address book switching option that states something along the lines of 'this card is contained within mailing lists and cannot be moved'.

Comment 2

[Mark Banner (:standard8)]

Attachment: Skeleton patch

This does the detection for whether or not a card is in a mail list, and disables the list box appropriately.

On Mac, at least, this will need some css so that the list looks disabled.

There's still a string to be added in as well.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Here's my first run at a solution.

I'm open to suggestions on the wording of the warning.  I can imagine some languages (German comes to mind) where that sentence could get pretty long.

I've also updated the CSS to make the e-mail address readable, because it was mostly invisible before.  See the attached screenshots to see what I mean.  I couldn't find a bug for that one, so I thought I'd take care of that here - hope that's OK.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Before patch - notice that the e-mail address is mostly invisible.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: After patch - the text is read-only, and more visible.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Also, did you want me to try to put together some tests for this?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: After patch - the warning on OSX

Is the warning enough?  Or should we find a way to style the drop-down to look more disabled?

Comment 8

[Ludovic Hirlimann [:Usul]]

(In reply to comment #6)
> Also, did you want me to try to put together some tests for this?

Yes please.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2 - now with tests

Alright, added a couple of Mozmill tests for this.

I brought over a few testing functions I had originally written for bug 422845.  I'll make that bug depend on this and re-base, since this patch needs the quickness.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3 - now with tests, and less cruft

Sorry for the bugspam.  Some unnecessary cruft in that last patch, which I've removed.

I'll give this a closer look tomorrow before I set the review flags.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

Comment 12

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

Comment on attachment 531109 [details] [diff] [review]
Patch v4

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

I feel like I want the dialog to be a little wider on OSX, so that the "You cannot change the address book because this contact is in one or more mailing lists." text doesn't look like it's poking out the sides.  Or maybe we just want to change the text to something a little shorter, like "You can't change the address book because the contact is in a mailing list."…

Next, the text in the address book should also probably be a disabled-grey if possible, or better might be to replace the dropdown with a label.

Finally, the Name/Email/Address Book labels should be right-aligned.

But, ui-r=me with those changes.

Thanks,
Blake.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5

Hm - I don't have a Mac to work on here at UDS, so while I can try to blindly implement those changes, I can't really test them. :/

I've updated the warning text to the shorter version that Blake suggested.

Also, a note on why I removed the editContactTextbox[readonly="true"] - notice how in the attachment 530092 [details], the readonly e-mail address is almost entirely invisible (ignore the part I blocked out manually - notice how "mike" and ".com" are almost completely unreadable.

So I removed that CSS so that it could be more readable - see attachment 530093 [details].

Comment 14

[Mark Banner (:standard8)]

I'm currently looking at the Mac stuff, I've also got a few other changes that I'm trying out as well (including even shorter text ;-) ), should have a patch up in an hour or so.

Comment 15

[Mark Banner (:standard8)]

Attachment: Possible reworking on Mac

Comment 16

[Mark Banner (:standard8)]

Attachment: With the longer text

Comment 17

[Mark Banner (:standard8)]

Attachment: Patch v6

I've done a few tweaks to the patch:

- The description for the warning is now displayed as part of the grid. Hence it only shows in the second column.
- The text is for the description is stored in the dtd and hence we don't need to be copying the string around all the time.
- Provided theming for Mac and Windows.
- Added end-alignment for the main labels.

I'm currently reviewing the test code and pushing this to try server.

Comment 18

[Mark Banner (:standard8)]

Attachment: Patch v7

Uses GrayText rather than grey.

Comment 19

[Mark Banner (:standard8)]

Attachment: Patch v8

Ok, this fixes an issue with the buttons getting shrunk on linux (we've added a vbox around the buttons which means they seem to work better when the new description gets wrapped).

It has my r+ for the bits Mike's done.

Comment 20

[Mark Banner (:standard8)]

Attachment: Windows screenshot

Comment 21

[Mark Banner (:standard8)]

Attachment: Linux screenshot

Comment 22

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

Comment on attachment 531378 [details] [diff] [review]
Patch v8

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

For the UI-Review, on Mac the alignment is all over the place, and I would like to see that cleaned up (but I would be okay with part of that happening in a followup bug).  Other than that, I'm pretty happy with the UI.

And nothing jumped out at me for the review either, so I'll call it r=me, too.

Thanks,
Blake.

Comment 23

[Mark Banner (:standard8)]

Landed with a minor change to the alignment on Mac.

Filed bug 656113 on fixing the alignments up completely later.

Checked in: http://hg.mozilla.org/comm-central/rev/10b8d5efa321