Closed Bug 1527535 Opened 6 years ago Closed 6 years ago

[de-xbl] Replace address book datepicker widget with non-XBL elements

Categories

(Thunderbird :: Address Book, task)

Product:
Thunderbird
Component:
Address Book
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files, 2 obsolete files)

1527535-addrbook-datepicker-3.diff
(deleted), patch
darktrojan
: review+
Details | Diff | Splinter Review
Add old datepicker.xml to suite
(deleted), patch
frg
: review+
Details | Diff | Splinter Review

Like bug 1524456, but for the date picker in the address book, which doesn't need to be a date picker at all. I'm replacing it with two menulists.

Attached patch 1527535-addrbook-datepicker-1.diff (obsolete) (deleted) — — Splinter Review
Attachment #9043494 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9043494 [details] [diff] [review] 1527535-addrbook-datepicker-1.diff Review of attachment 9043494 [details] [diff] [review]: ----------------------------------------------------------------- Yes, works. I think if we don't have a month, the day should also reset. ::: mail/components/addrbook/content/abCard.inc.xul @@ +229,5 @@ > <spacer flex="1"/> > <label control="Birthday" value="&Birthday.label;" > accesskey="&Birthday.accesskey;"/> > <hbox class="AddressCardEditWidth" align="center"> > + <menulist id="BirthMonth"> any reason not just to simply use <html:select> for this? @@ +239,5 @@ > + <menupopup> > + <menuitem label="" value="-1"/> > + </menupopup> > + </menulist> > + <textbox id="BirthYear" maxlength="4" class="YearWidth" /> please keep the placeholder. Actually, the other menus also would benfit from a description for the empty value. @@ +241,5 @@ > + </menupopup> > + </menulist> > + <textbox id="BirthYear" maxlength="4" class="YearWidth" /> > + <label control="Age" value="&Age.label;"/> > + <textbox id="Age" maxlength="4" class="YearWidth" /> and <html:input type="number"> ::: mail/components/addrbook/content/abCard.js @@ +405,5 @@ > + birthDay.menupopup.appendChild(menuitem); > + } > + } > + > + birthMonth.addEventListener("command", setDisabledMonthDays); I think it would nicer to pass in birthMonth and birthDate to setDisabledMonthDays so we don't need to know the ids two times
Attachment #9043494 - Flags: review?(mkmelin+mozilla) → feedback+

(In reply to Magnus Melin [:mkmelin] from comment #2)

  •        <menulist id="BirthMonth">

any reason not just to simply use <html:select> for this?

Yes. It's a XUL dialog, nothing else in it uses an HTML control, and they look awful. Plus the stylesheet doesn't want to work properly on HTML namespace stuff, I tried adding the namespace to the stylesheet, all sorts of things looked wrong afterwards.

and <html:input type="number">

Doesn't offer us anything other than some spinbuttons. You can still enter garbage in there. At this point I'm just wasting time trying to get it to look right.

please keep the placeholder. Actually, the other menus also would benfit
from a description for the empty value.

Okay.

I think it would nicer to pass in birthMonth and birthDate to
setDisabledMonthDays so we don't need to know the ids two times

And instead have to fudge the addEventListener calls? This is much tidier IMO.

Attached patch 1527535-addrbook-datepicker-2.diff (obsolete) (deleted) — — Splinter Review
Attachment #9043494 - Attachment is obsolete: true
Attachment #9044538 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9044538 [details] [diff] [review] 1527535-addrbook-datepicker-2.diff Review of attachment 9044538 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, except for that leap year handling has some bug. The UI still allows (+sticks) to Feb 29 2001 etc. In the display it changes over to March 1. r=mkmelin with that fixed
Attachment #9044538 - Flags: review?(mkmelin+mozilla) → review+

This can't be landed yet because it needs bug 1524456 to land first.

Attachment #9044538 - Attachment is obsolete: true
Attachment #9044570 - Flags: review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/395833021007
Remove datepicker widget from address book; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0

I am having no problem with removing common bindings but this the second time now that it broke SeaMonkey compile without warning. I or IanN would really like to have an advance information.

I know SeaMonkey is broken but we still keep it compiling for a possible later fixup.

Thanks
FRG

Attached patch Add old datepicker.xml to suite (deleted) — — Splinter Review

bustage fix for suite

Attachment #9046163 - Flags: review+
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/ebe97898c429 Copy common datepicker.xml to suite bindings. rs=bustage-fix

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/395833021007
Remove datepicker widget from address book; r=mkmelin

I came here since I noticed that the date picker in the address book doesn't work in TB 66 beta, the month is stuck at January. Now, the patch rips out datetimepicker.xml altogether, which won't make certain add-ons very happy.

Sigh, I've already reported that as bug 1492560.

There's really very little reason for someone to use it. <input type="date"> gives you all you need. Well, if it would. Bug 1527615.

Depends on: 1534104
Type: defect → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: