Dragging a photo onto an address book card doesn't work any more in TB 65 and trunk

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

({regression})

Trunk
Thunderbird 66.0

Thunderbird Tracking Flags

(thunderbird65 fixed, thunderbird66 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 months ago

STR: Open a contact to Photo tab. Click OK.

Result: Not working.

Working fine in TB 60 ESR.

Aceman, your baby broke here :-(

Alice, could you please find the regression.

Flags: needinfo?(alice0775)
Flags: needinfo?(acelists)

Comment 1

4 months ago

STR: Open a contact to Photo tab. Click OK.

Can you please describe more detailed steps?

Flags: needinfo?(alice0775)
(Assignee)

Comment 2

4 months ago

OK. Open the address book. Double-click a contact. Open the Photo tab. Drag a (small) photo from the desktop to the photo area.

Comment 4

4 months ago

(In reply to Alice0775 White from comment #3)

Regression window:
https://hg.mozilla.org/comm-central/
pushloghtml?fromchange=6731dac68a6610d4e81447580c225ba8d63cbc97&tochange=6731
dac68a6610d4e81447580c225ba8d63cbc97
https://hg.mozilla.org/mozilla-central/
pushloghtml?fromchange=987ea0d6a000b95cf93928b25a74a7fb1dfe37b2&tochange=cc34
01e78e8bbae22e6dbc854e525ceae4923bcf

Via local build,
Last good: c-c 6731dac68a66, m-c 3d7f2fdc5bf7
First bad: c-c 6731dac68a66, m-c 9a2b02fe351b

Regressed by: 9a2b02fe351b Gijs Kruitbosch — Bug 1469916, r=ckerschb,jkt

(Assignee)

Comment 5

4 months ago

Thank you, Alice, very helpful!

Looks like https://hg.mozilla.org/mozilla-central/rev/9a2b02fe351b affected our ability to drag photos onto address book cards. I believe you're a TB user, so you might know this function. We implemented it in bug 892889:
https://hg.mozilla.org/comm-central/rev/7aa7b9e29d85

Looks like saveURI() takes a principal now:
https://hg.mozilla.org/mozilla-central/rev/9a2b02fe351b#l18.72

Flags: needinfo?(acelists)
(Assignee)

Comment 6

4 months ago

Oops, I was going to address the previous comment to Gijs, so please ignore the "I believe you're a TB user, so you might know this function.".

(Assignee)

Comment 7

4 months ago
Posted patch 1519732-saveURI.patch - WIP (obsolete) — Splinter Review

Hmm, I thought the attached patch would fix it, but it doesn't. It gets me a little further, but now the operation hangs with a progress bar "Saving the image" that never completes.

Hi Gijs,
I believe you're a TB user, so you might know this function.

What else do we need to do to restore the functionality, details in comment #5.

Assignee: nobody → jorgk
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9036265 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Comment 8

4 months ago

Following
https://hg.mozilla.org/mozilla-central/diff/9a2b02fe351b/toolkit/mozapps/extensions/LightweightThemeManager.jsm#l1.13
we should use Services.scriptSecurityManager.createCodebasePrincipal(source, {});
but that makes no difference.

Comment 9

4 months ago
Comment on attachment 9036265 [details] [diff] [review]
1519732-saveURI.patch - WIP

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

I'd start by actually echo'ing whatever error you get here.
Attachment #9036265 - Flags: feedback?(gijskruitbosch+bugs)

Comment 10

4 months ago

I'm a TB user but I never bother with pictures for email/contacts, and I can't quickly find this functionality. Please provide detailed steps to reproduce.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jorgk)
(Assignee)

Comment 11

4 months ago

Thanks for the comments. More detailed STR:
Open the address book. Double-click a contact. Open the Photo tab. Drag a (small) photo from the desktop to the photo area (see comment #2).

I looked a bit closer now and saw this error:
JavaScript error: chrome://messenger/content/addressbook/abCard.js, line 1137: TypeError: Value being assigned to HTMLProgressElement.value is not a finite floating-point value.

Removing the offending line brings the function back. We'll work out what's wrong with the progress meter there.

Gijs, last question: Using a codebase principal as per comment #8 is better than using the system principal, right?

Attachment #9036265 - Attachment is obsolete: true
Attachment #9036267 - Attachment is obsolete: true
Flags: needinfo?(jorgk)

Comment 12

4 months ago

(In reply to Jorg K (GMT+1) from comment #11)

Gijs, last question: Using a codebase principal as per comment #8 is better than using the system principal, right?

We should avoid using system principal for helper functions, yes. If this is a drag/drop from the filesystem I expect it won't make much difference, you'll get a file: principal for the codebase principal, which is also fairly powerful... Does it not work with the codebase principal?

(Assignee)

Comment 13

4 months ago
Posted patch 1519732-saveURI.patch (v4) (obsolete) — Splinter Review

This restores the function. Debugging showed that undefined was passed to onProgress() and it choked. That's most likely a side-effect of switching from XUL progress to HTML progress.

Yes, Gijs, the codebase principal works.

I think we're done here.

Attachment #9036300 - Attachment is obsolete: true
Attachment #9036302 - Flags: review?(acelists)
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 66.0
(Assignee)

Comment 14

4 months ago

Comment on attachment 9036302 [details] [diff] [review]
1519732-saveURI.patch (v4)

Messing with some flags here, so it shows up on my list of potential uplifts.

Attachment #9036302 - Flags: approval-comm-beta+
(Assignee)

Comment 15

4 months ago
Posted patch 1519732-saveURI.patch (v4b) (obsolete) — Splinter Review

I doesn't need the !! - I got confused with all those falsy values :-(

Attachment #9036302 - Attachment is obsolete: true
Attachment #9036302 - Flags: review?(acelists)
Attachment #9036344 - Flags: review?(acelists)
Attachment #9036344 - Flags: approval-comm-beta+
(Assignee)

Comment 16

4 months ago

Aceman pointed out that the percentage could be zero, so let' use !== undefined.

Attachment #9036344 - Attachment is obsolete: true
Attachment #9036344 - Flags: review?(acelists)
Attachment #9036455 - Flags: review?(acelists)
Attachment #9036455 - Flags: approval-comm-beta+

Comment 17

4 months ago
Comment on attachment 9036455 [details] [diff] [review]
1519732-saveURI.patch (v4c)

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

Thank you, this works for me.
Attachment #9036455 - Flags: review?(acelists) → review+

Comment 18

4 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f9cedb1d7834
fix AB photo drag, nsIWebBrowserPersist.saveURI() takes a principal parameter after bug 1469916. r=aceman

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Keywords: regression
You need to log in before you can comment on or make changes to this bug.