1448689 - View Message Source (Ctrl+U), then "View > Text Encoding" doesn't work

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

In bug 1439021 I made "View Message Source" work again:
https://hg.mozilla.org/comm-central/rev/2caecab6e8c0
Landed 27-Feb-2018. Looks like on 28-Feb-2018 there was no Daily.

In Daily of 1-Mar-2018, the "View > Text Encoding" worked. In that binary I could change, for example, from Western to Unicode, and when revisiting the menu later, it was still set to Unicode.

Now that doesn't work any more. Bug 1441939 also made changes to viewSource.xul, but that landed before the Daily build on 1-Mar-2018.

So somewhere between 1st of March and today, something broke. Actually, since TB 60 beta was branched off on 12-Mar-2018 and it's not working there either, the breakage occurred between the 1st and 12th.

Alice, can you please find it for us:

STR:
Ctrl+U on a message, View > Text Encoding, select an encoding. Close menu.
Revisit menu, check that encoding is still selected.

Comment 1

[Alice0775 White]

I cannot reproduce the issue on the following latest Windows 2012 x64 opt tc(B) build

Build ID  20180325101631
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Thunderbird/61.0a1
Built from comm-central revision 89435f04afee556e91d8f17634cd377dff5ac: 89435f04afee556e91d8f17634cd377dff5ac8d2
Built from mozilla-central revision 6862624e24d005fb4f8fb07c6800d2acef: 6862624e24d005fb4f8fb07c6800d2acef1d287e

My Steps To Reproduce:
1. Select a message in thread pane
2. Ctrl+U
   --- I can observe that "Source of bla bla" window pops up
3. View > Text Encoding in "Source of bla bla" window
   --- I can observe "Western" is selected now
4. Chose　”Unicode"
   --- the menu rolls up
5. Again, View > Text Encoding in "Source of bla bla" window
   --- I can observe "Western" is selected now

Comment 2

[Jorg K (CEST = GMT+2)]

Is there a misunderstanding?
In 4 you chose "Unicode" and in 5 you revisit and it's back at "Western". That's the bug. It should be "Unicode".

Also, when you chose a different encoding, the displayed content should actually change, which is doesn't. What do you get for a Japanese e-mail in UTF-8 or ISO-2022-JP? Also "Western"? - then the display would be garbled. Before you could correct that by choosing the correct encoding. Now choosing an encoding has no effect.

Comment 3

[Jorg K (CEST = GMT+2)]

Like I have an e-mail in windows-1252 here, with an á. That displays fine in "Western", but when I switch to "Unicode", it should be garbled. But since it doesn't switch, nothing happens.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Mail in UTF-8.eml

Try viewing the source of this e-mail in UTF-8. You can't you're stuck at Western.

Comment 5

[Alice0775 White]

(In reply to Jorg K (GMT+1) from comment #2)
> Is there a misunderstanding?
> In 4 you chose "Unicode" and in 5 you revisit and it's back at "Western".
> That's the bug. It should be "Unicode".
> 
> Also, when you chose a different encoding, the displayed content should
> actually change, which is doesn't. What do you get for a Japanese e-mail in
> UTF-8 or ISO-2022-JP? Also "Western"? - then the display would be garbled.
> Before you could correct that by choosing the correct encoding. Now choosing
> an encoding has no effect.

Aha, My bad. I had misunderstood. I will try again.

Comment 6

[Alice0775 White]

Regression window;

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=278d1f336d04a8bc0a6cb2181a98bee40ce6fb9f&tochange=2edea7714121b2f25cc155b0592520ce1ebfa6fa
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7543a3e938d5ec408a7d0c93dc71b40607e7d5b&tochange=bccdc684210431c233622650a91454c09f6af9eb

Comment 7

[Alice0775 White]

Suspect:
3cee5c02db0a	J. Ryan Stinnett — Bug 1443371 - Remove unused page descriptor in viewSourceUtils.js. r=bdahl
b98a010be06f	J. Ryan Stinnett — Bug 1443371 - Remove unused View Source message listeners. r=bdahl

Comment 8

[Jorg K (CEST = GMT+2)]

That's it:
https://hg.mozilla.org/mozilla-central/rev/b98a010be06f#l1.17

We used that to set the charset:
  onSetCharacterSet(event) {
    if (event.target.hasAttribute("charset")) {
      let charset = event.target.getAttribute("charset");

      // If we don't have history enabled, we have to do a reload in order to
      // show the character set change. See bug 136322.
      this.sendAsyncMessage("ViewSource:SetCharacterSet", {

:-(

Thanks so much for finding it.

Comment 9

[Jorg K (CEST = GMT+2)]

"ViewSource:ToggleWrapping", "ViewSource:ToggleSyntaxHighlighting" and "ViewSource:SetCharacterSet" were all removed, so that stuff will also not work any more. We should remove it from the menu, I've never used it.

Being able to set the charset is essential, we need to restore that.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1448689-charset.patch (v1)

This works for me.

It uses snippets from here:
https://hg.mozilla.org/mozilla-central/rev/b98a010be06f#l1.131
The reload is always necessary.

As far as I can tell, wrapping already didn't work for e-mail source and syntax highlighting makes no sense for e-mail source either.

Both were also used in SM, but since the message sending for "ViewSource:ToggleWrapping", "ViewSource:ToggleSyntaxHighlighting" has gone away, I'm just guarding this code with #ifdef and SM may want to restore whatever they deem fit. I can't dedicate more time to this and we need to restore at least the charset feature.

Please apply the patch on top of bug 1448652 (which will land soon).

Comment 11

[Richard Marti (:Paenglab)]

Highlight was also used to show the line when opened from console. I know, opening from console doesn't work now.

Comment 12

[Jorg K (CEST = GMT+2)]

Syntax highlight the JS code? Sorry, it was always *disabled*, I tried in TB 52.

Comment 13

[Richard Marti (:Paenglab)]

Sorry, only saw the "highlighting". Yeah, syntax highlighting wasn't enabled.

Comment 14

[Jorg K (CEST = GMT+2)]

"Goto" still works and that highlights the line ;-)

Comment 15

[:aceman]

Comment on attachment 8962202 [details] [diff] [review]
1448689-charset.patch (v1)

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

Thanks, works for me.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/019e34d5f6f5
fix setting charset and remove wrapping and highlight options in 'View Source'. r=aceman

Comment 17

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 2):
https://hg.mozilla.org/releases/comm-beta/rev/3a4f54410a545bb3303d7e73aeecd89972a07bba

Comment 18

[Jorg K (CEST = GMT+2)]

Grrrr, I removed id="menu_wrapLongLines" and id="menu_highlightSyntax" from the XUL file, but it's still referenced in the JS file :-(

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1448689-follow-up.patch

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5c77a04b630b
Follow-up: Don't access removed menu items. r=me

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3fc4e8ecfe15
Follow-up: Fix linting error. r=me DONTBUILD

Comment 22

[:aceman]

Coudl you please avoiding preprocessing? Maybe like at https://dxr.mozilla.org/comm-central/rev/5e66a21a7abacdcdf32cf4274dbf919b1c1ec914/mailnews/import/content/importDialog.js#981 .

Comment 23

[:aceman]

OR maybe it would be enough to check whether wrapMenuItem and highlightMenuItem are not null first?

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1448689-follow-up-take2.patch

Sorry, better with review this time :-(

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3296b386b6a7
Backed out changeset 3fc4e8ecfe15 and 5c77a04b630b. a=backout
https://hg.mozilla.org/comm-central/rev/c43c822333a1
Follow-up: Don't access removed menu items. r=aceman DONTBUILD

Comment 26

[:aceman]

Comment on attachment 8965586 [details] [diff] [review]
1448689-follow-up-take2.patch

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

Thanks.

Comment 27

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 3):
https://hg.mozilla.org/releases/comm-beta/rev/756d53caf14f9125a69f7a28f38fd224ceef7052