1563122 - Use HTML input instead of XUL textbox in mail/components/im/content/

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Alessandro Castellani [:aleca]]

• mail/components/im/content/addbuddy.xul
• mail/components/im/content/am-im.xul
• mail/components/im/content/chat-conversation-info.js
• mail/components/im/content/chat-messenger.inc.xul
• mail/components/im/content/imAccounts.xul
• mail/components/im/content/imAccountWizard.xul

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1563122-textbox-html-input.patch

The entire Chat tab and related dialogs should have all been de-textbox'ed.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9092232 [details] [diff] [review]
1563122-textbox-html-input.patch

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

::: chat/content/imAccountOptionsHelper.js
@@ +22,2 @@
>      if (aType == "number") {
> +      input.classList.add("number-inline");

maybe it would make sense to instead always have input-inline, but match it on 
input[type="number"].input-inline ?

::: mail/components/im/content/chat-conversation-info.js
@@ +75,5 @@
>              </description>
>              <image class="prplIcon"></image>
>            </hbox>
>            <description class="statusMessage" mousethrough="never" crop="end" flex="100000"/>
> +          <html:input class="statusMessageInput input-inline" mousethrough="never" crop="end" hidden="hidden"/>

not sure if mousethrough is needed or works for html:input, but crop at least doesn't do anything

@@ +128,5 @@
>  
>        let topic = this.topic;
>        let topicInput = this.topicInput;
> +      topic.removeAttribute("hidden");
> +      topicInput.hidden = true;

please be consistent. I'd  topicInput.removeAttribute("hidden");

@@ +175,5 @@
>        }
>  
>        this.setAttribute("editing", "true");
> +      topicInput.removeAttribute("hidden");
> +      topic.hidden = true;

for consistency, topic.toggleAttribute("hidden", true);

::: mail/components/im/content/chat-messenger.inc.xul
@@ +178,2 @@
>                        <label class="conv-nicklist-header-label"
> +                             id="participantLabel"

move id first

@@ +181,2 @@
>                               value="&chat.participants;"/>
> +                      <html:input readonly="true" class="plain" id="participantCount"/>

readonly="readonly", and please put id first

::: mail/components/im/content/imAccountWizard.js
@@ +102,5 @@
>      return this.userNameBoxes.reduce((prev, elt) => prev + elt.value, "");
>    },
>  
>    checkUsername() {
> +    console.log("checking");

remove

@@ +150,2 @@
>      }
> +    input.addEventListener("input", accountWizard.checkUsername);

would be nicer to change this to

input.addEventListener("input", event => {
   this.checkUsername();
});

::: mail/components/im/content/imAccountWizard.xul
@@ +80,5 @@
> +        <label id="aliasLabel"
> +               value="&accountAliasField.label;"
> +               class="label-inline"
> +               control="alias" />
> +        <html:input id="alias" class="input-inline"/>

add the type="text" maybe?

::: mail/components/im/content/imAccounts.xul
@@ +132,5 @@
> +        </vbox>
> +        <html:input id="displayNameInput" hidden="hidden" class="input-inline"/>
> +        <label id="statusMessage" crop="end" value=""
> +               onclick="statusSelector.statusMessageClick();"/>
> +        <html:input id="statusMessageInput" crop="end" value=""

remove the crop, that doesn't do anything for an html:input

::: mail/components/im/content/imStatusSelector.js
@@ +91,5 @@
>  
>    statusMessageClick() {
>      let statusMessage = document.getElementById("statusMessage");
>      let statusMessageInput = document.getElementById("statusMessageInput");
> +    statusMessage.hidden = true;

statusMessage.toggleAttribute("hidden", true)

@@ +148,5 @@
>        true
>      );
>    },
>  
>    statusMessageBlur(aEvent) {

looks like you can get rid of this method now.
Doesn't seem there is ever any other target than the input itself. 

So, you should be able to do this at the callers instaed:

statusMessageInput.addEventListener("blur", (event) => 
  this.finishEditStatusMessage(true);
});

@@ +185,5 @@
>      delete this._stopEditStatusTimeout;
>      let statusMessage = document.getElementById("statusMessage");
>      let statusMessageInput = document.getElementById("statusMessageInput");
> +    statusMessage.removeAttribute("hidden");
> +    statusMessageInput.hidden = true;

toggleAttribut("hidden", true);

@@ +256,5 @@
>    displayNameClick() {
>      let displayName = document.getElementById("displayName");
>      let displayNameInput = document.getElementById("displayNameInput");
> +    displayName.hidden = true;
> +    displayNameInput.removeAttribute("hidden");

here too, consistency

@@ +288,5 @@
>        true
>      );
>    },
>  
>    displayNameBlur(aEvent) {

this too can go I think

Comment 3

[Alessandro Castellani [:aleca]]

maybe it would make sense to instead always have input-inline, but match it on
input[type="number"].input-inline ?

I think it's better to keep those separate and not use the same class.
The .input-inline applies the necessary padding and margin for a text field that is not styled automatically.
The number field already comes with a default style, so we don't need those padding and margin declarations, and the .number-inline class is only necessary for those edge cases where it's required for the number field to grow as wide as its parent container.

toggleAttribute("hidden", true);

This method works for input fields but doesn't for XUL label elements as the default style expects a [hidden="true"] match, while this method only applies a hidden="" attribute.

I could use setAttribute("hidden", true); instead just for the labels, which it works.
What do you think?

Comment 4

[Magnus Melin [:mkmelin]]

Yeah that works for me. Perhaps name it input-number-inline though

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1563122-textbox-html-input.patch

Awesome, thanks.
Updated and ready for a review.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9092447 [details] [diff] [review]
1563122-textbox-html-input.patch

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

Looks good, just some nits. r=mkmelin

::: mail/components/im/content/chat-conversation-info.js
@@ +175,5 @@
>        }
>  
>        this.setAttribute("editing", "true");
> +      topicInput.removeAttribute("hidden");
> +      topic.setAttribute("hidden", true);

topic is the statusMessage desciption, still xul, so should not be changed I think. 
Or we can use hidden if it works, but a nit: "true" since attr values are always strings. Will get converted anyway so not a big deal.

::: mail/components/im/content/chat-messenger.inc.xul
@@ +47,5 @@
> +                <vbox flex="1"
> +                      orient="horizontal"
> +                      align="center"
> +                      class="input-container status-container">
> +                  <label id="statusMessage"

Can we change this id while we're here. Seems like a bad idea to have one statusMessage class and then an #statusMessage but for different things :/ 
statusMessageLabel perhaps?

@@ +181,2 @@
>                               value="&chat.participants;"/>
> +                      <html:input readonly="readonly" class="plain" id="participantCount"/>

move id first

::: mail/components/im/content/imStatusSelector.js
@@ +91,5 @@
>  
>    statusMessageClick() {
>      let statusMessage = document.getElementById("statusMessage");
>      let statusMessageInput = document.getElementById("statusMessageInput");
> +    statusMessage.setAttribute("hidden", true);

"true"

@@ +251,5 @@
>  
>    displayNameClick() {
>      let displayName = document.getElementById("displayName");
>      let displayNameInput = document.getElementById("displayNameInput");
> +    displayName.setAttribute("hidden", true);

"true"

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1563122-textbox-html-input.patch

Patch updated.
Try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7823c4d4f9c2a747a5523532b0754ac8061e8e99

Comment 8

[Alessandro Castellani [:aleca]]

a couple Z3 and X4 failures which I can't recreate locally.
Are those known?

Comment 9

[Jorg K (CEST = GMT+2)]

X4 gives a suggestion, bug 1508520. I think I've seen test-message-header.js::test_clicking_star_opens_inline_contact_editor before. Not very frequent, no bug yet. Don't worry about them. Seen here before:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=fc2d525229e607e2ae04152cde5bd41ad6c31c9d&selectedJob=266010949

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc79ff4e0560
Use HTML input instead of XUL textbox in mail/components/im/content/. r=mkmelin