1607526 - Don't show the To: field if the Newsgroup or Follow-up fields are in use

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Alessandro Castellani [:aleca]]

As per Magnus comment on bug 1601748#c7

if you have a newsgroup selected it should not have To showing at all. Newsgroup should show, and maybe Follow-up too, since that's kind of standard for news.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1607526-newsgroups-to-field.patch

I re-enabled the original check that was happening before bug 440377.
The To field is hidden only if the Newsgroups field is visible.
I also implemented an extra check to hide/show the To field if the user enables/disables the Newsgroups field.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9119582 [details] [diff] [review]
1607526-newsgroups-to-field.patch

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

Sorry but wow I can't select Newsgroup AND To, which should still be possible.

Comment 3

[Alessandro Castellani [:aleca]]

Ah, my bad, I misunderstood the scope of the request.

So, just to be sure I got it, the section should behave in this way:

• If Newsgroup is visible, To shouldn't be visible, but a label to activate it should still be available and clickable.

On it.

Comment 4

[Magnus Melin [:mkmelin]]

Correct.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1607526-newsgroups-to-field.patch

This one should be correct.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9119965 [details] [diff] [review]
1607526-newsgroups-to-field.patch

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

Works. For newsgroups it would make sense for the Followup (only) to be the one showing in additional fields, and the mail ones hidden. 
newsgroups + mail cross usage only partly works: you need to use a nntp identity and have an smtp set up for that. The normal case for nntp really is Newsgroup and Followup.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +160,5 @@
>        input.focus();
>        input.value = msgNewsgroups;
>        recipientAddPill(input, true);
> +
> +      // Collapse the 'To' field

But you're doing the oposite

Comment 7

[Alessandro Castellani [:aleca]]

For newsgroups it would make sense for the Followup (only) to be the one showing in additional fields, and the mail ones hidden.

So, you mean showing "Followup" to the right of the identity field, and all the other fields move them inside the popup panel?
If that's the case, do you want me to do it in this bug?

// Collapse the 'To' field
But you're doing the opposite

I'm actually collapsing the "To" field and revealing the "To" label.
I guess I should update the comment to make it more clear.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) (PTO to 17th Jan 2020, sporadically reading bugmail) from comment #7)

So, you mean showing "Followup" to the right of the identity field, and all the other fields move them inside the popup panel?
If that's the case, do you want me to do it in this bug?

Correct. Up to you where you want to do it.

Comment 9

[Alessandro Castellani [:aleca]]

Sounds good, I think it makes sense and it shouldn't be too complicated.
Updating the patch right now.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1607526-newsgroups-to-field.patch

This should cover all the expected scenarios when dealing with a Newsgroup.

• All the non-nntp labels are moved into the popup panel.
• All the nntp labels are moved outside the popup panel.
• The follow up label is visible.
• The closing label for the Newsgroup row is removed to prevent having a recipients area with no recipient row.

Let me know if you see any issue or something I missed.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9120851 [details] [diff] [review]
1607526-newsgroups-to-field.patch

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

Changing From to a newsgroup identity doesn't play out: that needs to do some toggling, so that newgroup becomes visible (and vice versa I suppose). 

You can get fields showing in the "overflow area" that are already visible

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +158,5 @@
> +      // Show the 'To' label.
> +      document.getElementById("addr_to").removeAttribute("collapsed");
> +
> +      // Move all extra labels inside the popup panel.
> +      let extra_recipients = document.getElementById("addressingWidgetLabels");

no snake_variable names please

@@ +182,5 @@
>        recipientAddPill(input, true);
> +
> +      // Show the 'Follow-up' label.
> +      document.getElementById("addr_followup").removeAttribute("collapsed");
> +      // Remove the close label from the newsgroup row.

this doesn't play well wrt changing from identity from a newsgroup one to a mail one

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1607526-newsgroups-to-field.patch

I updated the patch trying to cover everything.

Now the UI of the recipients area updates accordingly if the currently selected account is NNTP or not.
Also, I fixed a small focus bug when replying to a newsgroups.

I noticed that the message editor toolbar is hidden if the selected account is a newsgroup.
I guess we should re-enable it if the user switches account, but I wasn't able to figure out how to do it.
Any advice?

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #12)

I noticed that the message editor toolbar is hidden if the selected account is a newsgroup.

I think you mean the plain text compose comes up - yes, that's the default for news. It can't be changed on the fly.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9121957 [details] [diff] [review]
1607526-newsgroups-to-field.patch

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

When i start with newsgroup account, and then change to a mail account identity (From), the Newsgroup, and Follow-up show up in the overflow, even if they are already there. 
It's also not possible to remove the newgroup row if I started up by being in a newsgroup account.

In general this seems like it's relying to much on certain ids. It would probably be easier to code up to work generically if you add classes for what's what, and show/hide etc based on if it's a news or mail item.

Comment 15

[Alessandro Castellani [:aleca]]

All right, I will rework this to not use IDs but rather have classes to programmatically hide and show based on the account type.

Quick question to understand the workflow.

Start a new compose window with Newsgroup account

• The Newsgroups field is visible.
• The To field is hidden.
• The Follow Up label is visible on the right side of the identity menulist.
• Every other label is inside the popup panel.

Correct?

The user changes the identity from a news to an imap account?

• Should we hide the Newsgroups field or leave it open?
• Should we automatically show again the To field?
• Should we reorder the labels accordingly?

The user changes the identity from an imap to an news account?

• Should we hide the To field or leave it open?
• Should we automatically show again the Newsgroups field?
• Should we reorder the labels accordingly?

How do we handle pills already created? Do we leave them in?

Proposed approach

• Only hide/show the To field when changing identity.
• Any other opened field should retain its status.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1607526-newsgroups-to-field.patch

This should take care of all the scenarios listed above.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9122517 [details] [diff] [review]
1607526-newsgroups-to-field.patch

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

I get a bunch of
JavaScript error: chrome://messenger/content/messengercompose/addressingWidgetOverlay.js, line 248: TypeError: can't access property "closest", document.querySelector(...) is null

.. and newsgroup isn't shown when starting compose with a newsgroup selected

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6242,5 @@
>  /**
>   * Hides addressing options (To, CC, Bcc, Newsgroup, Followup-To, etc.)
>   * that are not relevant for the account type used for sending.
>   *
> + * @param {*} accountKey - Key of the account that is currently selected

what's {*}? 
Should be {string}, no?

Comment 18

[Alessandro Castellani [:aleca]]

I get a bunch of
JavaScript error: chrome://messenger/content/messengercompose/addressingWidgetOverlay.js, line 248: TypeError: can't access property "closest", document.querySelector(...) is null
.. and newsgroup isn't shown when starting compose with a newsgroup selected

Wait, what?
document.querySelector(...) is null at 248 means that querySelector(".mail-primary-input") is failing, which is weird because that class is statically written in the xhtml file on the To field.

I can't reproduce this error, I don't know what's up with this.
Pinging Richard to see if he experiences the same issue with this patch.

Comment 19

[Alessandro Castellani [:aleca]]

Here's also a try-run I pushed yesterday, with everything green except for telemetry: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=50754c7572b46b2c224b9558e3e2f0a6a11988ce

Comment 20

[Richard Marti (:Paenglab)]

Works for me too.

Comment 21

[Magnus Melin [:mkmelin]]

Hmm, let me retry it.

Comment 22

[Magnus Melin [:mkmelin]]

Maybe I had forgot to rebuild for the xhtml changes. It seems to work. But, starting with a newsgroup the To filed is shown.

Comment 23

[Richard Marti (:Paenglab)]

Starting TB with a newsgroup selected and then clicking on "Write" shows the newsgroup field and the To field is hidden.

Comment 24

[Alessandro Castellani [:aleca]]

Starting TB with a newsgroup selected and then clicking on "Write" shows the newsgroup field and the To field is hidden.

Same thing for me.
The To field is shown in a newsgroup only if I hit Reply and not Follow Up.
Attaching a screencap for posterity.

Comment 25

[Alessandro Castellani [:aleca]]

Attachment: news-email.webm

Comment 26

[Magnus Melin [:mkmelin]]

Very strange. To still keeps showing for me.

Comment 27

[Alessandro Castellani [:aleca]]

I'm gonna write a test to assert that the To field is hidden when composing an email from the newsgroup.

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: 1607526-newsgroups-to-field.patch

I updated the leftover comment string and implemented a quick check in test to be sure the To addressing row is hidden when opening a compose window from a news account.
The test passes locally, and I lunched a new try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6394f77b235070d2ce65000aba1f46adee611560

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9122752 [details] [diff] [review]
1607526-newsgroups-to-field.patch

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

Somehow it now works for me too. Good with some tests, we should add more of those.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/04d95196247e
Hide To: field if the Newsgroups filed is in use. r=mkmelin

Comment 31

[Jorg K (CEST = GMT+2)]

I'd be nice if commit messages could be grammatically correct. Please use hg out before pushing and read what you're going to push. There is hg commit --amend so edit the commit message of the topmost change set. There is also hg histedit (which I've never used). Using hg out also prevents pushing undesired changes. Remember that HG is forever, mistakes can't be corrected ever. For your reference, this little script has come in handy for me:

hg out

while true; do
    read -p "Go ahead and push this? (y/n)" yn
    case $yn in
        [Yy]* ) break;;
        [Nn]* ) echo -e "\nPush aborted :-("; exit;;
        * ) echo "Please answer yes or no.";;
    esac
done

echo -e "\nPushing... you've got 10 seconds to change your mind ;-)"
sleep 10

echo -e "\nhg push ssh://[e-mail]@hg.mozilla.org/comm-central/"
hg push ssh://[e-mail]@hg.mozilla.org/comm-central/

Comment 32

[Jorg K (CEST = GMT+2)]

And a tip for the reviewer: Check the commit message!

Comment 33

[Alessandro Castellani [:aleca]]

Thank you so much for this heads up.
It was my first live push, and I thought the commit message was clear, my bad.
I'll use your little script as a safety net.

Comment 34

[Rob Lemley [:rjl]]

Thunderbird 73.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/f226f333e4e92661f3723f8325e7ace2566b0b7e