1443215 - datepicker, timepicker don't work

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jonathan Kamens]

(I don't know where to file this. Does Thunderbird now need its own XUL category since XUL stuff being desupported by mozilla-central is being moved into Thunderbird?)

<datepicker> and <timepicker> XUL elements don't seem to work in current Thunderbird trunk.

I see changes at https://hg.mozilla.org/comm-central/filelog/9804e9b9a400a24ccaba36c58ac0b4ace3a59c73/common/bindings/datetimepicker.xml which seem to have been an attempt to add them, but when my add-on uses <datepicker> and <timepicker> in its XUL with current trunk build, they're completely blank.

There are no errors at all either in the error console or to stdout, so I don't know why they're not working.

Also, I'm a bit confused about why the changesets referenced above removed the <timepicker> implementation from datetimepicker.xml when it was migrated from mozilla-central into comm-central. Is that defined somewhere else? If so, I couldn't find it.

I'm asking because I have a bug-fixed version of the timepicker that I replace the built-in version with, but I have to understand what the code for the built-in version looks like so I can replace it properly, and I can't figure out where that code is in the code base now.

While we're on the subject, it sort of sucks that I took the time to track down a timepicker bug and submit a patch almost 7 months ago in bug 1389791, and that fix was never merged into the source tree. If it had been, then I wouldn't have to be dealing with this. Maybe if we're moving the timepicker definition into Thunderbird (though as noted above I can't figure out where it is) we can get this fix into the Thunderbird source even though it never made it into mozilla-central?

Comment 1

[Jorg K (CEST = GMT+2)]

We're not responsible for slow reviews in M-C for bug 1389791, but I certainly understand the frustration.

Yes, we tried to put that stuff back in bug 1429231, I understand that we use those pickers somewhere, but I don't know where. Richard, can you please take a look.

Comment 2

[Richard Marti (:Paenglab)]

We have only moved the datepicker because we only used this one. We haven't moved timepicker because we wouldn't notice when it regresses.

When you want to use the datepicker binding, you need the binding to the new path chrome://messenger/content/datetimepicker.xml. When your extension also works on SM, it's a different one. I sugest, you pack your own datetimepicker.xml with all needed bindings in your extension and point the bindings to it. Then it works with all TB/SM versions.

Comment 3

[Jonathan Kamens]

>We have only moved the datepicker because we only used this one. We haven't moved timepicker because we wouldn't notice when it regresses.

So there is no intention to support <datepicker> and <timepicker> for those Thunderbird add-ons that currently use it?

Was any effort made to search the source code of current Thunderbird add-ons to find out which of them use one of them and notify their maintainers that they're going away? A push notification via email would have been nice, but at the very least this needs to be documented at https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57. I don't see it mentioned there now.

>When you want to use the datepicker binding, you need the binding to the new path chrome://messenger/content/datetimepicker.xml. When your extension also works on SM, it's a different one. I sugest, you pack your own datetimepicker.xml with all needed bindings in your extension and point the bindings to it. Then it works with all TB/SM versions.

So, again, just to be clear, rather than continuing to support <datepicker> and <timepicker> within the standard XUL framework, you are now asking all maintainers to support it themselves? This is highly sub-optimal.

Also, it's not as simple as just including datetimepicker.xml in your add-on. Add-on maintainers will also need to deal with the localized file datetimepicker.dtd (for &firstdayofweek.default;), as well as with the skin file datetimepicker.css. This is hardly straightforward.

I realize it's not your fault that m-c is getting rid of datepicker and timepicker (which is I assume what happened here?), but I really think TB needs to keep providing bindings for both of them rather than asking every add-on maintainer that uses either of them to roll their own.

Comment 4

[Richard Marti (:Paenglab)]

Also if we would have the full bindings and you want to support TB 52 and 60, you need two different binding paths as we can't provide the old binding path in TB 60.

Comment 5

[Jonathan Kamens]

>Also if we would have the full bindings and you want to support TB 52 and 60, you need two different binding paths as we can't provide the old binding path in TB 60.

Yes, I know, that's one of the reasons why I don't think it's reasonable to pull the rug out from add-on developers by desupporting <datepicker> and <timepicker> and asking them to roll their own.

Comment 6

[Jonathan Kamens]

OK, so, I tried to do what you suggested, i.e., to maintain my own datepicker and timepicker, even though I don't think that's the right approach. I made some progress, but there are still some problems.

I modified my copy of datetimepicker.xml which I copied from mozilla-central before it was deleted, so that all the references in it to datetimepicker.xml referred to the copy in my add-on instead of chrome://global/content/bindings/datetimepicker.xml.

I changed chrome://global/locale/datetimepicker.dtd near the top of it to use a DTD file in my add-on, and I added firstdayofweek.default to that DTD file for every locale, copying the setting from the datetimepicker.dtd files that were recently removed from all the l10n-central locales.

I added <stylesheet src="chrome://communicator/skin/datetimepicker.css"/> and <stylesheet src="chrome://messenger/skin/datetimepicker.css"/>, also left in <stylesheet src="chrome://global/skin/datetimepicker.css"/>, for compatibility with old and new Thunderbird and SeaMonkey. As far as I can tell, I can get away with this because <stylesheet ...> references to stylesheets that don't exist are silently ignored.

I added a css file imported by my XUL files that has datepicker[type="popup"], datepicker[type="grid"], and timepicker moz bindings that point at my datetimepicker.xml.

With these changes in place and using the default Linux theme, I tested my add-on with Thunderbird 52.6.0 and the nightly build dated March 5. I will attach two images -- dtpicker-52.6.0.png and dtpicker-trunk.png -- which show how the datepicker and timepicker work in both of these. Here's what I see:

In Thunderbird 52.6.0, I have managed to preserve existing functionality, i.e., the datepicker and timepicker continue to look as they did before I started trying to fix all this. HOWEVER, the datepicker is still wrong -- the xul:dropmarker that's supposed to appear to the right of it is only one pixel in size and therefore unrecognizeable to anybody who doesn't know what that one pixel is and extremely difficult to click. This is not a new problem; it's been around for a while, and it persists with 52.6.0 even if I use the built-in datepicker rather than my copy. Given that the datepicker code has been moved from M-C into C-C, it would seem that this is now the TB dev team's problem to fix. It's probably a Linux-only CSS issue. One bug in bugzilla that looks like it may be related is bug 1331208. This is a different problem from the one that prompted me to open this bug, so maybe I should open a different bug for it, but as I noted at the top of this bug, I'm not sure what category to file it under since there's no XUL category for TB. Help?

In Thunderbird trunk, you can see from the image that the dropmarker remains one pixel wide, but in addition, there's a new problem -- the up and down arrows that are supposed to appear to the right of the datepicker and timepicker are gone. This is reproducible when I modify my moz bindings for datepicker to point to chrome://messenger/content/datetimepicker.xml rather than my copy, so I'm fairly certain it's broken in stock Thunderbird trunk, not just in my local copy of datetimepicker.xml.

Comment 7

[Jonathan Kamens]

Attachment: datepicker and timepicker in TB 52.6.0

Comment 8

[Jonathan Kamens]

Attachment: datepicker and timepicker on trunk

Comment 9

[Richard Marti (:Paenglab)]

For the dropmarker width: have you changed the class? When it's "datepicker-dropmarker" then this rule should apply: https://dxr.mozilla.org/comm-central/source/mail/themes/linux/mail/addrbook/cardDialog.css#35

Comment 10

[Jonathan Kamens]

I haven't changed any class names.

Comment 11

[Jonathan Kamens]

Ping? Just to summarize all the issues here:

1. The datepicker dropmarker isn't styled properly on Linux (old issue, not new in current trunk).
2. The datepicker arrows are missing in current trunk, at least on Linux.
3. Timepicker has been removed even though add-ons depend on it.
4. There are no default bindings for <datepicker> or <timepicker> even though add-ons depend on them.
5. If we decide that (3) and/or (4) is not going to be fixed, which I personally think is a poor decision, then this needs to be documented in https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57, where it currently isn't mentioned at all.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: Datepicker on Ubuntu 17.10

(In reply to Jonathan Kamens from comment #11)
> Ping? Just to summarize all the issues here:
> 
> 1. The datepicker dropmarker isn't styled properly on Linux (old issue, not
> new in current trunk).

No issue, see screenshot. Yes, the border doesn't look so good. Needs to be fixed.

> 2. The datepicker arrows are missing in current trunk, at least on Linux.

They are shown, see screenshot.

> 3. Timepicker has been removed even though add-ons depend on it.

I'm thinking about adding it. But we don't use it, how should we detect something breaks in it in the future?

> 4. There are no default bindings for <datepicker> or <timepicker> even
> though add-ons depend on them.

We have no access to xul.css where this was removed. You need to add:
<?xml-stylesheet href="chrome://messenger/content/bindings.css" type="text/css"?> in your extension to re-add all in core removed bindings.

Jörg, can you add this to the Wiki page?

Comment 13

[Jonathan Kamens]

(In reply to Richard Marti (:Paenglab) from comment #12)
> Created attachment 8959069 [details]
> Datepicker on Ubuntu 17.10
> 
> (In reply to Jonathan Kamens from comment #11)
> > Ping? Just to summarize all the issues here:
> > 
> > 1. The datepicker dropmarker isn't styled properly on Linux (old issue, not
> > new in current trunk).
> 
> No issue, see screenshot. Yes, the border doesn't look so good. Needs to be
> fixed.
> 
> > 2. The datepicker arrows are missing in current trunk, at least on Linux.
> 
> They are shown, see screenshot.

As I have already indicated -- with a screenshot attached to prove it -- this is not at all what I see.

Rather than snidely (or so it seems) dismissing my lived experience with "No issue", perhaps you could work with me to try to identify why I am seeing decidedly different behavior from you?

I will add two attachments after posting this comment: (1) a trivial, sample extension which opens a tab containing two datepickers, one of type popup and one of type grid, and which reproduces the problem I am seeing in the 2018-03-15 trunk build; (2) a screenshot of what I see in the tab opened by this extension, to demonstrate that it is indeed broken for me.

If you load this extension into Thunderbird and restart, then one of two things will happen: (1) either it will look right for you even though it looks wrong for me, in which case we need to figure out what is different about my environment which is causing it to look wrong for me (note that I'm using the standard Thunderbird nightly build with a clean profile, so I'm not really sure what that could be); or (2) it will look wrong for you as well, in which case we will need to figure out what the sample extension is doing differently from the code whose screenshot you sent. This should not be difficult, given that the entire extension is 58 lines of code.

> > 3. Timepicker has been removed even though add-ons depend on it.
> 
> I'm thinking about adding it. But we don't use it, how should we detect
> something breaks in it in the future?

If the criterion for code to be included in Thunderbird is, "We will detect proactively if it breaks in the future," then I imagine you would have to remove half the code from Thunderbird.

People will tell you if it breaks, just like people tell you when all sorts of other stuff breaks that does not have adequate tests.

Or you could, you know, add some tests.

> > 4. There are no default bindings for <datepicker> or <timepicker> even
> > though add-ons depend on them.
> 
> We have no access to xul.css where this was removed. You need to add:
> <?xml-stylesheet href="chrome://messenger/content/bindings.css"
> type="text/css"?> in your extension to re-add all in core removed bindings.

That seems acceptable to me as long as it is documented.

Comment 14

[Jonathan Kamens]

Attachment: Sample extension which loads a datepicker in a tab

Comment 15

[Jonathan Kamens]

Attachment: What I see in the tab opened by the sample extension

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #12)
> We have no access to xul.css where this was removed. You need to add:
> <?xml-stylesheet href="chrome://messenger/content/bindings.css"
> type="text/css"?> in your extension to re-add all in core removed bindings.
> 
> Jörg, can you add this to the Wiki page?
Can you please suggest a wording. So far we have:
https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57#Changes_in_thunderbird59

I'm not an expert on that, so how about:

Many bindings have moved from Mozilla core to Thunderbird. If you use any of the following
toolbar.xml#toolbox
toolbar.xml#toolbar
toolbar.xml#toolbar-drag
toolbar.xml#toolbar-menubar-autohide
mailWidgets.xml#menulist-description
mailWidgets.xml#menuitem-iconic-desc-noaccel
datetimepicker.xml#datepicker-popup
datetimepicker.xml#datepicker-grid
generalBindings.xml#statusbar
generalBindings.xml#statusbar
generalBindings.xml#statusbarpanel
generalBindings.xml#statusbarpanel

you need to include 
  <?xml-stylesheet href="chrome://messenger/content/bindings.css"> type="text/css"?> in the respective XUL file of your add-on.

Comment 17

[Jorg K (CEST = GMT+2)]

...
you need to include 
  <?xml-stylesheet href="chrome://messenger/content/bindings.css" type="text/css"?>
in the respective XUL file of your add-on.

Comment 18

[Jonathan Kamens]

Jörg, if the decision is made to put timepicker back (which is obviously what I am advocating for), then the list above will need to include the timepicker binding as well, and if we decide not to put it back, then there will need to be a separate addition to the document indicating that timepicker is gone.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: timepicker.patch

(In reply to Jonathan Kamens from comment #13)
> Rather than snidely (or so it seems) dismissing my lived experience with "No
> issue", perhaps you could work with me to try to identify why I am seeing
> decidedly different behavior from you?

It wasn't meant as it sounded, sorry.

This patch adds the timepicker binding and moves the datepicker-dropmarker style on Linux to make it work everywhere.

Jörg, your Wiki proposal looks good, with, when this patch will land, added timepicker line.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959196 [details] [diff] [review]
timepicker.patch

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

Since we don't use the timepicker in TB and I'm not on Linux, I can only rs this. One question below.

Note that Gecko will remove all XBL support in the next few months, so the more we fork, the more trouble we're causing ourselves in the future.

::: mail/themes/windows/mail/datetimepicker.css
@@ +18,5 @@
>  }
>  
> +panel > datepicker {
> +  margin: 0;
> +}

What's this?

Comment 21

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #20)
> Comment on attachment 8959196 [details] [diff] [review]
> timepicker.patch
> 
> Review of attachment 8959196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since we don't use the timepicker in TB and I'm not on Linux, I can only rs
> this. One question below.

This works on Windows too, the birthdate field in addressbook's contact edit.

> Note that Gecko will remove all XBL support in the next few months, so the
> more we fork, the more trouble we're causing ourselves in the future.
> 
> ::: mail/themes/windows/mail/datetimepicker.css
> @@ +18,5 @@
> >  }
> >  
> > +panel > datepicker {
> > +  margin: 0;
> > +}
> 
> What's this?

To remove the fat border around the datepicker in a popup, see attachment 8959196 [details] [diff] [review] how it looks without this.

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8959196 [details] [diff] [review]
timepicker.patch

For extension compat this should be uplifted.

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #21)
> This works on Windows too, the birthdate field in addressbook's contact edit.
Sure, that's the *date*picker, no? Not the *time*picker, or am I missing something?

Comment 24

[Jonathan Kamens]

Attachment: What I see with patch in 52.6.0 vs. trunk

I applied your patch to a current trunk source tree and build it from scratch. Then I added a <timepicker/> to the bottom of the tab that my proof-of-concept extension creates (will attach the updated extension after posting this comment) and launched Thunderbird 52.6.0 and trunk (with your patch) with the extension installed.

In the 52.6.0, both the pop-up datepicker and the timepicker have up/down arrows to the right of them as they should, but the downmarker on the datepicker is just a pixel wide, i.e.,, virtually invisible. 

In the trunk, the pop-up datepicker has a proper downmarker, but the up/down arrows have disappeared from both the datepicker and timepicker.

The attached image shows 52.6.0 vs. the trunk.

In both cases I was using a completely clean Thunderbird profile -- the only thing changed from a new profile was to install the POC extension.

Comment 25

[Jonathan Kamens]

Attachment: Sample extension that creates both datepicker and timepicker in the tab

Comment 26

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #23)
> (In reply to Richard Marti (:Paenglab) from comment #21)
> > This works on Windows too, the birthdate field in addressbook's contact edit.
> Sure, that's the *date*picker, no? Not the *time*picker, or am I missing
> something?

The timepicker is on Linux also not used by TB.

Comment 27

[Richard Marti (:Paenglab)]

(In reply to Jonathan Kamens from comment #24)
> In the trunk, the pop-up datepicker has a proper downmarker, but the up/down
> arrows have disappeared from both the datepicker and timepicker.

The spinbuttons are removed too in core. :(

Comment 28

[Jonathan Kamens]

(In reply to Richard Marti (:Paenglab) from comment #27)
> The spinbuttons are removed too in core. :(

I don't know whether to read this as, "The image files used for these buttons were removed in core so we have to copy them into Thunderbird and modify all the style sheets to reference them in the new location," or rather, "The datepicker and timepicker code that we copied from core into Thunderbird removed the spinbutton functionality and we're not going to put it back." Or maybe you meant something else entirely? Can you clarify? Thanks.

Comment 29

[Jorg K (CEST = GMT+2)]

Maybe not ready to land yet, please clarify.

Comment 30

[Richard Marti (:Paenglab)]

The spinbuttons aren't in datetimepicker binding. They are a other binding -> we need a new bug for this port.

Comment 31

[Jorg K (CEST = GMT+2)]

Can we do this for quickly and uplift both to beta?

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd4fc0651bec
Add the timepicker binding to the bindings. r=jorgk

Comment 33

[Jonathan Kamens]

(In reply to Richard Marti (:Paenglab) from comment #30)
> The spinbuttons aren't in datetimepicker binding. They are a other binding
> -> we need a new bug for this port.

Sorry to be dense, but by "we need a new bug" do you mean "you should file a new bug"?

Comment 34

[Jorg K (CEST = GMT+2)]

Beta (TB 60):
https://hg.mozilla.org/releases/comm-beta/rev/26224b3e60fb60c03a4371552255bcd3f8bd436a

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to Jonathan Kamens from comment #33)
> Sorry to be dense, but by "we need a new bug" do you mean "you should file a
> new bug"?
Already done, landed and uplifted to beta in bug 1446329.

Comment 37

[Jonathan Kamens]

Well, that didn't work. Apparently bugzilla doesn't like unicode emoji. That was supposed to be "Person With Folded Hands." :-)

Comment 38

[Jorg K (CEST = GMT+2)]

https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57#Changes_in_thunderbird59 updated.

Comment 39

[Jonathan Kamens]

Q: How is this going to play out in SeaMonkey? Is SeaMonkey going to automatically get the binding and style changes that have been created as a result of this bug, bug 1446329, and others, or is there additional work to ensure that these XUL bindings continue to be available in new SeaMonkey releases? If the latter, what's the right way to give a heads up about this to the folks maintaining SeaMonkey?

Comment 40

[Richard Marti (:Paenglab)]

The XML file is in a place they can pick it easily. But they need to add it their self. Unfortunately SM is actually in a bad state and they are behind in adding the removed bindings.

Comment 41

[Jonathan Kamens]

Are these and the others on their radar, i.e., will they get to them eventually without further input, or does somebody need to make sure they know about them?

Sorry if these are stupid questions, I just want to do everything I can do ensure that my add-on doesn't suddenly stop working in SeaMonkey. :-/

Comment 42

[Richard Marti (:Paenglab)]

The questions aren't stupid. I don't follow the SM bugs and I think, it doesn't hurt when you file a bug for them to be sure it's on their radar. If they have already a bug, your bug will be duped.

Comment 43

[Jonathan Kamens]

Thank you. See https://bugzilla.mozilla.org/show_bug.cgi?id=1447776.

Comment 44

[klaus]

see also https://bugzilla.mozilla.org/show_bug.cgi?id=1516360 for a disagreement between MDN definition of datepicker members and current implementation