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

RESOLVED FIXED in Firefox 28

Status

()

Toolkit
View Source
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: hsivonen, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

Trunk
mozilla29
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox28- fixed, firefox29- fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.)

Updated

4 years ago
Blocks: 943632
Duplicate of this bug: 946640

Updated

4 years ago
tracking-firefox28: --- → ?
Keywords: regression
(Assignee)

Comment 2

4 years ago
(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.

Updated

4 years ago
tracking-firefox29: --- → ?
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.
tracking-firefox28: ? → -
tracking-firefox29: ? → -
(Assignee)

Comment 4

4 years ago
Created attachment 8351727 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8351727 - Flags: review?(bmcbride)
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?

Updated

4 years ago
Component: Menus → View Source
Product: Firefox → Toolkit
Version: unspecified → Trunk
(Assignee)

Comment 6

4 years ago
(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.
(Reporter)

Comment 7

4 years ago
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. :-(
(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 on attachment 8351727 [details] [diff] [review]
Proposed patch

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

What Henri said in comment 7 needs resolved.
Attachment #8351727 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 10

4 years ago
(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.)
(Reporter)

Comment 11

4 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Assignee)

Comment 12

4 years ago
Created attachment 8355911 [details] [diff] [review]
Addressed review comments
Attachment #8351727 - Attachment is obsolete: true
Attachment #8355911 - Flags: review?(bmcbride)
(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?
Attachment #8355911 - Flags: review?(bmcbride) → review+
(Reporter)

Comment 15

4 years ago
Why isn't browser-charsetmenu.inc included into viewSource.xul but copied and pasted?
(Assignee)

Updated

4 years ago
Blocks: 956657
https://hg.mozilla.org/mozilla-central/rev/98660dab432f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Comment 17

4 years ago
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.
Attachment #8355911 - Flags: approval-mozilla-beta?
Attachment #8355911 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 18

4 years ago
Why approval-mozilla-beta? Firefox 27 should be unaffected, right?
(Assignee)

Comment 19

4 years ago
Comment on attachment 8355911 [details] [diff] [review]
Addressed review comments

Sorry, I thought beta was 28 for some reason.
Attachment #8355911 - Flags: approval-mozilla-beta?
Attachment #8355911 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/1222ad5d15b4
status-firefox28: --- → fixed
status-firefox29: --- → fixed

Comment 21

4 years ago
Moving l10n files around is a massive string freeze break.

Can we please back this out from aurora?
Flags: needinfo?(lsblakk)
Does this have in-testsuite coverage?
Flags: needinfo?(neil)
Flags: in-testsuite?
(Assignee)

Comment 23

4 years ago
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.)
Flags: needinfo?(neil)

Comment 24

4 years ago
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
Ryan - can you please back this out of 28?
Flags: needinfo?(lsblakk) → needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8613903c6b1

Setting the status back to affected. Please change to wontfix if necessary.
status-firefox28: fixed → affected
Flags: needinfo?(ryanvm)
Thank you Ryan, given the l10n impact we will indeed wontfix on 28 and let this ride the trains.
status-firefox28: affected → wontfix
Attachment #8355911 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
(Assignee)

Comment 28

4 years ago
Created attachment 8365337 [details] [diff] [review]
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!
Attachment #8365337 - Flags: feedback?(l10n)
Attachment #8365337 - Flags: approval-mozilla-aurora?

Comment 29

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

Thanks, this looks good from an l10n pov.
Attachment #8365337 - Flags: feedback?(l10n) → feedback+
Attachment #8365337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox28: wontfix → affected
You need to log in before you can comment on or make changes to this bug.