Closed Bug 1563118 Opened 1 year ago Closed 5 months ago

Use HTML input instead of XUL textbox in abCard.inc.xul

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, 7 obsolete files)

Assignee: nobody → alessandro
Mentor: alessandro
Status: NEW → ASSIGNED
Attached patch 1563118-textbox-html-input.patch (obsolete) — Splinter Review

New contact dialog ready for review.
The Photo tab doesn't really work, but it's not related to the input conversion.
I reported it on bug 1581253.

Attachment #9092815 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9092815 [details] [diff] [review]
1563118-textbox-html-input.patch

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

Work | Department is hosed.

::: mail/components/addrbook/content/abCard.inc.xul
@@ +147,5 @@
>                <spacer flex="1"/>
>                <label control="WorkPhone" value="&WorkPhone.label;"
>                       accesskey="&WorkPhone.accesskey;" />
> +              <html:input id="WorkPhone"
> +                          type="text"

for the phone numbers, type="tel" (for correctness)

@@ +220,5 @@
>                   accesskey="&HomeAddress2.accesskey;"/>
> +          <hbox class="AddressCardEditWidth input-container">
> +            <html:input id="HomeAddress2"
> +                        type="text"
> +                        class="input-inline"/>

for this and others were it would all fit well on one line, I'd keep it all on one line

@@ +316,5 @@
>              <label control="Department" value="&Department.label;"
>                     accesskey="&Department.accesskey;"/>
> +            <html:input id="Department"
> +                        type="text"
> +                        class="input-inline"/>

this one is not showing

@@ +589,5 @@
>                <radio id="WebPhotoType" value="web" label="&PhotoURL.label;"
>                       accesskey="&PhotoURL.accesskey;"/>
> +              <hbox class="indent input-container items-stretch">
> +                <html:input id="PhotoURI"
> +                            type="text"

type="url"

::: mail/themes/osx/mail/addrbook/cardDialog.css
@@ +30,5 @@
>  .placeholderOption {
>    color: GrayText;
>  }
>  
> +html|input.YearWidth {

maybe for these it's preferable to remove the default xul namespace instead...
Attachment #9092815 - Flags: review?(mkmelin+mozilla) → review-
Attached image Screenshot from 2019-09-20 11-01-01.png (obsolete) —

The department field shows up for me, how does it look for you?

Regarding the Address double line, I'd suggest to leave it as it's pretty common to have 2 address lines for extra info, like apartment #, buzzer, etc.
I noticed that the label for the second line doesn't actually have any text.
Should we add it for accessibility?

Flags: needinfo?(mkmelin+mozilla)
Attached image ab-textbox.png (obsolete) —

I notice there are other things off too.

Re "keep" it on one line, i was referring to the actual code. (That you now put each attribute on its own line.)

Flags: needinfo?(mkmelin+mozilla)

Yeah, I noticed now the Stat/Province line being busted.
I'm reworking the patch, and applying the aria-labelledby attribute as well.

Attached patch 1563118-textbox-html-input.patch (obsolete) — Splinter Review
Attachment #9092815 - Attachment is obsolete: true
Attachment #9094263 - Attachment is obsolete: true
Attachment #9094267 - Attachment is obsolete: true
Attachment #9094275 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9094275 [details] [diff] [review]
1563118-textbox-html-input.patch

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

Nope. At least Department and Zip hosed, like in the screenshot
Attachment #9094275 - Flags: review?(mkmelin+mozilla) → review-
Attached patch 1563118-textbox-html-input.patch (obsolete) — Splinter Review

This is so weird.
What about this?

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

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

I'll attach a screenshot
Attachment #9094278 - Flags: review?(mkmelin+mozilla) → review-
Attached image foo-input.png (obsolete) —

I think I figured it out.
For some reason your fields have a min-width that doesn't allow them to properly "flex" into the limits of their container.
It doesn't happen on mine, it might be a HiDPI thing.
Working on a fix.

Attached patch 1563118-textbox-html-input.patch (obsolete) — Splinter Review

This should do it.

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

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

Yeah, looks good now.

::: mail/components/addrbook/content/abCard.inc.xul
@@ +354,3 @@
>              <label control="Age" value="&Age.label;"/>
> +            <html:input id="Age"
> +                        type="number"

please add min="0" too
Attachment #9094668 - Flags: review?(mkmelin+mozilla) → review+

Patch updated and try-run pushed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b50186e4b9b764edfeb4e41079bb1980897a579c

This will remove 48 textbox elements, yay!

Attachment #9094387 - Attachment is obsolete: true
Attachment #9094668 - Attachment is obsolete: true
Attachment #9094949 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/06921c904e77
Use HTML input instead of XUL textbox in abCard.inc.xul. r=mkmelin

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