306495 - autodetect remote calendar type so user doesn't need to pick (with DNS or .well-known)

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, enhancement)

Description

[Dan Mosedale (:dmosedale, :dmose)]

When selecting a remote calendar, it's often not obvious to a user what WebDAV
and CalDAV mean, and which they should pick.  It should be possible to
autodetect whether a URL supports CalDAV or not.  We should do so, and make this
piece of UI go away.

Comment 1

[Helge Hess]

Additionally it would be great if WebDAV servers are detected. That is, the
panel detects that the URL is not CalDAV but _is_ a generic WebDAV collection,
it should  bring up a WebDAV browser allowing the user to navigate to a CalDAV
(or GroupDAV ;-) collection.

Suggested sequence:
a) check for CalDAV using OPTIONS
   - if its a "leaf" CalDAV collection, directly subscribe
   - if its a collection containing CalDAV folders, show a panel with the 
     available calendars for subscription
b) check for WebDAV by trying to issue a PROPFIND (and handling a HTTP 501)
   - show a panel for traversing the WebDAV hierarchy, highlight CalDAV,
     GroupDAV collections and c)-like entities for subscription
c) try a GET and check whether the result is text/vcalendar or text/xcal or 
   whatever is understood
   - directly subscribe the GET/PUT-all-in-one calendar
c) show an error

Comment 2

[Joey Minta]

Would accept a patch in the very near future for this.  Without fairly broad test-coverage to ensure we get this right, we won't take it though.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Cleaning up incorrectly assigned bugs; search for dmosecleanup to get rid of this bugmail.

Comment 4

[Sebastian Schwieger]

see bug 355270 comment 12 (point 3 and 4) and following for a related discussion.

Comment 5

[Bruno Browning]

Attachment: proposed revamped calendar-creation wizard ui for autodetection

Proposing a slightly-revamped calendar creation UI to accommodate calendar-type detection. 

This has discovery of existing-but-unregistered CalDAV calendars, which I think we need to do, partly because Apple has made doing so standard. We could do something similar for ICS-on-WebDAV without changing this UI substantially (though we wouldn't be using a DAV:displayname property).

Comment 6

[Christian Jansen]

The proposal looks good for me. 

Some comments:
I would suggest to use check boxes instead of option buttons for "Choose an existing calendar". This would allow users to subscribe to more than in one single step.

It should also be possible to search/filter calendars on a specific server.

[ John                           ] [Search]

+--------------------------------+
| [ ] John Doe                   |
| [ ] John's Sports Calendar     |
|                                |
|                                |
|                                |
+--------------------------------+

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

While this proposal looks fine on first sight, it fails for non-url based providers. For example, given that I would want to create a wizard that lets users select their google calendars by only entering username and password, where would I hook that in?

Comment 8

[Bruno Browning]

(In reply to comment #6)

> I would suggest to use check boxes instead of option buttons for "Choose an
> existing calendar". This would allow users to subscribe to more than in one
> single step.
> 

I assume that we would then want a multiple-calendar Customize page, allowing the user to set display name / color / alarms for all the newly-subscribed calendars all at once? Not terribly simple, but I think preferable to a sequence of single-calendar Customize pages.

> It should also be possible to search/filter calendars on a specific server.
> 
> [ John                           ] [Search]
> 
> +--------------------------------+
> | [ ] John Doe                   |
> | [ ] John's Sports Calendar     |
> |                                |
> |                                |
> |                                |
> +--------------------------------+
> 

Searching is going to raise some new UI issues here: we won't be able to simply use the DAV:displayname property when displaying search results and when pre-filling the Name field on the Customize page, due to the inevitability of collisions in that space cross-principal (for instance, it's quite likely that several people will name their main calendar collection "Work Calendar"). So I think we'll want to in some fashion include an indication of the associated principal as well as the displayname. The obvious way of doing this would be:
(Christian Jansen) Work Calendar
Which is simple but expensive in terms of horizontal screen real estate. That's probably acceptable when displaying search results but probably not for use in the calendar list - and if we're going to pre-fill that Name field in the wizard we ought to do so with something usable. Another possibility would be to introduce some hierarchy into the calendar list, e.g.
Christian Jansen
|-Work Calendar
This burns screen space vertically instead and provides a familiar UI (similar to the Thunderbird accounts/folders pane). It would also allow us - if we decided we wanted to - to display the CalDAV Inbox/Outbox/Notifications folders in the calendar list. That latter is of questionable utility though. There's been some discussion of this in bug 407840. Also, I think that currently CalDAV is the only provider that would benefit from such a hierarchy.

(In reply to comment #7)
> it fails for non-url based providers

Good point. Perhaps that first page should read
0 On My Computer
0 At Google
0 Elsewhere on the Network

? I assume we'd want some way for any new non-url-based providers to plug themselves in here as well.

Comment 9

[Bruno Browning]

I'm nominating this for the tb-integration flag. Our current calendar-creation process is a huge usability issue; we need to do better before Lightning lands in tb. I think we have a good idea what needs to be done here, at least initially, so it should not be too much of a stretch.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Regarding the UI, I think we should reduce wizard steps if the wizard screeen space permits. Also, I think we might still want to allow the user to select the provider, but instead either preselect the right type after detection or provide an "advanced..." button to let the user change that selection.

Also, when fixing this bug, we should keep in mind bug 378873 and its dependant bug. If at all possible, we should allow providers to plug in somewhere in case they are not url dependant or need custom steps. This might partially be covered in the proposal from comment 8, but we need some ui-review there.

Regarding the possibility to create a new calendar or choose existing calendars, we may want to make this more pluggable as well. I can imagine that other server types can provide facilities to choose existing or create new calendars and maybe want to display more information (maybe a color, or a timezone) when choosing the calendar. In an early stage of the gdata provider I mocked up a wizard page that used a <tree> that contained the existing calendars' display name, color and timezone. Maybe something like this would be beneficial, allowing the provider to implement its own tree view to display the calendars, maybe providing base prototypes to ease things for beginners.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think the next step for implementation is to think of some sort of API that allows extensions and shipped providers to determine the type of provider from the uri, with some sort of importance level. Imagine the following scenario:

https://www.google.com/calendar/dav/[e-mail]/user/
* The caldav provider will try to detect this and fail due to a Google bug.
* A separate implementation of the API could detect that this is specifically a
  Google CalDAV server and return "caldav".

For the importance I currently don't have a specific use case, but in general I can imagine:

* Provider A detects a certain URL to be itself and returns "providerA"
* Provider B does a much better job since it has server-specific extensions to
  whatever Provider A does and returns "providerB".

Without importance, depending on in which order providers are enumerated, providerA may win even though providerB does a better job.

Querying the provider should be done asynchronously, since its possible that the detection mechanism needs to do further HTTP requests to find out if its the correct provider.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Early ideas regarding custom calendar subscription (GData)

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

From bug 446099, this autodetection code should also handle HTTP errors like 404, telling the user that no calendar resources could be found at the location provided.

Comment 15

[Bernard Desruisseaux]

It should be noted that multiple CalDAV server implementations (e.g., Apple, Google, Oracle, etc.) also provide support for Internet Calendar Subscriptions by allowing GET requests to be targeted at CalDAV calendar collections.

While the default should be to access the calendar collection with CalDAV when possible, users should have a way to specify that they simply want to subscribe to the calendar.

With Apple iCal this is a non issue since you are forced to specify a principal URL when configuring a CalDAV account.

Comment 16

[David :Bienvenu]

I'm marking P1 based on the theory that this is pretty important for adoption - feel free to correct me if I'm wrong.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think this is actually more than one bug. One part of it would be the caldav part of discovering the right calendar urls using OPTIONS, the other would be to revise the new calendar dialog to allow auto-config similar to what is anticipated for thunderbird.

In any case, I don't believe this is something that is required for Lightning being integrated into Thunderbird. I do believe it is beneficial and the caldav part should be blocking-calendar1.0+, but I think we can live with the old dialog for 1.0/Thunderbird3, if time doesn't permit.

Updating the new calendar dialog is something we'll need the thunderbird autoconfig dialog for. We should split this bug, I don't have the time to do it now though.

Comment 18

[Les Barstow]

Agreed that this might be more than one bug, and that you can get away with releasing 1.0 without it, but I think P1 is appropriate given the immediate pain that resolving it would relieve.

I agree with Bruno's comment above - Apple has made CalDAV discovery using the /principals/users/{username} URL the gold standard that everyone wants to follow.  I set up an iPhone with Zimbra CalDAV yesterday and it was both painless and pleasant.  The same cannot be said about calendar subscriptions in Lightning (1.0b2pre).

Comment 19

[Andrew McMillan]

While it is true that Apple's search for /principals/users/{username} is first kid on the block, any implementation should consider that there is a standard under development that will replace it.

See the "Defining Well-Known URIs" Internet-Draft:

 http://tools.ietf.org/html/draft-nottingham-site-meta-04


Combine that with the current-user-principal extension (RFC5397) and there is a very straightforward path to finding a user's calendars once you have a server hostname and port to connect to.  Sprinkle an appropriate _srv lookup onto that and in due course it should be possible to find a user's calendars simply from the domain name and authentication details.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Lenni will be working on this for the 2011 Google Summer of Code. I'm looking forward to his work!

Comment 21

[Dave Miller [:justdave]]

(In reply to Philipp Kewisch [:Fallen] from comment #20)
> Lenni will be working on this for the 2011 Google Summer of Code. I'm
> looking forward to his work!

2011 GSoC is over, did it actually happen?

Comment 22

[Felix Möller]

Dave the result of the GSoC can be found as an attachment to bug #378873 as far as I know.

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Dave, the result is indeed the extension mentioned there. I am still in contact with the student and he will be continuing his work soon. I have found the extension to work fine for some calendar servers, but it still needs refining for servers that do special things (i.e for Google servers)

Comment 25

[Greg K.]

Firefox OS appears to implement CalDAV auto-provisioning (that is, you enter one URL, specify your credentials, and all your calendars are loaded).

Any chance the same back-end code, at least, can be used by Lightning? Who did that work?

Comment 27

[Greg K.]

Since Lightning's never going to get the cycles Mozilla will put behind Gaia, why not leverage their work for this? I can't believe the underpinning's of Gaia's calendar client are so different as to make that impractical.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Its not too much about the actual code to do the detection, but rather the code to hook it up to Lightning's quite historical architecture. Aside from what has been done in this bug, there needs to be some changes to make autodetection possible for all types of providers, then its trivial to put the existing code into the caldav provider.

In an ideal world we would also change the calendar manager to have accounts that have calendars, instead of a simple list of calendars.

And foremost, its a matter of finding someone with enough time to take care :)

Comment 29

[Greg K.]

Thanks, Philipp, that makes sense. It sounds like there could be some discussion (not in this bug), then, about the idea of perhaps rebasing Lightning off the calendar work in Gaia, to whatever extent possible.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Indeed, there is some code that could likely be used there. To get there we need to do some more backend changes though, b2g has the advantage that they got to reboot the whole architecture. First and foremost, we want to switch from libical to ical.js. It is already part of the production code, but there are still some blocking issues keeping us from enabling it by default.

I already have a work in progress caldav provider locally that uses the b2g-caldav code. Until we have ical.js enabled, it still is a bit cludgy (convert iCalendar from caldav -> jCal in b2g-caldav -> iCalendar to be usable with Lightning -> Lightning's objects). Also, some of the more advanced caldav features are not yet adapted.

There are surely some other areas we can and should make use of the b2g code, but again, we need developer to move in that direction. If this is something you'd like to help out with, please email me. Its a step by step process :)

I suggest further discussion either via email or in the mozilla.dev.apps.calendar newsgroup.

Comment 34

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

When finishing up this bug, the following things should be taken care of:

* caldav autodetection via well-known uris
* DNS SRV/TXT autodetection
* Trying common patterns

If any of this is not taken care of, followup bugs should be filed.

Comment 41

[Johannes Rohr]

I understand that commenting here when I don't contribute any code myself is not necessarily productive, but I just wanted to flag, that all CardDAV/CalDAV clients which appeared since this bug was first reported have auto discovery, neither on an iPhone, gnome-online-accounts, an iBook, DAVdroid, the CardBook addon for TB, not even Windows 10 do I have to provide the full URL to the individual calendars. So Lightning seems have vastly fallen behind in those 12 years. So how Caldav is handled in Lightning really feels so antiquated. As I am trying to get my organisation to use  Nextcloud  for joint scheduling, I am confronted daily with the fact that you only need to enter three things on most clients (URL, user name, password) to get all of your passwords and address books onto your device, but TB/Lightning is the negative exception to the rule. 

Since the CardBook extension already implements DAV discovery for TB, it can't be rocket science after all to adapt this code for Lightning. Maybe I'm wrong about that. But I have difficulties understanding why what is not a problem for CardBook should be so much more difficult for Lightning.

Comment 42

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

It is not rocket science, and I have some work in progress code that I never manage to finish up :-/ Sorry for the delays!

Comment 43

[[:MakeMyDay]]

Philipp, what's the scope and state of your wip patch for this bug? For bug 1299610, I would need to fire an OPTIONS request to detect auto-scheduling capability of the respective calendar for a consistent UX across calendar setup and property editing.

If this bug is not only about detecting the appropriate base URI for a calendar principal for a calendar server, but also about providing a list of available calendars in a users collection within the wizard, such a detection is probably already part of your patch?

Comment 44

[Johannes Rohr]

(In reply to Philipp Kewisch [:Fallen] from comment #42)
> It is not rocket science, and I have some work in progress code that I never
> manage to finish up :-/ Sorry for the delays!

Hi Philipp, it was great to hear that someone is working on it at last. Did you by chance have the time to make any progress on that since you commented 5 months ago? Really sorry for nagging, but this would be one of the major issues with lightning being finally solved....

Comment 45

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'll upload my WiP patches soon. IIRC it was just a matter of polish and some cleanup, possibly some migration code. The code is complete from backend to UI. I'm not sure if it will allow for the use case in comment 43, but at least some of the calendar info will be available earlier on.

That said, I haven't touched them in 5 months. It would be nice to get this in, but of course it is too late for strings again. Maybe this will get easier with the new gecko-strings repo and we can ship it earlier.

Comment 46

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I take back the strings part, I was thinking 57. We still have time until 59 hits beta.

Comment 47

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 1 - Move openLocalCalendar out of calendar-creation.js - v1

This needs to be done in a separate patch, otherwise hg freaks out.

Comment 48

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Part 2 - Autodetection WiP - v1

This is the main patch. It looks like I haven't taken care of l10n yet. The strings required are hardcoded. I see I used todo-l10n in some cases, but I'm not sure if this is all.

Given it is not a lot of strings we could probably pull these out to commit now, but I won't get to that before the end of the year. Anyone else interested?

Comment 49

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Strings - v1

Here are the strings I'd like to push. The patch only contains additions to avoid running into trouble if I don't mak eit. If you would like to see them in action please check out this clickable mockup: https://app.moqups.com/kewisch/h5tL4vWrox/view/page/adcac4348

Comment 50

[[:MakeMyDay]]

Comment on attachment 8958308 [details] [diff] [review]
Strings - v1

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

r+, just some nits for consistency.

::: calendar/locales/en-US/chrome/calendar/calendarCreation.dtd
@@ +13,2 @@
>  
> +<!ENTITY locationpage.description           "Provide info about what is needed to access your remote calendar" >

Even if it has been that way before, maybe information is better then info here.

@@ +18,5 @@
> +<!ENTITY locationpage.location.placeholder  "URL or host name of the remote calendar service">
> +<!ENTITY locationpage.optional.placeholder  "optional"
> +
> +<!-- Discovery states -->
> +<!ENTITY discovery.inprogress.label "Please wait while your calendars are being discovered">

inprogress hasn't a closing dot other then the other discovery labels.

@@ +24,5 @@
> +<!ENTITY discovery.authfail.label "The username and password you have entered were not accepted. Please check your settings."
> +
> +<!-- Calendar selection -->
> +<!ENTITY selectcalendar.description "Please select the calendars you would like to subscribe to.">
> +<!ENTITY providertype.label "Calendar Type">

The other labels for input controls have a trailing :

Comment 51

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: [checked in] Strings - v2

Thanks for the review. I spent some time actually implementing the dialog to see how it feels and got some feedback from #maildev. I'm also opting for better string entity names instead of name consistency that is no longer correct.

I'm going to push strings v2 now.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c8cb75798fd
Add strings for calendar autodiscovery. r=MakeMyDay

Comment 53

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Obviously not fixed yet. The strings were technically pushed with late-l10n, so I am marking as such. Bug staying open for the actual patch which is 90% done. Need to handle some of the edge cases.

Comment 54

[Worcester12345]

You were talking about porting in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=467153#c15

Not sure if that has happened here yet or not.

Comment 56

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Move files around for better naming - v1

This patch depends on bug 1546606 and applies on an older version of c-c, please see bug 1546606 for the parent. I'm dumping it here because it doesn't look like I will get to this any time soon, but I'd like it to be finished. Magnus, can you find someone to work on this?

There are two parts to it, mainly because hg got confused when I had them both in one. Maybe the hg mv'ing around is not really great. I'm fine adapting the filenames differently if necessary.

Comment 57

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Autodetect calendar code - v1

This is the actual meat. What we may want to do is de-xpcom the new classes, as we're aiming to get rid of it. I'm fairly certain there are no tests in this patch, but some of those would be good too.

Comment 58

[Magnus Melin [:mkmelin]]

-> Paul.

Comment 59

[Khushil Mistry [:khushil324]]

Comment on attachment 9060343 [details] [diff] [review]
Autodetect calendar code - v1

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

::: calendar/base/content/dialogs/calendar-creation.xul
@@ +43,5 @@
> +           ondialogaccept="return createOther()"/>
> +    </tabs>
> +    <tabpanels id="source-tabpanels" flex="1">
> +      <tabpanel id="source-local-tabpanel">
> +        <grid id="local-grid" flex="1">

This can be converted into the html:table. Also, we can convert the labels to <html:th>.
Example: https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-item-summary.js#50

@@ +68,5 @@
> +        </grid>
> +      </tabpanel>
> +      <tabpanel id="source-network-tabpanel">
> +        <deck id="network-deck" flex="1">
> +          <grid id="network-deck-settings" flex="1"

html: table and convert the labels.

@@ +135,5 @@
> +          </vbox>
> +        </deck>
> +      </tabpanel>
> +      <tabpanel id="source-other-tabpanel">
> +        <grid id="other-grid" flex="1">

This is the container grid. There will be a child grid here so inline-grid will be useful here.

@@ +155,5 @@
> +              <label value="&calendar.server.dialog.name.label;" control="other-calendar-name-textbox"/>
> +              <textbox id="other-calendar-name-textbox" flex="1" required="true" oninput="checkRequired()"/>
> +            </row>
> +            <deck id="other-calendar-type-deck" flex="1">
> +              <grid id="other-wcap-grid" flex="1" provider="wcap">

This can be easily html:table and convert the labels.

Comment 60

[Paul Morris [:pmorris]]

Attachment: on-my-computer.png

Thanks Khushil! I've now got the patch mostly un-bit-rotted by converting various things to the new ways to do them. Still WIP.

• I've omitted the "other" tab for now since we no longer support WCAP calendars (bug 1579020) and birthday calendars appear to be unimplemented. (Those were the two options in the "other" tab.) We can add this back when needed.
• I left out the gdata changes since it is now in its own repo.
• I've kept the two new XPCOM classes as XPCOM for now.
• Creating local calendars works.
• TODO: Troubleshoot issues when trying to set up a remote google calendar, do further testing.
• TODO: calendar-list-tree no longer exists, so will need to use a richlistbox instead, like we've done elsewhere
• TODO: add an email field to local calendar tab, as in the current dialog.

Here are a few screenshots. (I know the input fields should be wider.)

Comment 61

[Paul Morris [:pmorris]]

Attachment: on-the-network.png

As you type in an email address, the domain appears in the location field. (You can still type something into the location field if you want to override it.) There's no need to choose "CalDAV" or "ICS" as in the current dialog; it is automatically detected.

If you check the box for "doesn't require credentials" the username, password, and "use password manager" rows are immediately hidden.

Comment 62

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Make changes to `openLocalCalendar` function. r=darktrojan

Comment 63

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Move openLocalCalendar out of calendar-creation.js. r=darktrojan

Depends on D78369

Comment 64

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Implement a new calendar creation dialog. r=darktrojan

This is the main patch for this bug that Philipp wrote, with
various changes made to counter bit rot and update some parts.

For example WCAP calendars are no longer supported and the
gData provider is now in its own repo, so I have removed
these parts. That includes the "other" tab since it contained
WCAP and birthday calendars, and birthday calendars are not
implemented yet.

The bit rot changes include using HTML table instead of XUL grid,
using static XPCOM component registration in components.conf
files, updating module import syntax, replacing
calendar-list-tree (no longer exists) with a richlistbox,
using 'addEventListener' instead of 'ondialogaccept' attributes
since the latter were not working, updating things related
to <dialog> now being a child of <window>, etc.

Depends on D78370

Comment 65

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - WIP De-xpcom calDavProvider and calICSProvider modules. r?darktrojan

Depends on D78371

Comment 66

[Paul Morris [:pmorris]]

I just uploaded my current WIP patches so Philipp can take a look, particularly for how to do the de-xpcom part. (Apologies for the churn. When using phabricator I guess I should leave the 'r?darktrojan' out of the commit messages until I'm actually ready for review.)
Edit: I've used comments with "AUTODETECT" to mark places where I know there are still things to do.

Comment 67

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Paul Morris [:pmorris] from comment #64)

For example WCAP calendars are no longer supported and the
gData provider is now in its own repo, so I have removed
these parts. That includes the "other" tab since it contained
WCAP and birthday calendars, and birthday calendars are not
implemented yet.

When I wrote this, I was trying to keep extensibility in mind. Add-ons may be adding different types of calendars, some can do autodetection based on few information, others might require a specific URL, others might not need anything at all. The birthdays case was for example to account for the ThunderBirthday extension.

Even if these are empty (and thus hidden) now, it would allow provider extensions to be more flexible.

Comment 68

[Paul Morris [:pmorris]]

Thanks Philipp for the feedback. I've just uploaded a new WIP revision that:

• addresses almost all of your comments
• has a working first pass on the de-xpcom parts like we discussed
• is now working for subscribing to network calendars
• includes some other minor improvements

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #67)

When I wrote this, I was trying to keep extensibility in mind.

Ah, okay, that makes sense. I have some questions, so will try to set up a meeting to discuss extensibility details and a few other items like the TODOs for filterprovider and preferprovider, etc.

Comment 69

[Paul Morris [:pmorris]]

Attachment: create-calendar-dialog-sidebar.svg

On extensibility, maybe there are better options than having an "other" tab. Having an "other" tab implies that things in it don't fit under the "on my computer" or "on the network" tabs. For example, technically "birthdays" would be "on my computer" and WCAP (if still supported) would be "on the network". So for those to be under "other" is not that bad but not ideal either.

What if, instead of tabs at the top, there was a list on the left side where add-ons could add new options below "on the computer" and "on the network". (Like "birthdays" in this image.) That avoids having an "other" category and you don't have to click on "other" to see what's under there. All the options are visible at the top level.

(You could do the same thing with the tabs at the top, allowing add-ons to add new tab types, but I think the sidebar list would be better for use of space. Also picking an item from a list seems more in line with this kind of dialog flow than the tabs at the top.)

Alex, I'd be curious to hear what you think.

Comment 70

[Alessandro Castellani [:aleca]]

That's interesting, thanks for the NI on this.

I'm not sure about the sidebar list because, even if it's a good concept to organize and allowing the implementation of multiple sections, it doesn't translate very well in the context of a dialog.
A sidebar takes up too much space, that's why we use those in Tabs, and since this is a creation wizard, you don't really need to always see the other options once you make the decisions of creating a specific calendar.

Probably I missed something from previous comments (apologies if my questions are silly or already answered) but is there any specific reason why we don't want to keep the initial radio button selection?
Another solution might be reusing the styling of the Account Central by implementing those styled hub buttons for the different calendar types.

Comment 71

[Paul Morris [:pmorris]]

Thanks Alex. I've discussed some of these UI/UX details with Philipp, Magnus, and Alex over the last two days, and here are the main take-aways:

• go back to using the radio button selection for the first view (for "on my computer" or "on the network"), rather than tabs or sidebar
• probably show no password field (and no "save password with password manager" checkbox) on the first "on the network" view
• then if a password is needed, ask for it on the next view (along with a "save password with pwd manager" checkbox)
• by default check the "use pwd manager to save this pwd" checkbox, for consistency with email setup wizard
• maybe disable rather than hide the username field when "no credentials" checkbox is checked, to prevent jumpiness (if this works well)
• for the calendar properties view, don't allow edits to username or calendar name (e.g. like location which is already read-only there)

There are further improvements that we want to do, but that's for a future iteration. (For example, streamlining setting up calendars for existing email accounts, etc.)

Comment 72

[Paul Morris [:pmorris]]

Attachment: disabled-calendars.png

Another UI question. How to handle when the user has already subscribed to some of the network calendars found by auto-detection:

1. Don't include already subscribed calendars in the list of calendars that you can subscribe to.
2. Show them in the list but disabled so their text and color circle are greyed out and you can't check their checkbox.

1 is simpler but the user won't see all the calendars for a given provider in the list, which might be confusing. With 2 the user can see all their calendars for a provider in the list but some of them are disabled.

I have 2 working, since Philipp's patch was already moving in that direction (see comment below). It would be easy to switch to 1 if we want that. If we want 2, then if all the calendars are already subscribed, we'd want a string to indicate that (as Philipp mentions in the comment below). We don't have such a string right now and the string freeze date has passed.

Another approach (also involving a new string) would be to add something like this to the names of the disabled calendars in the list:

[ ] Thunderbird Events (Already Subscribed) 

In the attached image the first two calendars in the list are disabled (already subscribed) and you cannot check their check box.

Philipp's comment on this from his patch:

// Remove existing calendars first. In the future we may just show them with special UI, but
// then we also need a string to show in case all calendars are already subscribed.

Comment 73

[Alessandro Castellani [:aleca]]

I agree with solution 2.
Show the already subscribed calendar disabled to let the user know that we found them but he doesn't need to do anything.
The (Already subscribed) string would be a really nice addition, but I guess it's not possible since we missed the string freeze window.

Comment 74

[Paul Morris [:pmorris]]

Thanks aleca, that makes sense to me.

I've uploaded a new revision of the patches to Phabricator that, among other things (like adding email field to local calendar panel), implements the changes from comment 71, (except for "for the calendar properties view, don't allow edits to username or calendar name" which I haven't gotten to yet).

The way I've done the separate password step could be more efficient, but it should be sufficient to get it working. More testing of edge cases is needed. (When an authentication error occurs, advance to the password screen to allow entering a password, then do everything again but now with the password. Ideally we'd use the results from the first attempt to only do what was really needed on the second attempt.)

I've started to update the existing tests. The browser_localICS.js test is still failing, and I haven't had time to fully investigate, but it looks like there might be an issue there beyond just updating the test.

There are various refinements left to do, but I've uploaded another WIP patch in case anyone wants to take a look or test it out.

Comment 75

[Paul Morris [:pmorris]]

Updated the main patch again.

• Added support for subscribing to local ICS files (needed for one of the tests).
• Existing tests updated and working.
• Existing tests converted to mochitest style.
• Password prompt is now a separate prompt rather than a panel in the dialog (per discussions with Magnus and Alex).
• Changes to calendar color and name are reflected in the calendar list.

Main outstanding tasks:

• Extensibility: make it easier for add-ons to add UI (radio button on first panel and panel with iframe they can add UI to, like cloudfile provider)
• Write tests.
• Add more assertions to existing tests.

UI questions:

• "no credentials" check box, currently disables username and clears it. Is this working like we want? Previously it hid (and cleared) the username, password, and "save password" elements.
• Now that we've changed the password flow, "save password in password manager?" is never asked or answered in the oauth flow (e.g. google calendar).

Comment 76

[Magnus Melin [:mkmelin]]

For OAuth2, "save password" is not really a thing. What you really save is the token anyway... Using it without saving "passwords" would be very cumbersome - and it's also not supported when accessing mail with OAuth2 authentication.

Comment 77

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Make the calendar creation dialog more extensible. r=Fallen

To allow for add-ons that want to add their own UI to this
dialog for particular calendar types (e.g. birthdays).

Depends on D78371

Comment 78

[Paul Morris [:pmorris]]

Comment on attachment 9154333 [details]
Bug 306495 - Implement a new calendar creation dialog. r=darktrojan

Thanks Magnus for answering that Oauth question, glad it's not an issue.

Patches are ready for review now. I'm also flagging aleca for ui-review and Fallen for feedback on these patches. The three cases I've been focusing on are 1. local calendars, 2. caldav calendars (e.g. fastmail), 3. oauth calendars (google).

The existing tests are updated to work. The next step on my list is to write more tests to better cover the new functionality. I can do that either in another patch on this bug before landing or we could do it as a follow-up if we want to go ahead and land this to get some experience with it on nightly.

Comment 79

[Alessandro Castellani [:aleca]]

Comment on attachment 9154333 [details]
Bug 306495 - Implement a new calendar creation dialog. r=darktrojan

removing the UI review flag as the patch has bitrotted.

Comment 80

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Improve error handling in calendar detection code. r=darktrojan,Fallen

Also refactor the previously unused error handling code to make
it work for this.

Depends on D78371

Comment 81

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Convert `Autodetect` module to `cal.provider.detection`. r=darktrojan

And rename from "autodetect" to "detect" or "detection" everywhere.

Depends on D82231

Comment 82

[Paul Morris [:pmorris]]

Comment on attachment 9154333 [details]
Bug 306495 - Implement a new calendar creation dialog. r=darktrojan

Requesting ui-review now that the patches are updated to address Geoff's review. Here is the order to use when applying the patches.

1. Bug 306495 - Make changes to openLocalCalendar function. r=darktrojan
2. Bug 306495 - Move openLocalCalendar out of calendar-creation.js. r=darktrojan
3. Bug 306495 - Implement a new calendar creation dialog. r=darktrojan
4. Bug 306495 - Always reject the CalDavResponseBase.responded promise with an error. r=darktrojan
5. Bug 306495 - Convert Autodetect module to cal.provider.detection. r=darktrojan
6. Bug 306495 - Make the calendar creation dialog more extensible. r=darktrojan

(See the "Downloading a patch from Phabricator" section here: https://pypi.org/project/MozPhab/ for how to download a series of patches all in one command using moz-phab.)

Comment 83

[Alessandro Castellani [:aleca]]

Comment on attachment 9154333 [details]
Bug 306495 - Implement a new calendar creation dialog. r=darktrojan

Marking this ui-r+ to match the comments left on Phabricator

Comment 84

[Paul Morris [:pmorris]]

Thanks aleca, I've made those changes. I've rebased on top of bug 1650925, and everything is still working as expected. I haven't uploaded the latest versions to phab yet since it would add that extra patch as well. Better to wait for it to land first.

Since there's more to do on the 'extensibility' patch it may make sense to go ahead with landing the others and then finalize the extensibility patch afterwards. So here's a try run without the extensibility patch: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eed1767333148c2e719cc3a1085c0494d5bf653a

Comment 85

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Improve provider preference handling for calendar detection. r=Fallen

Depends on D81005

Comment 86

[Paul Morris [:pmorris]]

All patches have now been reviewed, and the review comments addressed. Here's a try run, recently started:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=142d216fa26fe1018bf20c08c55f3df780b5367b

Comment 87

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Fix the browser_calendarList.js mochitest. r=darktrojan

Depends on D83567

Comment 88

[Paul Morris [:pmorris]]

Here's a new try run with a new patch added to fix the browser_calendarList test:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2213c6a6c73510c0501a5d05f61d7132614d1057
The two failing windows tests are a mystery. I couldn't figure them out by looking at the try logs and don't have a windows machine handy at the moment to try and fix them. Maybe we could go ahead and land this and file a bug for those tests and fix them in a follow-up? (Once the new patch is reviewed.)

Comment 89

[Paul Morris [:pmorris]]

Made some changes Geoff suggested in code review. Here's a new try run, which looks good except for the same windows failures. I think we should fix them in a follow-up so we can go ahead and land these changes, since I don't have access to a windows machine at the moment.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3ee4653181395856ceef9c61dad84885ee5a6d88

Comment 90

[Paul Morris [:pmorris]]

I'm happy to do the landing on this, since it's several patches.

Comment 91

[Magnus Melin [:mkmelin]]

You shouldn't land it with known failures. For that you'd at least have to add a patch to disable those tests, get review, then leave this bug open to fix them.

From the logs, I'd guess the problem is the (right) window doesn't have/get focus as expected, so the bowser_basicFunctionality.js test doesn't run.

Comment 92

[Paul Morris [:pmorris]]

Attachment: Bug 306495 - Remove mozmill code from browser_basicFunctionality.js test. r=darktrojan

This test was failing on Windows for some reason, so remove
the mozmill parts.

Depends on D85620

Comment 93

[Paul Morris [:pmorris]]

Here is the latest try run. The windows failure for browser_basicFunctionality.js is still there, after rewriting the test in mochitest style and adding a line to focus the window. I don't have access to a windows machine to troubleshoot it locally. Not sure what's causing the problem. Geoff, do you have any ideas?
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d154e94472ad5039a8b55cf04afdbe4fc5d4285b
Log where the failure is happening:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312606987&repo=try-comm-central&lineNumber=2711-2720

Comment 94

[Geoff Lankow (:darktrojan)]

Try putting Services.focus.focusedWindow = window; in the cleanup function. You want it to happen after the test does its thing, not before it. No other great ideas I'm afraid.

Comment 95

[Paul Morris [:pmorris]]

(In reply to Geoff Lankow (:darktrojan) from comment #94)

Try putting Services.focus.focusedWindow = window; in the cleanup function. You want it to happen after the test does its thing, not before it. No other great ideas I'm afraid.

I gave that a try but got the same failure: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d93d1a8e84b199affc5baebfe3ef0a58ac1be4cc&selectedTaskRun=bJ8b5rt2Tt-SYvtTlhs0zA.0

Comment 96

[Paul Morris [:pmorris]]

All patches are reviewed, and per Geoff's suggestion I've disabled the test that was failing on windows for now (only disabled on windows). So this is ready to land.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aef5aa0cd418bb690b952ca5bcd57de503526114

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55ca0c018533
Make changes to openLocalCalendar function. r=darktrojan
https://hg.mozilla.org/comm-central/rev/3267048c6fc8
Move openLocalCalendar out of calendar-creation.js. r=darktrojan
https://hg.mozilla.org/comm-central/rev/e02191ac356d
Implement a new calendar creation dialog. r=darktrojan
https://hg.mozilla.org/comm-central/rev/2b27e47b6985
Make the calendar creation dialog more extensible. r=Fallen
https://hg.mozilla.org/comm-central/rev/39c6f1c56fbd
Improve provider preference handling for calendar detection. r=Fallen
https://hg.mozilla.org/comm-central/rev/77ce4d218966
Fix the browser_calendarList.js mochitest. r=darktrojan
https://hg.mozilla.org/comm-central/rev/437c9b55e071
Remove mozmill code from browser_basicFunctionality.js test. r=darktrojan
https://hg.mozilla.org/comm-central/rev/2dff15f0c150
Improve error handling in calendar detection code. r=darktrojan,Fallen
https://hg.mozilla.org/comm-central/rev/d6c3a697d63c
Convert Autodetect module to cal.provider.detection. r=darktrojan

Comment 98

[Magnus Melin [:mkmelin]]

Please if there are many phab patches in the bug make sure they are all stacked up. Otherwise it's a bit of work to find out how to get them all landed at once.

Comment 99

[Paul Morris [:pmorris]]

Glad to help with this. Can you say more about what you mean by "all stacked up"? Do you mean just putting "part 1", "part 2" etc. in the commit messages?

Comment 100

[Magnus Melin [:mkmelin]]

I think the last 3 patches (of the landed) were not connected to the series. Didn't check the details, but I guess they didn't (all) have a parent changeset in the patches to land and were thus independent?
-> lando as well as using moz-phab to grab the patches would only automatically want to land 6 of the patches

Comment 101

[Paul Morris [:pmorris]]

Huh, they are all in a series for me locally. When I do hg log I see a parent listed for only the first commit. Maybe there's something I need to do when running moz-phab.

Comment 102

[Rob Lemley [:rjl]]

Comment on attachment 9154330 [details]
Bug 306495 - Make changes to openLocalCalendar function. r=darktrojan

[Triage Comment]
For all commits in comment 97. Approved by pmorris via Matrix.

https://hg.mozilla.org/comm-central/rev/55ca0c018533
https://hg.mozilla.org/comm-central/rev/3267048c6fc8
https://hg.mozilla.org/comm-central/rev/e02191ac356d
https://hg.mozilla.org/comm-central/rev/2b27e47b6985
https://hg.mozilla.org/comm-central/rev/39c6f1c56fbd
https://hg.mozilla.org/comm-central/rev/77ce4d218966
https://hg.mozilla.org/comm-central/rev/437c9b55e071
https://hg.mozilla.org/comm-central/rev/2dff15f0c150
https://hg.mozilla.org/comm-central/rev/d6c3a697d63c

Comment 103

[Rob Lemley [:rjl]]

Thunderbird 81.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/9b162bc01ae6
https://hg.mozilla.org/releases/comm-beta/rev/165fcb63f9c3
https://hg.mozilla.org/releases/comm-beta/rev/00761c3607c3
https://hg.mozilla.org/releases/comm-beta/rev/c8c85671d457
https://hg.mozilla.org/releases/comm-beta/rev/ee4ed0b36e84
https://hg.mozilla.org/releases/comm-beta/rev/7f0999d4a9bd
https://hg.mozilla.org/releases/comm-beta/rev/a306df9065ca
https://hg.mozilla.org/releases/comm-beta/rev/df7e75ecf827
https://hg.mozilla.org/releases/comm-beta/rev/08c5698909b3

Comment 104

[Magnus Melin [:mkmelin]]

I don't think we'll be able to uplift to 78. Too many loose ends.