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

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Alessandro Castellani [:aleca]]

https://searchfox.org/comm-central/rev/94d2c771f935e9402ca3d19a43fafeeaa8f0ac4f/mail/components/addrbook/content/abCard.inc.xul

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1563118-textbox-html-input.patch

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.

Comment 2

[Magnus Melin [:mkmelin]]

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...

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-09-20 11-01-01.png

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?

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: ab-textbox.png

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.)

Comment 5

[Alessandro Castellani [:aleca]]

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

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1563118-textbox-html-input.patch

Comment 7

[Magnus Melin [:mkmelin]]

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

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1563118-textbox-html-input.patch

This is so weird.
What about this?

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9094278 [details] [diff] [review]
1563118-textbox-html-input.patch

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

I'll attach a screenshot

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: foo-input.png

Comment 11

[Alessandro Castellani [:aleca]]

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.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1563118-textbox-html-input.patch

This should do it.

Comment 13

[Magnus Melin [:mkmelin]]

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

Comment 14

[Alessandro Castellani [:aleca]]

Attachment: 1563118-textbox-html-input.patch

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

This will remove 48 textbox elements, yay!

Comment 15

[Pulsebot]

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