1315440 - Shorten data URIs in user interface (Insert > HTML and image properties dialogue).

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

When dragging an image onto a compose window, a data URI is inserted.

This looks bad in the image properties and the HTML view (Insert HTML):
data:image/bmp;base64,Qk2KQAAAAAA... (goes on forever if the image is large).

Lets apply a few tricks so the user doesn't see them.

Comment 1

[Richard Marti (:Paenglab)]

What about decoding it in the dialog and show the image like we already do in the image properties dialog?

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v1)

This replaces the data URI in the HTML window like this:

data:image/bmp;base64,Qk2KQAAAAAA...
becomes
data:image/bmp;(1).

Try inserting two images, then swap the numbers and the images will be swapped.

This is partly Magnus work from another bug, partly taken from ThunderHTMLedit.

If you like it, I'll do the same replacement in the image properties.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v1a).

Oops, try/catch went missing.

(In reply to Richard Marti (:Paenglab) from comment #1)
> What about decoding it in the dialog and show the image like we already do
> in the image properties dialog?
Sorry, I don't understand: "decoding it in the dialog"?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v1b).

Sorry about the noise. I've made it more generic to detect arbitrary data URIs (not online images) and also in href attributes.

Comment 5

[Jorg K (CEST = GMT+2)]

Correction:
I've made it more generic to detect arbitrary data URIs (not *only* images) and also in href attributes.

To try it out:
Drag two images, go to HTML view. Swap the numbers and accept. The images will be swapped.

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 8807840 [details] [diff] [review]
1315440.patch (v1b).

Looks good. Replacing the shortened data URI with an other full URI in the HTML source works also.

(In reply to Jorg K (GMT+1) from comment #3)
> (In reply to Richard Marti (:Paenglab) from comment #1)
> > What about decoding it in the dialog and show the image like we already do
> > in the image properties dialog?
> Sorry, I don't understand: "decoding it in the dialog"?

Forget comment 1. It's already like I asked.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v2).

OK, this fixes the image properties, too. There I use an ellipsis | …| since there is only one URI, so no number is required.

Sorry, Richard, for the double-review. The HTML dialogue is as it was in the previous patch.

Comment 8

[Jorg K (CEST = GMT+2)]

Damn, typo in the commit message "inteface". I'll fix that together with any review issues.

Comment 9

[:aceman]

Comment on attachment 8807857 [details] [diff] [review]
1315440.patch (v2).

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

Dragging somehow doesn't work for me on Linux (and dragging from Firefox pastes the image with URL to the online source), but I can image what this wants to do. I think I like the idea.

::: editor/ui/dialogs/content/EdInsSrc.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* Insert Source HTML dialog */
>  
> +var gDataURLs = {};

This could be a Map().

@@ +26,5 @@
>      selection = editor.outputToString("text/html", kOutputFormatted | kOutputSelectionOnly | kOutputWrap);
>    } catch (e) {}
>    if (selection)
>    {
> +    var count = 0;

let

@@ -44,5 @@
> -    } catch (e) {}
> -  }
> -  else
> -  {
> -    dump("Null value -- not inserting in HTML Source dialog\n");

Is this message no longer needed?

@@ +50,3 @@
>      return false;
> +
> +  var html = gDialog.srcInput.value;

You can set this at the top of the function and use in the first if.

@@ +51,5 @@
> +
> +  var html = gDialog.srcInput.value;
> +  // Add back the original data URLs we stashed away earlier.
> +  html = html.replace(/(src|href)="data:[^;]*;\(([0-9]+)\)/g, function(match, attr, num) {
> +    var index = parseInt(num);

let

::: mailnews/compose/content/mailComposeEditorOverlay.xul
@@ +53,5 @@
> +    function OnAcceptOverlay()
> +    {
> +      // Restore data URI.
> +      if (/^data:/i.test(gMsgCompInputElement.value.trim()) &&
> +          gMsgCompInputElement.value.includes(" …")) {

This is hard to read. Can it be written more readably, or commented?

@@ +115,5 @@
> +
> +        // Hide data URIs.
> +        gMsgCompInputElement.value = gMsgCompInputElement
> +          .value.replace(/data:([^;]*;).*/, function(match, mimetype) {
> +            return "data:" + mimetype + " …";

Where does mimetype come from? Is that from the () in the regex?
And we do not hide the data URI, just cut off the encoded data but keep the header "data:mimetype". In the other file you call it "shortening".

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #9)
> This could be a Map().
What's a map, teach me, I'm ignorant.

> let
Sorry, var used in this file.

> Is this message no longer needed?
Not needed. Dump is always bad style.

> > +  var html = gDialog.srcInput.value;
> You can set this at the top of the function and use in the first if.
Right.

> This is hard to read. Can it be written more readably, or commented?
Will comment.

> Where does mimetype come from? Is that from the () in the regex?
> And we do not hide the data URI, just cut off the encoded data but keep the
> header "data:mimetype". In the other file you call it "shortening".
data:image/png;... The image/png is the mimetype. I thought this is useful for the user, but the raw binary data isn't.

Let me research the Map() and present it again ;-)

Comment 11

[:aceman]

(In reply to Jorg K (GMT+1) from comment #10)
> > Where does mimetype come from? Is that from the () in the regex?
> > And we do not hide the data URI, just cut off the encoded data but keep the
> > header "data:mimetype". In the other file you call it "shortening".
> data:image/png;... The image/png is the mimetype. I thought this is useful
> for the user, but the raw binary data isn't.
Yes.

> Let me research the Map() and present it again ;-)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

Comment 12

[Richard Marti (:Paenglab)]

I think the whole data URI shouldn't be hidden in the properties dialog. It's great to hide the data URI in the HTML code as this can lead to unwanted changes by the user when he edits the code and removes parts of the data. In the dialog it is in it's own field and can help for copy paste functions (and we have a cheap data URI converter ;-) ). This isn't a field the user normally changes something for edits. I'm more for the previous patch.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v2b).

Most of Aceman's comments addressed (apart from the 'let').

(In reply to Richard Marti (:Paenglab) from comment #12)
> This isn't a field the user normally changes something for edits.

I think no raw binary data should ever be presented in the UI, let alone for editing (!!). The user can actually modify the image data. So here we have a basic image editor as well ;-) Try it on a PNG.

Also, for medium to large images the field is completely overrun, you can take a year to move the cursor to the end. It also makes the panel slow.

Now is the time to kill those data URIs since - as we all know - more of them are coming ;-)

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #9)
> Dragging somehow doesn't work for me on Linux (and dragging from Firefox
> pastes the image with URL to the online source), but I can image what this
> wants to do. I think I like the idea.
Aceman, for your testing, just paste this:

<img
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAHCAIAAABPxRC5AAAAGklEQVQImWOwcyuBIHv3Unv3UgYIBUfU5gMAd2wiSRT4dtcAAAAASUVORK5CYII=">

and for a second one, this:

<img
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAQAAAAFCAIAAADtz9qMAAAAPUlEQVQImT3CYQqAMAgG0O8qmxEhSgneH2Eu7Fb7M3o8lNvnVm7lhrr1fXakcgpP4RTGuM4/4uhBPagFtQXsFxYEkXyiHgAAAABJRU5ErkJggg==">

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8807881 [details] [diff] [review]
1315440.patch (v2b).

Okay hide it in dialog too.

Maybe you can make it working when the user is in the dialog and he copies the shortened URI he gets the full data in the clipboard.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8807881 [details] [diff] [review]
1315440.patch (v2b).

(In reply to Richard Marti (:Paenglab) from comment #15)
> Maybe you can make it working when the user is in the dialog and he copies
> the shortened URI he gets the full data in the clipboard.
OK, I'll look into that.

Let's clear the r? for now. The match should also be case insensitive.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v3a).

OK, this should now also satisfy Richard ;-)

The full data URI is placed onto the clipboard if the user copies from the shortened display in the image properties.

Not sure what the use case for this would be.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v4a).

Sorry about the noise.

I think it's better not to match on the <attr>="data: ..." since the HTML could be formatted in a way that <attr>= is on one line and that value on the next.

This is what Magnus had initially.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v4b).

Grrr, wrong grammar and trailing white-space.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v3b).

OK, Aceman convinced me that attr="value" can't be broken only multiple lines. So I'm going back to the 3x version here.

Also note that the serialised HTML we receive from Gecko always has the value in double quotes.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8807909 [details] [diff] [review]
1315440.patch (v3b).

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

::: mailnews/compose/content/mailComposeEditorOverlay.xul
@@ +116,5 @@
>          gMsgCompAttachSourceElement.checked = attach;
> +
> +        // Hide the raw binary data part of data URIs.
> +        gMsgCompInputElement.value = gMsgCompInputElement
> +          .value.replace(/data:([^;]*;).*/, function(match, mimetype) {

I should make this /data:([^;]*;).*/i before landing to make it consistent with everything else.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v5a).

OK, yet another version.

After reading
https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs that specifies that the syntax is data:[<mediatype>][;base64],<data> I decided that showing data:image/png;(1) is not such a good idea since the ; belongs to the base 64. All we really want to do is hide the ugly binary data.

So this new patch uses data:image/png;base64,1 in the HTML window and |data:image/png;base64, …| in the properties dialogue.

If the ;base64 is not found, then no replacement takes place. I think this is best.

Comment 23

[:aceman]

Comment on attachment 8807914 [details] [diff] [review]
1315440.patch (v5a).

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

OK, I pasted your sample imgs with data URIs into a draft msg.

Seems to work fine.
Just when I got into the Image insertion (not HTML insertion) the src is shown fine as '...'. But then going into Advanced Edit in that dialog, the src is shown as data:image/png;base64,%E2%80%A6 . That's probably some encoding of the ellipsis.

::: editor/ui/dialogs/content/EdInsSrc.js
@@ +32,5 @@
> +    // Hide the raw binary data part of data URIs.
> +    selection = selection.replace(/(src|href)="data:([^;]*;base64,)[^"]+/gi, function(match, attr, mimetype) {
> +      count++;
> +      gDataURIs.set(count, match);
> +      return attr + "=\"data:" + mimetype + count;

Can you copy also the "data:" string from the original string? Maybe like /(="data:[^;]*;base64,).* . Also, can the 'count' be separated in some more obvious way? Otherwise the user will not understand why there is no data. Maybe "data;;base64, ... [1]". And could there be a tooltip about the shortening? In this dialog the full URI can't be copied to clipboard. Or instead of tooltip you could just put the shortening message in the URI, like "data;;base64, <binary data snipped> [1]" (of course localizable)...

@@ +52,5 @@
> +
> +  // Add back the original data URIs we stashed away earlier.
> +  html = html.replace(/(src|href)="data:[^;]*;base64,([0-9]+)/gi, function(match, attr, num) {
> +    var index = parseInt(num);
> +    if (gDataURIs.get(index) === undefined)

You can use gDataURIs.has(index) here.

::: mailnews/compose/content/mailComposeEditorOverlay.xul
@@ +118,5 @@
> +        // Hide the raw binary data part of data URIs.
> +        gMsgCompInputElement.value = gMsgCompInputElement
> +          .value.replace(/data:([^;]*;base64,).*/i, function(match, mimetype) {
> +            gMsgCompInputElement.addEventListener("copy", onCopy, true);
> +            return "data:" + mimetype + " …";

Can you copy also the "data:" string from the original string? Maybe like /(data:[^;]*;base64,).* . Could there be a tooltip again about the shortening? Is it possible on the field?

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v5b).

This addresses most comments.

Regex matches adjusted, the data replaced by |data:image/png;base64, ... [1]|.

Let's not over-engineer this with localizable text. Remember, this has ui-r+ already.

A tooltip on a text entry field also doesn't seem to work.

I replaced the ellipsis with "..." in order not to affect the advanced edit.

Good enough?

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1315440.patch (v5b).

(shortened some long lines.)

Comment 26

[:aceman]

Comment on attachment 8807923 [details] [diff] [review]
1315440.patch (v5b).

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

Better :)

Comment 27

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e0d8cfec4210f377ecb4de5d0191d5deb0a49cdc

Comment 28

[Jorg K (CEST = GMT+2)]

Landed follow-up to remove spaces before/after ellipsis as discussed with Richard:
https://hg.mozilla.org/comm-central/rev/4a9e63083936cf91348be5010ad1f72b744c70ad

Comment 29

[Magnus Melin [:mkmelin]]

I'd like to reopen this as there are a bunch of things you changed from the original patch that make things quite non-obvious. Yes, some good too ;)

Most obvious, please don't shorten it to exclude ALL the data. I'd really also like you to loose the number. It's quite non-obvious for people that that you could copy the real URL in the properties dialog. For instance the Firefox dev tools has it shortened similarly like what my original patch had: Some data .. the end of the data. What you have now is going to be very confusing for some poor sole who's trying to understand this. Maybe trying to understand what open standards and open protocols are about. You shouldn't dumb things down when there's no need to (no, they are not ugly!).

I find it very contradictory you want to further raw HTML composition but all the sudden what to make this the one thing that no-body can touch/understand even though it's a perfectly fine standard, commonly in use in may places.

Furthermore, it's quite tacky to take part of someone else's patch, then not even ask feedback from the original author before pushing ahead :(

Comment 30

[Jorg K (CEST = GMT+2)]

Formally the patch was landed with approval from Aceman, who is Thunderbird and MailNews peer. It has UI approval from Richard, who is the Theme owner.

Reopening the bug is the wrong process, as I learned in bug 1266836 comment #69 (Quote: This is the incorrect process. We only reopen bugs if the landed code is backed out. Follow-up work is always filed as a separate bug blocking the bug that caused the issue.).

This bug doesn't break anything and no tests fail so there is no reason for a backout.

As for the arguments you have against it:
I see no need to show the user raw binary data which is base64 encoded. The FF *developer* tools (which are for developers) show the full data string, however, the DOM view it's also shortened in the middle.
  "data:image/png;base64,iV...EH/MQAAAABJRU5ErkJggg=="
Maybe I haven't seen what you're referring to.

As you know, shortening the data string does not uniquely identify the image. In another bug, where this patch originated, Henri Sivonen complained about a non-uniqueness of the start of the string. He suggested to use a cryptographic hash instead, which would also have nothing to do with the original data. The number which you want to remove actually gives it the uniqueness.

A different issue is the image/properties dialogue, where a shortened string could be shown. I tried doing a tooltip indicating the copy-ability of the full value, but I didn't try hard enough. It should be possible.

So I'm happy to improve the image/properties dialogue by just shortening the data string and adding the tooltip.

I think the HTML display should remain as it is.

Your work was acknowledged in comment #2.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v1)

I've changed the image properties dialogue so now it displays a shortened data URI à la FF's developer tools. I've shortened so that it fits nicely into the box.

There is now also a tooltip that's only shown if a data URI is displayed.

This is how Richard wanted it initially.

Comment 32

[Richard Marti (:Paenglab)]

Comment on attachment 8807986 [details] [diff] [review]
1315440-followup2.patch (v1)

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

Looks good. I think the textbox width can't be calculated? I'm asking because on OS X the shortened UI doesn't fill the textbox and on Linux it's longer.

::: editor/ui/locales/en-US/chrome/dialogs/EditorImageProperties.dtd
@@ +12,5 @@
>  <!-- These are in the Location tab panel -->
>  <!ENTITY locationEditField.label "Image Location:">
>  <!ENTITY locationEditField.accessKey "L">
>  <!ENTITY locationEditField.tooltip "Type the image's filename or location">
> +<!ENTITY locationEditField.shortenedDataURI "Shortened data URI (you can copy the full data URI)">

What about "Shortened data URI (full URI can be copied)"? This would make the sentence impersonal. I'm not a native English speaker, so sorry when wrong.

Comment 33

[Jorg K (CEST = GMT+2)]

Thanks for the (ui) review.

(In reply to Richard Marti (:Paenglab) from comment #32)
> Looks good. I think the textbox width can't be calculated? I'm asking
> because on OS X the shortened UI doesn't fill the textbox and on Linux it's
> longer.
I'm showing 5 + ... + 35 characters. On Windows is fits in the cases I tried. Since the font used for the textbox is not equidistant, it will depend on the data how well it fills the box, 35 characters for some data will be wider the for other data. Since presenting raw binary data makes no sense IMHO, displaying anything that vaguely looks like base64-encoded data should be OK. That it's a little shorter on Mac and a little wider on Linux shouldn't be a problem.

> ::: editor/ui/locales/en-US/chrome/dialogs/EditorImageProperties.dtd
> > +<!ENTITY locationEditField.shortenedDataURI "Shortened data URI (you can copy the full data URI)">
> What about "Shortened data URI (full URI can be copied)"? This would make
> the sentence impersonal. I'm not a native English speaker, so sorry when
> wrong.
I think your suggestion is better than mine, but maybe include the word 'data' again, so:
  "Shortened data URI (full data URI can be copied)"
I'll change it when landing or fixing other review issues.

Comment 34

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #30)
> Reopening the bug is the wrong process, 

You could argue that. Ideally it's one patch per bug - but splitting discussion over several bugs is also troublesome when there's more work to be done for the feature the bug says it provides.

> As for the arguments you have against it:
> I see no need to show the user raw binary data which is base64 encoded. The
> FF *developer* tools (which are for developers) show the full data string,
> however, the DOM view it's also shortened in the middle.

Yes the DOM view from the Firefox developer tools. 
>   "data:image/png;base64,iV...EH/MQAAAABJRU5ErkJggg=="
> Maybe I haven't seen what you're referring to.

Yes.

> As you know, shortening the data string does not uniquely identify the
> image. In another bug, where this patch originated, Henri Sivonen complained
> about a non-uniqueness of the start of the string. He suggested to use a
> cryptographic hash instead, which would also have nothing to do with the
> original data. The number which you want to remove actually gives it the
> uniqueness

I responded to that comment. The likelyhood for of some two images you want to send to have such similar bits is extremely small + it will almost always be just the one url you're playing with.

> I think the HTML display should remain as it is.

Well I need somewhere to access this for debugging, and your html edit add-on just dumbs it down too much. I think the shortened urls your latest patch uses would work just fine. You don't seem to realize you're primary audience here is developers looking to experiment/debug things. A number showing up all the sudden where it doesn't belong is just confusing.

Comment 35

[Magnus Melin [:mkmelin]]

Comment on attachment 8807986 [details] [diff] [review]
1315440-followup2.patch (v1)

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

Yep, much better thx

::: editor/ui/locales/en-US/chrome/dialogs/EditorImageProperties.dtd
@@ +12,5 @@
>  <!-- These are in the Location tab panel -->
>  <!ENTITY locationEditField.label "Image Location:">
>  <!ENTITY locationEditField.accessKey "L">
>  <!ENTITY locationEditField.tooltip "Type the image's filename or location">
> +<!ENTITY locationEditField.shortenedDataURI "Shortened data URI (you can copy the full data URI)">

maybe
Shortened data URI (copy will copy the full URI)

::: mailnews/compose/content/mailComposeEditorOverlay.xul
@@ +122,5 @@
> +          .value.replace(/(data:[^;]*;base64,)(.*)/i,
> +            function(match, nonDataPart, dataPart) {
> +              gMsgCompInputElement.addEventListener("copy", onCopy, true);
> +              gMsgCompInputElement.setAttribute("tooltip", "shortenedDataURI");
> +              return nonDataPart + dataPart.substring(0, 5) + "..." +

any reason not to use a real ellipsis?

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #35)
> maybe
> Shortened data URI (copy will copy the full URI)
Repeat the word 'copy'?
How about:
  Shortened data URI (copy will place the full data URI onto the clipboard)
Too long?

> any reason not to use a real ellipsis?
Yes, comment #23, you'll get %E2%80%A6 in the Advanced Edit.
I and Aceman actually worked on this *a lot*, see all the superseded patches, so it's unfair to call it "tacky" that we took stuff off your already overfull plate.

So you still want to change the HTML view?

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v2)

This brings back the data URIs to the HTML view, but very short 5 + ... + 10. I trick is applied to make the key unique.

Comment 38

[Jorg K (CEST = GMT+2)]

Sorry, "*A* trick is applied ...". The previous r+ part is also included here with a different tooltip.

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 8808397 [details] [diff] [review]
1315440-followup2.patch (v2)

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

::: editor/ui/dialogs/content/EdInsSrc.js
@@ +30,3 @@
>      selection = (selection.replace(/<body[^>]*>/,"")).replace(/<\/body>/,"");
> +    // Shorten data URIs for display.
> +    selection = selection.replace(/(src|href)(="data:[^;]*;base64,)([^"]+)/gi,

While we're fixing. This should probably be

/(src|href)(=["']?data:[^;]*;base64,)([^"' >])/gi

... because quoting is optional, can be " or '

@@ +36,5 @@
> +        // we include more data.
> +        do {
> +          key = dataPart.substr(0, l) + "…" + dataPart.substr(dataPart.length-10);
> +          l++;
> +        } while (gDataURIs.has(key) && l < dataPart.length);

l < dataPart.length - 10)

.. I suppose. 

The real world case to actually worry about I think isn't that you wouldn't get unique parts for different images. This should check first that it's not the exact same data part (== same image), so that when you paste the same image twice (which is conceivable) you don't get the full string.

::: editor/ui/locales/en-US/chrome/dialogs/EditorImageProperties.dtd
@@ +12,5 @@
>  <!-- These are in the Location tab panel -->
>  <!ENTITY locationEditField.label "Image Location:">
>  <!ENTITY locationEditField.accessKey "L">
>  <!ENTITY locationEditField.tooltip "Type the image's filename or location">
> +<!ENTITY locationEditField.shortenedDataURI "Shortened data URI (copy will place the full data URI onto the clipboard)">

I'd leave out the second "data".
Shortened data URI (copy will place the full URI onto the clipboard)

Comment 40

[Magnus Melin [:mkmelin]]

Before I forget, could you also make copying from he insert html dialog copy full source? You already (almost) have the code for it from the properties dialog.

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #39)
> While we're fixing. This should probably be
> /(src|href)(=["']?data:[^;]*;base64,)([^"' >])/gi
> ... because quoting is optional, can be " or '
Yes, quoting is optional and you can use single and double-quoted. According to
https://www.w3.org/TR/html-markup/syntax.html#syntax-attributes
you can even have spaces around the "=".
Fact is that Gecko always returns attr="value". No spaces, always uses single quotes. And we do this processing on a value returned from Gecko.
So what do you prefer? Cater for the impossible and still don't cover all the cases or just cover the case at hand?

> l < dataPart.length - 10)
Well, I just wanted to avoid an endless loop.

> The real world case to actually worry about I think isn't that you wouldn't
> get unique parts for different images. This should check first that it's not
> the exact same data part (== same image), so that when you paste the same
> image twice (which is conceivable) you don't get the full string.
If you paste the same image twice you get
ABCDE...XYZ for the first one and
ABCDEF...XYZ for the second one.
Otherwise I'd have to have another Map to store the shortened URI as value against the full match as key.

> I'd leave out the second "data".
> Shortened data URI (copy will place the full URI onto the clipboard)
OK.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v3)

Changes:
1) Tooltip fixed.
2) Using another map to produce the same key for the same image.
3) ... l < dataPart.length - 10

Not changed:
1) Regexp still assumes one double-quote.
   Magnus, please indicate whether you want to cater for
   single quote or no quote and also spaces around the = which
   Gecko currently doesn't return.
2) Clipboard not "repaired" when copying from HTML view:
   "could you also make copying from he insert html dialog
   copy full source?"
   No, I can't. I would have to access the clipboard data during the
   "copy" event so restore the full URI.
   However, the clipboard content can't be accessed during this event, see:
   https://developer.mozilla.org/en-US/docs/Web/Events/copy
   "A handler for this event cannot read the clipboard data using
   clipboardData.getData()."
   The only way would be to try to get the selected text from the widget.
   I don't know that this effort is warranted.

Comment 43

[Jorg K (CEST = GMT+2)]

Interdiff works in this case to show the adjustments:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8808397&action=interdiff&newid=8808558&headers=1

I also changed the 'var' to 'let'.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v4)

OK, here's your copy from the HTML window.

Let me know you whether want to cater for single quote or no quote and also spaces around the = which Gecko currently doesn't return.

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 8808587 [details] [diff] [review]
1315440-followup2.patch (v4)

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

::: editor/ui/dialogs/content/EdInsSrc.js
@@ +51,5 @@
> +
> +        // Attach a listener. In case anyone copies from the windows, we want
> +        // to restore the data URI.
> +        if (!listenerAttached) {
> +          gDialog.srcInput.addEventListener("copy", onCopy, false);

This should be 'true' for consistency with the other one I'm adding in the image properties. Or both should be 'false'. In fact, the argument can just be removed. Do you have a preference?

::: mailnews/compose/content/mailComposeEditorOverlay.xul
@@ +120,4 @@
>          gMsgCompInputElement.value = gMsgCompInputElement
> +          .value.replace(/(data:[^;]*;base64,)(.*)/i,
> +            function(match, nonDataPart, dataPart) {
> +              gMsgCompInputElement.addEventListener("copy", onCopy, true);

Here's the other one.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v5)

Sorry about the noise.

Structured the regexp so it's the same in all three cases:
[attr] nonDataPart dataPart.

Only the dataPart is used as key into the maps.

Let me know you whether want to cater for single quote or no quote and also spaces around the = which Gecko currently doesn't return.

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 8808683 [details] [diff] [review]
1315440-followup2.patch (v5)

I think we need "cut" and "paste", too.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v6)

Implemented cut and paste also.

In summary, this is now a little more elaborate than Magnus' initial solution from our "favourite" other bug.

Repeating the question:
Let me know whether you want to change the regexp to cater for single quote or no quote and also spaces around the = which Gecko currently doesn't return.

Now that we do paste processing, the pasted content could originate from a different source and contain these variations. But hey, if they don't paste "Gecko format", they won't get the benefit of the shortening.

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 8808715 [details] [diff] [review]
1315440-followup2.patch (v6)

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

For the quotes I was thinking of the case when someone edited/pasted stuff. It's "Insert HTML" after all. But, ok let's just skip prettifying it for that case. 

Good catch about cut. Add that for the dialog too?

Comment 50

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #45)
> Comment on attachment 8808587 [details] [diff] [review]
> This should be 'true' for consistency with the other one I'm adding in the
> image properties. Or both should be 'false'. In fact, the argument can just
> be removed. Do you have a preference?

Not really.

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup2.patch (v7)

Here is the 19th patch in this bug and the 7th version of the follow up.

I've reworked the copy in the image properties and added the cut case, although it will of course destroy the image when cutting the location :-(

I will land this now.

Comment 52

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/94785ab39d5d274bc86cbbe1043625b1ba57ae68

Comment 53

[Magnus Melin [:mkmelin]]

Actually, I just noticed a bug. If you have a data uri image and then go into the properties dialog + accept, it gets destroyed.

Comment 54

[Magnus Melin [:mkmelin]]

At least if you go in and say, change the height (Through the Advanced Edit...)

Comment 55

[Jorg K (CEST = GMT+2)]

I've changed the alt text and the tooltip and there wasn't a problem.

I'll try the Advanced Edit now.

Comment 56

[Jorg K (CEST = GMT+2)]

Yes, Advanced Edit seems to apply the shortened data string to the image. I'll send another patch ;-(

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup3.patch (v1).

OK, the bad news and the good news.

Bad news:
Everything we've done so far in mailComposeEditorOverlay.xul was 100% wrong.
I backed it all out to how it was before I started.

All the code needs to go into EdImageOverlay.js to be integrated with Advanced Edit.

The good news:
I fixed it and is was just a matter to lift the code from where it shouldn't be to where it should be pretty much 1:1.

Now, if we ever introduce anchors with href=<data URI> they need the same treatment. At least we know now, how it's done.

This is patch number 20 in this bug :-(

Comment 58

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup3.patch (v2).

Image preview also needed a tweak.

Comment 59

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup3.patch (v2 for real now).

Oops, forgot hg qref.

Comment 60

[Magnus Melin [:mkmelin]]

Comment on attachment 8808943 [details] [diff] [review]
1315440-followup3.patch (v2 for real now).

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

LGTM, r=mkmelin

Not for this bug, but I'll note that changing image width/height through Advanced Edit does not work unless you first go to Dimensions and select custom size.

::: editor/ui/dialogs/content/EdImageOverlay.js
@@ +75,5 @@
> +}
> +function onCopy(event) {
> +  commonCopyCut(event, false);
> +}
> +function commonCopyCut(event, isCut) {

nitty, but this could preferably be named onCopyOrCut 
... and then loose the onCut/onCopy

@@ +91,5 @@
> +  //              a new value that the user pasted in.
> +  if (selection == gDialog.srcInput.value.trim() &&
> +      /^data:/i.test(selection) && selection.includes("…")) {
> +    event.clipboardData.setData("text/plain", gFullDataURI);
> +    if (isCut) {

event.type == "cut" instead

Comment 61

[Jorg K (CEST = GMT+2)]

Thanks.

(In reply to Magnus Melin from comment #60)
> nitty, but this could preferably be named onCopyOrCut 
> ... and then loose the onCut/onCopy

Then I need to fix it in EdInsSrc.js as well (landed yesterday):
https://hg.mozilla.org/comm-central/rev/94785ab39d5d274bc86cbbe1043625b1ba57ae68#l2.94

Comment 62

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup3.patch (v3)

In this patch all the issues in EdImageOverlay.js are addressed.

As I said in comment #61, I had to revisit EdInsSrc.js to fix the same issues. In doing so and testing it again, I found more problems:
1) Paste listener only attached if selection wasn't empty.
   It needs to be attached unconditionally to allow paste into an empty
   HTML window.
2) Paste listener only worked when src="data: ..." was pasted since
   (src|hrf) was part of the regexp. That's a little to restrictive,
   since if anyone copies only the |data: ...| the paste substitution
   doesn't work. So I modified all the regexp again to take
   out the attribute part.
3) Since I took out the |(src|href)="data: ...| I also took out the
   fixed double-quote, so that opened the door to changing the
   regexp even further to allow for the single-quote and no-quote
   versions.

This has turned out much more difficult than I thought, so I hope it's finally done now.

Comment 63

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5cb6c4f805a525ffda697c35221af3612f4cccf3

Resolved for the third time, well, perhaps third time lucky ;-)

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: 1315440-followup4.patch

Needed another follow up:
https://hg.mozilla.org/comm-central/rev/6d9be007357c6500b1894b19000c02bcf45fb003