Closed Bug 1356300 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

52 Branch
defect
Not set

Tracking

(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 53+ fixed
thunderbird53 --- fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

(Keywords: regression)

Attachments

(4 files, 12 obsolete files)

146.47 KB, image/jpeg
Details
10.35 KB, image/gif
Details
11.38 KB, patch
jorgk
: review+
aceman
: review+
Details | Diff | Splinter Review
4.01 KB, patch
jorgk
: review+
aceman
: feedback+
Details | Diff | Splinter Review
+++ 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.
Keywords: regression
Component: Untriaged → Message Compose Window
Depends on what you call a problem ;)
Sorry Magnus, I'm managing the release and I am tracking this. You can help with a review.
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8858116 - Flags: review?(mkmelin+mozilla)
Attachment #8858116 - Flags: review?(acelists)
(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 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.
Attachment #8858116 - Flags: review?(acelists) → feedback+
Or make some new shared file similar to EdImageOverlay.js that gets included in image handling dialogs.
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.
Blocks: 1356600
No longer blocks: 1356600
(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.
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.
Attachment #8858116 - Attachment is obsolete: true
Attachment #8858116 - Flags: review?(mkmelin+mozilla)
Attachment #8858350 - Flags: feedback?(acelists)
Blocks: 1322155
No longer depends on: 1322155
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.
Attachment #8858350 - Flags: feedback?(acelists) → feedback+
Incorporating Aceman's suggestions. I guess interdiff should show them.
Attachment #8858350 - Attachment is obsolete: true
Attachment #8858426 - Flags: review?(acelists)
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?
Good catch!
Should be
  gBackgroundImage = gDialog.BackgroundImageInput.fullDataURI;
Check interdiff.
Attachment #8858426 - Attachment is obsolete: true
Attachment #8858426 - Flags: review?(acelists)
Attachment #8858450 - Flags: review?(acelists)
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?
Attachment #8858452 - Flags: feedback?(jorgk)
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?
Attachment #8858452 - Flags: feedback?(jorgk) → feedback+
(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:)
Blocks: 1355913
No longer depends on: 1355913
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.
Attachment #8858450 - Flags: review?(acelists) → review+
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.
Attachment #8858450 - Attachment is obsolete: true
Attachment #8858452 - Attachment is obsolete: true
Attachment #8858508 - Flags: review+
Attached patch tooltip fixes (trunk only) (obsolete) — Splinter Review
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.
Attachment #8858509 - Flags: review?(jorgk)
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.
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- :-(
I'll fix it, give me 5 minutes.
Attached patch 1356300-TB52.patch (v7). (obsolete) — Splinter Review
Like this, right?
Attachment #8858508 - Attachment is obsolete: true
Attachment #8858513 - Flags: review+
Attachment #8858513 - Flags: feedback?(acelists)
I've done want I wanted here. I removed the tooltip from the label, no idea why you added that. Try interdiff.
Attachment #8858509 - Attachment is obsolete: true
Attachment #8858509 - Flags: review?(jorgk)
Attachment #8858515 - Flags: review+
Attachment #8858515 - Flags: feedback?(acelists)
I noticed that the labels all have tooltips, so I restored it. Sorry.
Attachment #8858515 - Attachment is obsolete: true
Attachment #8858515 - Flags: feedback?(acelists)
Attachment #8858516 - Flags: review+
Attachment #8858516 - Flags: feedback?(acelists)
(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 :)
Attachment #8858516 - Flags: feedback?(acelists) → feedback+
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.
Attachment #8858513 - Flags: feedback?(acelists) → feedback+
I suggest:
aDialogField.value = aImageData.replace(/^(data:.+;base64,)(.*)/i, ...
and not return anything. Do we need the return value?
Attached patch 1356300-TB52.patch (v8). (obsolete) — Splinter Review
This should do it for TB 52.
Attachment #8858513 - Attachment is obsolete: true
Attachment #8858526 - Flags: review+
Aceman, can you please test v8+v4 before I land them.
Attachment #8858516 - Attachment is obsolete: true
Attachment #8858527 - Flags: review+
Now with length checking.
Attachment #8858526 - Attachment is obsolete: true
Attachment #8858529 - Flags: review+
Attachment #8858527 - Attachment is obsolete: true
Attachment #8858530 - Flags: review+
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.
Attachment #8858529 - Flags: review+
Comment on attachment 8858530 [details] [diff] [review]
1356300-trunk.patch (v5) - tooltip fixes (trunk only)

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

Thanks.
Attachment #8858530 - Flags: feedback+
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;
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Attachment #8858529 - Flags: approval-comm-esr52?
Attachment #8858529 - Flags: approval-comm-aurora+
Attachment #8858529 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.