Closed Bug 1563118 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

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-

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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: