940907 - Re-introduce the Character Encoding menu to the View Source window

Status: RESOLVED FIXED

(Toolkit :: View Source, enhancement)

Description

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

Bug 805374 removes the Character Encoding submenu of the View menu from the View Source window.  Per review request of there, this is the bug for tracking the re-introduction of that submenu. (Personally, I don't think it's particularly important to do this,  since View Source  picks up the encoding from the browser window, so this menu is only needed if the user decides that an override is necessary after viewing source.)

Comment 2

[[e-mail]]

(In reply to Henri Sivonen from comment #0)
> I don't think it's particularly important to do this, since View Source
> picks up the encoding from the browser window, so this menu is only needed
> if the user decides that an override is necessary after viewing source.

That's assuming the browser window is the only entry point into view source. Obviously you other Toolkit applications to think of, including the Error Console which links to the view source window, while DOM Inspector is an example of an extension that links to the view source window.

Comment 3

[Benjamin Kerensa [:bkerensa]]

I recommend that someone nominate for tracking or uplift based on what release milestone this gets targeted for but for now I don't think we will track this.

Comment 4

[[e-mail]]

Attachment: Proposed patch

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8351727 [details] [diff] [review]
Proposed patch

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -9,17 +9,17 @@
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/NotificationDB.jsm");
> Cu.import("resource:///modules/RecentWindow.jsm");
> Cu.import("resource://gre/modules/WindowsPrefSync.jsm");
> 
> XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                   "resource://gre/modules/Task.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "CharsetMenu",
>-                                  "resource:///modules/CharsetMenu.jsm");
>+                                  "resource://gre/modules/CharsetMenu.jsm");

Doesn't your addition to toolkit/content/charsetOverlay.js make this redundant?

>--- a/toolkit/locales/en-US/chrome/global/charsetOverlay.dtd
>+++ b/toolkit/locales/en-US/chrome/global/charsetOverlay.dtd
>@@ -1,17 +1,27 @@
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
> <!-- extracted from charsetOverlay.xul -->
> <!ENTITY charsetMenu.label             "Character Encoding">
> <!ENTITY charsetMenu.accesskey         "C">
> <!ENTITY charsetMenuAutodet.label      "Auto-Detect">
>-<!ENTITY charsetMenuAutodet.accesskey  "a">
>+<!ENTITY charsetMenuAutodet.accesskey  "D"><!-- A reserved for Arabic -->
>+
>+<!ENTITY charsetMenuAutodet.off.label     "(Off)">
>+<!ENTITY charsetMenuAutodet.off.accesskey "O">
>+<!ENTITY charsetMenuAutodet.ja.label      "Japanese">
>+<!ENTITY charsetMenuAutodet.ja.accesskey  "J">
>+<!ENTITY charsetMenuAutodet.ru.label      "Russian">
>+<!ENTITY charsetMenuAutodet.ru.accesskey  "R">
>+<!ENTITY charsetMenuAutodet.uk.label      "Ukranian">
>+<!ENTITY charsetMenuAutodet.uk.accesskey  "U">

Does charsetMenuAutodet.accesskey need to be renamed since other locales could have similar conflicts? Or is this business as usual because you're adding items to the menu, which should always make localizers check their access keys?

Comment 6

[[e-mail]]

(In reply to Dão Gottwald from comment #5)
> (From update of attachment 8351727 [details] [diff] [review])
> > XPCOMUtils.defineLazyModuleGetter(this, "CharsetMenu",
> >-                                  "resource:///modules/CharsetMenu.jsm");
> >+                                  "resource://gre/modules/CharsetMenu.jsm");
> 
> Doesn't your addition to toolkit/content/charsetOverlay.js make this redundant?

That's quite possible, I only checked that I didn't break anything.

> >-<!ENTITY charsetMenuAutodet.accesskey  "a">
> >+<!ENTITY charsetMenuAutodet.accesskey  "D"><!-- A reserved for Arabic -->
> 
> Does charsetMenuAutodet.accesskey need to be renamed since other locales
> could have similar conflicts? Or is this business as usual because you're
> adding items to the menu, which should always make localizers check their
> access keys?

I know access keys are always tricky. My preference is to post in .l10n to warn localisers of potential access key changes. In this case I was planning to let localisers know that these were moved from charsetMenu.dtd in the first place.

Comment 7

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

Comment on attachment 8351727 [details] [diff] [review]
Proposed patch

>-<!ENTITY % charsetDTD SYSTEM "chrome://browser/locale/charsetMenu.dtd" >
>+<!ENTITY % charsetDTD SYSTEM "chrome://global/locale/charsetOverlay.dtd" >

Folding this stuff back into charsetOverlay.dtd makes it harder to stop shipping dead code in Firefox and harder to stop keeping non-Firefox code in mozilla-central in bug 943252. I'd prefer keeping the browser-oriented charsetMenu.dtd (etc.) separate from the legacy charserOverlay stuff, which should no longer ship in Firefox and should, IMO, move out of m-c.

>+<!ENTITY charsetMenuAutodet.uk.label      "Ukranian">

Please watch out for re-introducing bug 952736 if its fix lands first.

> <!ENTITY charsetMenuMore.label         "More Encodings">
> <!ENTITY charsetMenuMore.accesskey     "m">
> <!ENTITY charsetMenuMore1.label        "West European">
> <!ENTITY charsetMenuMore1.accesskey    "w">
> <!ENTITY charsetMenuMore2.label        "East European">
> <!ENTITY charsetMenuMore2.accesskey    "E">
> <!ENTITY charsetMenuMore3.label        "East Asian">
> <!ENTITY charsetMenuMore3.accesskey    "A">

Having these in the same .dtd means re-shipping these dead UI strings in Firefox. :-(

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #5)
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
[...]
> Doesn't your addition to toolkit/content/charsetOverlay.js make this
> redundant?

No, charsetOverlay isn't used in Firefox.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8351727 [details] [diff] [review]
Proposed patch

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

What Henri said in comment 7 needs resolved.

Comment 10

[[e-mail]]

(In reply to Henri Sivonen from comment #7)
> Folding this stuff back into charsetOverlay.dtd makes it harder to stop
> shipping dead code in Firefox and harder to stop keeping non-Firefox code in
> mozilla-central in bug 943252.

OK, so if I just move rather than rename the dtd and the properties, import CharsetMenu.jsm from the view source window, and paste the charsetMenu into viewSource.xul, does that cover everything?

> Having these in the same .dtd means re-shipping these dead UI strings in
> Firefox. :-(

(You're already shipping those strings, unless you wanted to do some funky #ifdef in toolkit's jar.mn files. Anyway, you already covered that above.)

Comment 11

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

(In reply to [e-mail] from comment #10)
> (In reply to Henri Sivonen from comment #7)
> > Folding this stuff back into charsetOverlay.dtd makes it harder to stop
> > shipping dead code in Firefox and harder to stop keeping non-Firefox code in
> > mozilla-central in bug 943252.
> 
> OK, so if I just move rather than rename the dtd and the properties, import
> CharsetMenu.jsm from the view source window, and paste the charsetMenu into
> viewSource.xul, does that cover everything?

The reason I didn't do that in the first place is that the charset menu depends on JS from browser.js, so you'll probably need to refactor that code into CharsetMenu.jsm.

Comment 12

[[e-mail]]

Attachment: Addressed review comments

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Henri Sivonen (:hsivonen) from comment #11)
> The reason I didn't do that in the first place is that the charset menu
> depends on JS from browser.js, so you'll probably need to refactor that code
> into CharsetMenu.jsm.

Could you file a followup bug, Neil?

Comment 14

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98660dab432f

Comment 15

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

Why isn't browser-charsetmenu.inc included into viewSource.xul but copied and pasted?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98660dab432f

Comment 17

[[e-mail]]

Comment on attachment 8355911 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 805374
User impact if declined: 
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: Two locale files moved, this makes it possible to fix further uses of the charset menu.

Comment 18

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

Why approval-mozilla-beta? Firefox 27 should be unaffected, right?

Comment 19

[[e-mail]]

Comment on attachment 8355911 [details] [diff] [review]
Addressed review comments

Sorry, I thought beta was 28 for some reason.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1222ad5d15b4

Comment 21

[Axel Hecht [:Pike]]

Moving l10n files around is a massive string freeze break.

Can we please back this out from aurora?

Comment 22

[u279076]

Does this have in-testsuite coverage?

Comment 23

[[e-mail]]

I don't think the menu itself has ever had any tests. (The underlying functionality might do, but I assume that's not relevant here. I don't see any test annotations on the bug that removed the menu either.)

Comment 24

[Magnus Melin [:mkmelin]]

comm-central has/had a basic test for it which we should now enable again - http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-policy/test-view-source.js#31

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Ryan - can you please back this out of 28?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8613903c6b1

Setting the status back to affected. Please change to wontfix if necessary.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Thank you Ryan, given the l10n impact we will indeed wontfix on 28 and let this ride the trains.

Comment 28

[[e-mail]]

Attachment: Branch-safe patch

With string moves faked using repackaging and merged to branch.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 805374
User impact if declined: 
Testing completed (on m-c, etc.): Baked on m-c
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: NONE!

Comment 29

[Axel Hecht [:Pike]]

Comment on attachment 8365337 [details] [diff] [review]
Branch-safe patch

Thanks, this looks good from an l10n pov.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/79cbd6fb8205