Closed Bug 1563122 Opened 8 months ago Closed 6 months ago

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

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: aleca, Assigned: aleca)

References

Details

Attachments

(1 file, 2 obsolete files)

  • 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
Assignee: nobody → alessandro
Mentor: alessandro
Status: NEW → ASSIGNED
Attached patch 1563122-textbox-html-input.patch (obsolete) — Splinter Review

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

Attachment #9092232 - Flags: review?(mkmelin+mozilla)
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
Attachment #9092232 - Flags: review?(mkmelin+mozilla) → feedback+

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?

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1563122-textbox-html-input.patch (obsolete) — Splinter Review

Awesome, thanks.
Updated and ready for a review.

Attachment #9092232 - Attachment is obsolete: true
Attachment #9092447 - Flags: review?(mkmelin+mozilla)
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"
Attachment #9092447 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

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

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

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

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Regressions: 1591148
You need to log in before you can comment on or make changes to this bug.