1356300 - When a background image is inserted, resulting data: URI is not shortened

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1355913 +++

When a background image is inserted, resulting data: URI is not shortened.

This can lead to problems when large images are inserted.

Comment 1

[Magnus Melin [:mkmelin]]

Depends on what you call a problem ;)

Comment 2

[Jorg K (CEST = GMT+2)]

Sorry Magnus, I'm managing the release and I am tracking this. You can help with a review.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-background-images.patch (v1).

Very simple cut/paste port from EdImageOverlay.js where this was done before.

To test:
Add a background image. OK.
Open |Format > Page Colour and Background| again.
See the shortened data: URL.
Copy that to a text editor, it will be long.
Now go OK on the panel to run through ValidateAndPreviewImage().

I've put Aceman as reviewer in case Magnus is too busy. He reviewed the changes in EdImageOverlay.js.

Re:
> Depends on what you call a problem ;)
Try with a 10 MB picture and you'll see what I mean, seriously!

All the stuff we did we've tested on Mickey Mouse data and now users are pounding it and it falls apart.

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #3)
> > Depends on what you call a problem ;)
> Try with a 10 MB picture and you'll see what I mean, seriously!

But it's not a real world problem. Many servers won't even send/receive messages that large. Having that as bg is just not reasonable for anyone.

Comment 5

[Anje]

Perhaps the info below is related to this bug.
Note the size of image is small.
Old template created several versions ago worked until version 52.0
now when you open it no background image shown.
Templates folder is for a pop mail account.

Decided to recreate.
click on 'Write'
'Format' > 'Page colours and backgrounds'
click on 'choose file'
select 10.3 KB gif image - white background with top rose border size 407 x 766
click on 'Open'
image shows correct
click on 'OK'
nothing happens.
No background image in 'Write' message.
'Page colours and backgrounds' window remains open as if I had not clicked on 'OK'.
Cannot proceed, so have to close 'Page colours and backgrounds' window.

Tested in Thunderbird Safe mode with same results.
Windows Vista
Thunderbird version 52.0

Comment 6

[Anje]

Attachment: template background not.jpg

Image showing issue with adding a background image.

Comment 7

[:aceman]

Anje, I don't think your problem relates to the bug here.

Comment 8

[Jorg K (CEST = GMT+2)]

We're in the wrong bug here. This bug is about showing the "base64 jibber" (as you call it) in a shortened form.

You are saying that your "rosechain_inline.gif" doesn't work. To check that, please attach that image here and I'll open a new bug for you. I tried a GIF background and it worked.

Comment 9

[:aceman]

Comment on attachment 8858116 [details] [diff] [review]
1356300-background-images.patch (v1).

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

Thanks, seems to work, but please find a way to share all this code. At worst you can put it in editorUtilities.js that is included in all the dialogs.

Comment 10

[:aceman]

Or make some new shared file similar to EdImageOverlay.js that gets included in image handling dialogs.

Comment 11

[Jorg K (CEST = GMT+2)]

Which code do you want to share? Note that field is called gDialog.BackgroundImageInput and for normal images it's gDialog.srcInput. The only snipped that could be shared is onCopyOrCut(). You could get the field name via event.target, so
  let field = event.target;
  let startPos = field.selectionStart;
It's very un-elegant to have the global 'gFullDataURI' in the function, which, since it's an event listener, can't be passed in. So you'd have 'gFullDataURI' defined in EdImageOverlay.js and EdColorProps.js and accessed somewhere else.

I think you're driving purism to far. If you don't want to approve it, I'll wait for Magnus.

Comment 12

[Anje]

Attachment: Rosechain_inline plus.gif

rose chain image gif

Comment 13

[:aceman]

(In reply to Jorg K (GMT+2) from comment #11)
> Which code do you want to share? Note that field is called
> gDialog.BackgroundImageInput and for normal images it's gDialog.srcInput.

So what?  gDialog.BackgroundImageInput.value = shortenedImageData(gBackgroundImage, gDialog.BackgroundImageInput);

And similarly for the "// Restore shortened URIs from the original."-block.

> The only snipped that could be shared is onCopyOrCut(). You could get the
> field name via event.target, so
>   let field = event.target;
>   let startPos = field.selectionStart;
> It's very un-elegant to have the global 'gFullDataURI' in the function,
> which, since it's an event listener, can't be passed in. So you'd have
> 'gFullDataURI' defined in EdImageOverlay.js and EdColorProps.js and accessed
> somewhere else.

The gFullDataURI could also be defined once in the file having the onCopyOrCut() and the shortenedImageData() and the third function. Then you have uses of the variable contained in a single file.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-background-images.patch (v2).

Purist version ;-)

I don't think I need the 'gListenerAttached'. The listener gets attached on init when the dialogue is first put up and dies with the panel.

Comment 15

[:aceman]

Comment on attachment 8858350 [details] [diff] [review]
1356300-background-images.patch (v2).

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

::: editor/ui/composer/content/editorUtilities.js
@@ +982,5 @@
> +  return aImageData.replace(/(data:.+;base64,)(.*)/i,
> +    function(match, nonDataPart, dataPart) {
> +      aDialogField.addEventListener("copy", onCopyOrCut);
> +      aDialogField.addEventListener("cut", onCopyOrCut);
> +      gFullDataURI = aImageData;

May there be multiple image elements and fields in a dialog? Then a single global var will not work. What about storing this in aDialogField.fullDataURI ?

::: editor/ui/dialogs/content/EdColorProps.js
@@ +346,5 @@
> +      image = gFullDataURI;
> +    }
> +    else
> +    {
> +      gBackgroundImage = image;

So would there be a way to also put this handling of gFullDataURI into a shared function too? So that all the usage of the var is in editorUtilities.js as you wanted.

::: editor/ui/dialogs/content/EdImageOverlay.js
@@ -112,5 @@
> -          gDialog.srcInput.addEventListener("copy", onCopyOrCut);
> -          gDialog.srcInput.addEventListener("cut", onCopyOrCut);
> -          gListenerAttached = true;
> -        }
> -        gDialog.srcInput.setAttribute("tooltip", "shortenedDataURI");

This line got lost from the new shortenedImageData(). But it probably belongs here so just restore it after the gDialog.srcInput.value = shortenedImageData() call.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-background-images.patch (v3).

Incorporating Aceman's suggestions. I guess interdiff should show them.

Comment 17

[:aceman]

Comment on attachment 8858426 [details] [diff] [review]
1356300-background-images.patch (v3).

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

::: editor/ui/dialogs/content/EdColorProps.js
@@ +342,5 @@
>    {
> +    if (/^data:/i.test(image) && image.includes("…"))
> +    {
> +      // Restore shortened URIs from the original.
> +      image = gDialog.BackgroundImageInput.fullDataURI;

After the patch the value of 'image' image is never used. Is that correct? Shouldn't we run the following code with it?

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-background-images.patch (v4).

Good catch!
Should be
  gBackgroundImage = gDialog.BackgroundImageInput.fullDataURI;
Check interdiff.

Comment 19

[:aceman]

Attachment: 1356300-background-images.patch (v5)

This is my proposal with your v4 and adding a new function that encapsulates retrieving the .fullDataURI.
It also merges handling the tooltip of the field in both dialogs.

What do you think?

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8858452 [details] [diff] [review]
1356300-background-images.patch (v5)

Fine, I suspected that the real purist could get it even more purist ;-)
(I hope you haven't forgotten that this bug is the most unimportant of the five regressions we're fixing here. And in three days is branch day.)

Remember that we're fixing TB 52, so no string changes. So you need to undo the .xul and .dtd and move the tooltip processing out of the purist's function.

If you insist, make it two patches, one for all versions and another one to be applied on top to do the tooltip stuff.

How does that sound?

Comment 21

[:aceman]

(In reply to Jorg K (GMT+2) from comment #20)
> Fine, I suspected that the real purist could get it even more purist ;-)
> (I hope you haven't forgotten that this bug is the most unimportant of the
> five regressions we're fixing here. And in three days is branch day.)

Yes, I look at all of them in parallel and will be available to you for the next 3 days :)
 
> Remember that we're fixing TB 52, so no string changes. So you need to undo
> the .xul and .dtd and move the tooltip processing out of the purist's
> function.

Ok.
 
> If you insist, make it two patches, one for all versions and another one to
> be applied on top to do the tooltip stuff.
> 
> How does that sound?

Good deal:)

Comment 22

[:aceman]

Comment on attachment 8858450 [details] [diff] [review]
1356300-background-images.patch (v4).

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

This is fine and I build on top of it in the next patch.

Comment 23

[:aceman]

Attachment: 1356300-background-images.patch (v6)

The is the mean of the patch as accepted by jorg and without any string changes. This is to be landed on trunk and branches.

Comment 24

[:aceman]

Attachment: tooltip fixes (trunk only)

This makes tooltips in the URL textboxes showing the shortened strings behave the same (initial tooltip is there, once a shortened URL is in the box, other tooltip is shown). This has string change so is only for trunk.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8858509 [details] [diff] [review]
tooltip fixes (trunk only)

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

You forgot to remove
gDialog.srcInput.setAttribute("tooltip", "shortenedDataURI");
from EdImageOverlay.js

::: editor/ui/dialogs/content/EdColorProps.xul
@@ +103,5 @@
>    <textbox id="BackgroundImageInput" class="uri-element" oninput="ChangeBackgroundImage()"
> +           tooltiptext="&backgroundImage.tooltip;" flex="1"/>
> +  <tooltip id="shortenedDataURI">
> +    <label value="&backgroundImage.shortenedDataURI;"/>
> +  </tooltip>

Nit: Can you please move the tooltip before the textbox to make it consistent with EdImageOverlay.xul.

Comment 26

[Jorg K (CEST = GMT+2)]

I think your purification has broken the thing in the middle. Reading the code, adding a picture from a http:// source won't work any more.

Here's why:
Original code:
gDialog.srcInput.value = src.replace(/(...),
If there was nothing to replace that come down to
gDialog.srcInput.value = src;

That doesn't execute any more since the
aDialogField.value = shortURI;
is only set in the match function.

So insert an image from http://www.jorgk.com/images/coflag.png, OK, then edit it again. Result: Location empty.

So this should be r- :-(

Comment 27

[Jorg K (CEST = GMT+2)]

I'll fix it, give me 5 minutes.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-TB52.patch (v7).

Like this, right?

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-trunk.patch (v2) - tooltip fixes (trunk only)

I've done want I wanted here. I removed the tooltip from the label, no idea why you added that. Try interdiff.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-trunk.patch (v3) - tooltip fixes (trunk only)

I noticed that the labels all have tooltips, so I restored it. Sorry.

Comment 31

[:aceman]

(In reply to Jorg K (GMT+2) from comment #25)
> You forgot to remove
> gDialog.srcInput.setAttribute("tooltip", "shortenedDataURI");
> from EdImageOverlay.js

Why? It was there before the patch I just kept it there (see comment 15). In the "tooltip" patch I move it to the shared function.

> ::: editor/ui/dialogs/content/EdColorProps.xul
> Nit: Can you please move the tooltip before the textbox to make it
> consistent with EdImageOverlay.xul.

OK.

(In reply to Jorg K (GMT+2) from comment #29)
> I've done want I wanted here. I removed the tooltip from the label, no idea
> why you added that. Try interdiff.

In one dialog the tooltip was on the label in another it was on the textbox. So I put it onto both, and only one of them is replaced by the "shortened URI" tooltip later :)

Comment 32

[:aceman]

Comment on attachment 8858513 [details] [diff] [review]
1356300-TB52.patch (v7).

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

Yeah, I understand.

You could still set aDialogField.value as the last line in shortenImageData(), outside of the match, but I'm fine with this version too. Thanks for catching it.

Comment 33

[Jorg K (CEST = GMT+2)]

I suggest:
aDialogField.value = aImageData.replace(/^(data:.+;base64,)(.*)/i, ...
and not return anything. Do we need the return value?

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-TB52.patch (v8).

This should do it for TB 52.

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-trunk.patch (v4) - tooltip fixes (trunk only)

Aceman, can you please test v8+v4 before I land them.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-TB52.patch (v9).

Now with length checking.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1356300-trunk.patch (v5) - tooltip fixes (trunk only)

Comment 38

[:aceman]

Comment on attachment 8858529 [details] [diff] [review]
1356300-TB52.patch (v9).

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

Thanks, tested and still works for me :)

::: editor/ui/composer/content/editorUtilities.js
@@ +990,5 @@
> +function shortenImageData(aImageData, aDialogField) {
> +  let shortened = false;
> +  aDialogField.value = aImageData.replace(/^(data:.+;base64,)(.*)/i,
> +    function(match, nonDataPart, dataPart) {
> +      if (dataPart.length > 35) {

I wrote (dataPart.length <= 35) return match;, which could be more readable, but push which version you like better.

Comment 39

[:aceman]

Comment on attachment 8858530 [details] [diff] [review]
1356300-trunk.patch (v5) - tooltip fixes (trunk only)

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

Thanks.

Comment 40

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/75a9da1c2f8430366cd98e40f63eef226a7f905e - TB 52/Aurora version
https://hg.mozilla.org/comm-central/rev/9461b19dd9e3c9de96524ee70d02df5c292d2d30 - string changes for trunk only.

Landed with minor changes:
- renamed function to onCopyOrCutShortened()
- if (dataPart.length <= 35) return match;

Comment 41

[Jorg K (CEST = GMT+2)]

Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/d8949acb05a315407e2b06cc78600f3590c8393a

Comment 42

[Jorg K (CEST = GMT+2)]

Beta (TB 53):
https://hg.mozilla.org/releases/comm-beta/rev/9930396291feb3466f0ab68a1d8a3efa10b92928

Comment 43

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/53e418ac5daaed871851d8eb4acf14a00ba50d42