901332 - Google CalDAV OAuth2 authentication dialog not shown in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Stefan Sitter [:ssitter]]

Lightning 2.7a1 + SeaMonkey 2.22a1

STR:
1. connect to Google Calendar using the new CalDAV endpoint 
   https://apidata.googleusercontent.com/caldav/v2/{calid}/events

Actual:
Nothing happens after closing the new calendar wizard. Google CalDAV OAuth2 authentication dialog is not shown. No error is shown in Console.

Comment 1

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

Hey Stefan, sorry I didn't see this bug a month ago.

Comment 2

[Stefan Sitter [:ssitter]]

I'm note sure this is a duplicated of Bug 919018. 

In Bug 919018 CalDAV is reported broken because the http.jsm module is missing in SeaMonkey 2.21. In this report I used SeaMonkey 2.22 that was build several weeks after Bug 884319	
was fixed and that should contain the http.jsm module. In addition I did not saw any error messages as written in comment #0.

More testing and feedback from SeaMonkey users would be nice.

Comment 3

[Iacopo Benesperi [:iacchi]]

This is still not working in SeaMonkey 2.22. The OAuth window doesn't appear.

Comment 4

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

Just to be sure, is this specifically an issue with 2.22, or is it an ongoing issue with the latest releases and nightly Seamonkey builds?

Comment 5

[Iacopo Benesperi [:iacchi]]

Ongoing

Comment 6

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

Timestamp: 4/1/14, 11:26:07 PM
Error: Error: Permission denied for <moz-safe-about:neterror?e=fileNotFound&u=chrome%3A//messenger/content/browserRequest.xul&c=UTF-8&f=regular&d=The%20file%20chrome%3A//messenger/content/browserRequest.xul%20cannot%20be%20found.%20Please%20check%20the%20location%20and%20try%20again.> to create wrapper for object of class UnnamedClass

Comment 7

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

Attachment: Fix - v1 [check-in comment 13][c-a,c-b check-in comment 16]

Seamonkey is missing browserRequest.* and its artwork. I've tested this by manually injecting the files into a Seamonkey nightly and I must admit this patch is totally untested. Nevertheless, I hope I've copied all the files into the right locations.

I'd appreciate if you could give this patch a test run.

Comment 8

[Philip Chee]

Comment on attachment 8400232 [details] [diff] [review]
Fix - v1 [check-in comment 13][c-a,c-b check-in comment 16]

>     const wpl_security_bits = wpl.STATE_IS_SECURE |
>                               wpl.STATE_IS_BROKEN |
>                               wpl.STATE_IS_INSECURE |
>                               wpl.STATE_SECURE_HIGH |
>                               wpl.STATE_SECURE_MED |
>                               wpl.STATE_SECURE_LOW;
>     var browser = document.getElementById("requestFrame");
>     var level;

>     switch (aState & wpl_security_bits) {
>       case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH:
>         level = "high";
>         break;
>       case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_MED:
>       case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_LOW:
>         level = "low";
>         break;
>       case wpl.STATE_IS_BROKEN:
>         level = "broken";
>         break;

> #security-button[level="high"],
> #security-button[level="low"] {
>   background-image: url("chrome://messenger/skin/icons/secure.png");
> }

Your levels high and low are identical in the UI. You should simplify this like (for example) https://hg.mozilla.org/comm-central/rev/24ad7887d181#l2.7
See Bug 810671.

>     this.securityButton.setAttribute("tooltiptext",
>                                      browser.securityUI.tooltipText);
tooltipText property has been removed. See Bug 832848.

> function loadRequestedUrl()
> {
>   let request = window.arguments[0].wrappedJSObject;
>   document.getElementById("headerMessage").textContent = request.promptText;

I think that the promptText should go to the window title, because several lines further down you set the headerMessage to the url.

>   let account = request.account;
>   if (request.iconURI != "")

if (request.iconURI)

>     document.getElementById("headerImage").src = request.iconURI;
"headerImage" doesn't exist. Looking at the chat version of this file I think you need "security-button". Actually you don't since it's so overloaded.

>   var url = request.url;
>   if (url != "") {
if (url)

>     <image id="security-button" src="chrome://branding/content/icon32.png"/>
Don't hard code paths to theme images. Theme authors really really hate this. You already have browserRequest.css so should move this into CSS.

> #security-button {
  list-style-image: url("chrome://branding/content/icon32.png")
> }

> #security-button[level="high"],
> #security-button[level="low"] {
>   background-image: url("chrome://messenger/skin/icons/secure.png");
> }
This is XUL so please use list-style-image. With list-style-image you can get rid of background-repeat and background-position.

> #security-button[level="broken"] {
>   background-image: url("chrome://messenger/skin/icons/insecure.png");
> }

> #security-button[loading="true"] {
>   background-image: url("chrome://messenger/skin/icons/loading.png");
>   background-position: 4px 2px;
> }

So you start off with a 32x32px icon. Then switch to a 16x16px spinner and then your end state is a 22x22px secure/insecure image. Actually the latter is 18x18px with a 2px margin built in. A really horrendous bit of coding here...

>     <image id="security-button" src="chrome://branding/content/icon32.png"/>
Also because the image src is used. At the end point of the procedure I see a SeaMonkey 32px icon on top of the 22px secure icon. I don't understand how the Thunderbird reviewers missed this.

> +  skin/classic/messenger/icons/[e-mail]                           (mac/messenger/icons/[e-mail])
I think if we want to support retina displays we should have 2x for all of loading/secure/insecure/broken or none at all.

>   background-image: url("chrome://messenger/skin/icons/secure.png");
>   background-image: url("chrome://messenger/skin/icons/insecure.png");

> +  skin/classic/messenger/icons/insecure.png                             (messenger/icons/insecure.png)
> +  skin/classic/messenger/icons/loading.png                              (messenger/icons/loading.png) 

I see some 16x16px images that we could use instead in /chat/themes/ available-16.png and away-16.png.

> /*
> #header {
>   overflow: hidden;
>   padding: 5px;
>   border-bottom: 1px solid black;
>   font-weight: bold;
>   font-size: 1.2em;
> }
> */
This whole comment should be removed.

> diff --git a/mail/themes/linux/mail/browserRequest.css b/suite/themes/classic/messenger/browserRequest.css
> copy from mail/themes/linux/mail/browserRequest.css
> copy to suite/themes/classic/messenger/browserRequest.css
> diff --git a/mail/themes/linux/mail/icons/insecure.png b/suite/themes/classic/messenger/icons/insecure.png
> copy from mail/themes/linux/mail/icons/insecure.png
> copy to suite/themes/classic/messenger/icons/insecure.png 
For SeaMonkey Classic, please copy from the winstripe versions instead.

Comment 9

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

I understand that the files copied from Thunderbird may not be in good shape, but I really just created this patch as a quick copy from TB to SM so that users are happy. I didn't expect this to be a full cleanup, which would mean clean up in TB, SM and IB together. Is there any chance you'd r+ the patch with just the Seamonkey related bits fixed, i.e copying the icons from winstripe instead?

Comment 10

[Philip Chee]

Comment on attachment 8400232 [details] [diff] [review]
Fix - v1 [check-in comment 13][c-a,c-b check-in comment 16]

(In reply to Philipp Kewisch [:Fallen] from comment #9)
> I understand that the files copied from Thunderbird may not be in good
> shape, but I really just created this patch as a quick copy from TB to SM so
> that users are happy. I didn't expect this to be a full cleanup, which would
> mean clean up in TB, SM and IB together.

Sorry for getting carried away. r=me for landing in suite on condition you file a followup bug to fix the issues detailed Comment 8. I'll probably work on getting it into shape and then moved to shared /mailnews/

> patch with just the Seamonkey related bits fixed, i.e copying the icons from
> winstripe instead?
Sure.

Comment 11

[Philip Chee]

> Sorry for getting carried away. r=me for landing in suite on condition you
> file a followup bug to fix the issues detailed Comment 8. I'll probably work
> on getting it into shape and then moved to shared /mailnews/
I guess I can reuse this bug for follow-up patches after you get ui-r from Neil().

Comment 12

[[e-mail]]

Comment on attachment 8400232 [details] [diff] [review]
Fix - v1 [check-in comment 13][c-a,c-b check-in comment 16]

Well, that was awful.

First, I accidentally created the calendar while I was offline. Nothing happened. I couldn't find out how to access the calendar after creating it, nor could I delete the calendar, so I resorted to deleting all calendar prefs from prefs.js just so that I could recreate the calendar so as to trigger the authentication.

The window that was opened wasn't sure whether it was supposed to be a dialog. It had the title bar height of a regular window, but the lack of window features of a dialog. It was also always on top of other windows from the same application, so I couldn't easily switch to, say, DOM Inspector. It also had no title.

The window included something that seemed to want to resemble a URL bar, but it had a strange icon and was poorly themed. DOM Inspector showed that the URL wanted to wrap although its overflow had been hidden not once, but twice.

Comment 13

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/e07e871681ab

Comment 14

[Philip Chee]

Comment on attachment 8400232 [details] [diff] [review]
Fix - v1 [check-in comment 13][c-a,c-b check-in comment 16]

[Approval Request Comment]
Regression caused by (bug #): bug 853236 - Provide access to Google CalDAV v2 API
User impact if declined: Lightning users won't be able to sync with Google calendar.
Testing completed (on m-c, etc.): comm-central
Risk to taking this patch (and alternatives if risky): low to none.
String changes made by this patch: None.

Comment 15

[Ian Neal]

Comment on attachment 8400232 [details] [diff] [review]
Fix - v1 [check-in comment 13][c-a,c-b check-in comment 16]

a=me

Comment 16

[Philip Chee]

(In reply to Ian Neal from comment #15)
> Comment on attachment 8400232 [details] [diff] [review]
> a=me

Pushed to comm-aurora: changeset b6127809aa04
http://hg.mozilla.org/releases/comm-aurora/rev/b6127809aa04
Pushed to comm-beta: changeset fea21f2a0ab4
http://hg.mozilla.org/releases/comm-beta/rev/fea21f2a0ab4

Comment 17

[Philip Chee]

Yargh. I think I landed the patch on the wrong branch in comm-beta. Running graft now.
comm-beta: graft to default/tip
http://hg.mozilla.org/releases/comm-beta/rev/43d59f510a7e
http://hg.mozilla.org/releases/comm-beta/graph/43d59f510a7e

Comment 18

[[e-mail]]

Ah, hg copy, that would explain why the tooltipText code still exists after it was removed in Gecko 24...