Closed Bug 1584459 Opened 7 months ago Closed 6 months ago

replace <textbox> in mailnews/base/prefs/ (am-copies.inc.xul, am-identity-edit.xul, am-main.xul, am-server-advanced.xul, am-server.xul, am-serverwithnoidentities.xul, am-smtp.xul)

Categories

(MailNews Core :: XUL Replacements, task, P1)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 4 obsolete files)

Replace <textbox> in mailnews/base/prefs/ (am-copies.inc.xul, am-identity-edit.xul, am-main.xul, am-server-advanced.xul, am-server.xul, am-serverwithnoidentities.xul, am-smtp.xul)

https://searchfox.org/comm-central/search?q=%3Ctextbox&case=false&regexp=false&path=%5Emailnews%2Fbase%2Fprefs%2F

AccountWizard and SmtpServerEdit.xul have their own bugs.

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Attached patch 1584459-part1.patch (obsolete) — Splinter Review

First part of this bug which includes:

  • am-copies.inc.xul
  • am-identity-edit.xul

This one is pretty big so I'm splitting it in smaller patches.

Attachment #9097071 - Flags: review?(mkmelin+mozilla)
Attached patch 1584459-textbox-html-input.patch (obsolete) — Splinter Review

I ended up doing it all in one patch.
Here's the try run, which I'm expecting to have some failures: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9835a432634f6f3600a6157a38eaeb3f5b1d560d

Attachment #9097071 - Attachment is obsolete: true
Attachment #9097071 - Flags: review?(mkmelin+mozilla)
Attachment #9097519 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9097519 [details] [diff] [review]
1584459-textbox-html-input.patch

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

Looks ok, except for that at least the Server Settings is now too wide, so the default value, and some buttons are partly pushed outside view -> scrollbars.

::: mailnews/base/prefs/content/am-identity-edit.xul
@@ +102,4 @@
>                </html:td>
> +              <html:td class="identity-table">
> +                <html:input id="identity.replyTo"
> +                            type="email"

I think we need to keep Reply to as type="text". It's really the mailbox (name and addr).

::: mailnews/base/prefs/content/am-main.xul
@@ +82,4 @@
>            </html:td>
> +          <html:td class="identity-table">
> +            <html:input id="identity.replyTo"
> +                        type="email"

(here too)
Attachment #9097519 - Flags: review?(mkmelin+mozilla)
Attachment #9097519 - Flags: review-
Attachment #9097519 - Flags: feedback+
Attached patch 1584459-textbox-html-input.patch (obsolete) — Splinter Review

This should be fixed now.

Attachment #9097519 - Attachment is obsolete: true
Attachment #9097730 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9097730 [details] [diff] [review]
1584459-textbox-html-input.patch

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

Looks good visually

::: mail/themes/shared/mail/input-fields.css
@@ +64,5 @@
> +}
> +
> +html|td.identity-table html|input {
> +  width: 100%;
> +}

looks like the wrong place to put a identity-table rule

also, wouldn't it make more sense to the identity-table rule on the <table>?

::: mailnews/base/prefs/content/am-identity-edit.xul
@@ +63,5 @@
>          <groupbox>
>            <label class="header">&publicData.label;</label>
>            <html:table>
>              <html:tr>
>                <html:td>

please use html:th for th row headers
Attachment #9097730 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch 1584459-textbox-html-input.patch (obsolete) — Splinter Review
Attachment #9097730 - Attachment is obsolete: true
Attachment #9098357 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9098357 [details] [diff] [review]
1584459-textbox-html-input.patch

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

r=mkmelin with some small adjustments

::: mail/themes/shared/mail/accountManage.css
@@ +96,5 @@
> +.identity-table td input {
> +  width: 100%;
> +}
> +
> +

an extra blank line snuck in here

::: mailnews/base/prefs/content/am-identity-edit.xul
@@ +79,4 @@
>                </html:td>
>              </html:tr>
>              <html:tr>
> +              <html:th align="left">

Please align using css instead (f.identity-table > th { text-align: left; } I guess
Here and the other places

::: mailnews/base/prefs/content/am-server-advanced.xul
@@ +11,5 @@
>  
>  <!DOCTYPE dialog SYSTEM "chrome://messenger/locale/am-server-advanced.dtd">
>  
>  <dialog
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

if you're going to touch this, put the namespace on same row as dialog
Attachment #9098357 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9098357 - Attachment is obsolete: true
Attachment #9098371 - Flags: review+
Keywords: checkin-needed
Blocks: 1584462

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3d6b66cfd6a5
replace <textbox> in am-copies*.xul, am-identity-edit.xul, am-main.xul, am-server*.xul, am-smtp.xul. r=mkmelin

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