704204 - Allow user to increase Web Console font size

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Kevin Dangoor]

When viewing web pages, the user can zoom the text. The user can't zoom the web console output text.

I'm filing this on behalf of a user who described console text in all of the developer tools as being hard to read with "over 40" eyes. Potentially, this could be useful for people doing demos as well, but this is a weaker use case.

Comment 1

[Mihai Sucan [:msucan]]

This is something I would very much appreciate to see fixed. This should be a good first bug. I am volunteering to be a mentor.

Comment 2

[Nigel Babu [:nigelb]]

Attachment: patch-v1

First run of a patch :)

Comment 3

[Paul Rouget [:paul]]

Comment on attachment 578052 [details] [diff] [review]
patch-v1

I am not sure to understand how that can work:
> +        hudElement.fontSize = hudElement.fontSize + 1px;

1px is not a valid JavaScript identifier.

(And thank you for taking care of this bug!)

Comment 4

[Nigel Babu [:nigelb]]

Bah, my bad :) I'll refresh this and attach a new patch tonight. Note to self - coding at 2 am isn't a great idea.

Comment 5

[Nigel Babu [:nigelb]]

Attachment: patch-v2

Ok, this is better. It doesn't work yet, not sure where its going wrong. At least the context menu is populated.

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 578285 [details] [diff] [review]
patch-v2

> hudElement.style.fontSize = hudElement.style.fontSize + 1;

That won't work. You're changing a value that is a string.
If the font-size is "1em", you'll get "1em1", which doesn't make sense.

You probably want to do something like that:

> hudElement.style.fontSize = (parseFloat(hudElement.style.fontSize) * 1.1) + "em";

If you want to add pixels to the "em" value, you should use -moz-calc:

> hudElement.style.fontSize = "-moz-calc(" + hudElement.style.fontSize + " + 1px)";

These are basic examples of what you can do. I would encourage a smarter approach where you would store somewhere (in a pref?) the current "em" value without the "em" suffix and increment it in JavaScript, and then add it to the CSS value.

Comment 7

[Nigel Babu [:nigelb]]

Attachment: patch-v3

This version works. Now it needs some prefs foo, which I'm trying to do on my own, if that doesn't work, I'll seek help :-)

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 578486 [details] [diff] [review]
patch-v3

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

This is awesome! Great work Nigel! It works fine. I only have minor nits, small comments.

Next step:
- add a pref. Should be as easy as Services.prefs.setIntPref("devtools.webconsole.fontsize", parseInt(outputNode.style.fontSize)). In makeHUDNodes(), where outputNode is constructed, you can do outputNode.style.fontSize = Services.prefs.getIntPref("devtools.webconsole.fontsize").

Thank you very much for your contribution!

::: browser/devtools/webconsole/HUDService.jsm
@@ +6348,5 @@
>          HUDService.saveRequestAndResponseBodies = checked;
>          break;
>        }
> +      case "increaseSize": {
> +        let hudElement = HUDService.hudReferences[hudId].outputNode;

Nit: let outputNode = hud.outputNode; (you already have the hud reference)

@@ +6353,5 @@
> +        if (!hudElement.style.fontSize) {
> +          let cs = hud.chromeWindow.getComputedStyle(hudElement, null);
> +          hudElement.style.fontSize = cs.fontSize;
> +        }
> +        hudElement.style.fontSize = (parseInt(hudElement.style.fontSize) + 1) + 'px';

Nit: please use double quotes (coding style). :)

::: browser/themes/gnomestripe/devtools/webconsole.css
@@ +135,5 @@
>  
>  .hud-output-node,
>  .jsterm-input-node,
>  .jsterm-complete-node {
> +  font: 12px "DejaVu Sans Mono";

Please change this "Dejavu Sans Mono" to monospace. (maybe on IRC I was not clear enough, sorry!)

I looked more into the work needed for bug 704181 - the bug we talked on IRC. It seems it would add more complexity than needed for this patch. So we leave that to be fixed in its own bug.

Comment 9

[Nigel Babu [:nigelb]]

Attachment: patch-v4

So, this fixes the nits and add the prefs.  I'm not so sure about swallowing that exception when getting the pref, otherwise, I have a good feeling about this now.

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 578823 [details] [diff] [review]
patch-v4

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

Patch looks really good now. Thanks for your updates!

Next steps:
- it would make sense for the jsterm input field to also increase its font size. In the code that updates the outputNode.style.fontSize, please also set the same size to hud.jsterm.completeNode and hud.jsterm.inputNode.
  - in JSTF_generateUI() please read the fontSize pref and set it for the two elements, like you do in makeHUDNodes().
- we need an automated test that checks these two commands work.

Also, would it make sense to add Ctrl + and Ctrl - support? When the input is focused. I think it would. You could do this in JSTF_keyPress(): when any of the two keys are pressed, you could call the font size increase/decrease commands as needed.

::: browser/devtools/webconsole/HUDService.jsm
@@ +3685,5 @@
> +    try {
> +      let cs = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> +      this.outputNode.style.fontSize = cs + "px";
> +    } catch(e) {
> +    }

Nit: code style:

try {
}
catch (ex) { }

@@ +6359,5 @@
> +          let cs = hud.chromeWindow.getComputedStyle(outputNode, null);
> +          outputNode.style.fontSize = cs.fontSize;
> +        }
> +        outputNode.style.fontSize = (parseInt(outputNode.style.fontSize) + 1) + "px";
> +        Services.prefs.setIntPref("devtools.webconsole.fontSize", (parseInt(outputNode.style.fontSize) + 1));

You are saving the new fontSize + 1. I assume you want:

Services.prefs.setIntPref("devtools.webconsole.fontSize", parseInt(outputNode.style.fontSize));

(because the fontSize is already increased)

@@ +6369,5 @@
> +          let cs = hud.chromeWindow.getComputedStyle(outputNode, null);
> +          outputNode.style.fontSize = cs.fontSize;
> +        }
> +        outputNode.style.fontSize = (parseInt(outputNode.style.fontSize) - 1) + "px";
> +        Services.prefs.setIntPref("devtools.webconsole.fontSize", (parseInt(outputNode.style.fontSize) -1));

Same as above.

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +86,5 @@
>  copyCmd.label=Copy
>  copyCmd.accesskey=C
>  selectAllCmd.label=Select All
>  selectAllCmd.accesskey=A
> +increaseSizeCmd.label=Increase Font Size

Please add localization notes for these new strings.

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 578823 [details] [diff] [review]
patch-v4

Stephen: can you please provide your input about this bit of new UI?

Nigel is a contributor who wants to add support for increasing/decreasing the Web Console output font size. How would it make sense to present this UI to users?

For starters (more like as a "placeholder" waiting for better ideas) I suggested Nigel to add two new context menu items: Increase/decrease font size. I also believe Ctrl + and Ctrl - would be appropriate for "zoom in/out" when the user has the Web Console command line focused.

Any thoughts?

Thank you very much!

Comment 12

[Nigel Babu [:nigelb]]

Attachment: patch-v5

Keyboard shortcuts added and works on linux, restoration works, increase and decrease moved into its own functions and called as needed.  Also, added localization notes!

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 579674 [details] [diff] [review]
patch-v5

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

This is really great work. Thank you! Only some nits below. f+!

Next step: the test!

::: browser/devtools/webconsole/HUDService.jsm
@@ +4151,5 @@
> +    this.jsterm.completeNode.style.fontSize = "";
> +    this.jsterm.inputNode.style.fontSize = "";
> +    Services.prefs.clearUserPref("devtools.webconsole.fontSize")
> +
> +  }

nit: empty line above.

@@ +5263,5 @@
>                                             this.inputNode.value.length);
>            aEvent.preventDefault();
>            break;
> +        case 61:
> +          // control =

Please use Ci.nsIDOMKeyEvent.DOM_VK_EQUALS here.

Also please add case 43 (which is the char code for +).

@@ +5273,5 @@
> +          hud.decreaseFontSize();
> +          aEvent.preventDefault();
> +          break;
> +        case 48:
> +          // control 0

Please use Ci.nsIDOMKeyEvent.DOM_VK_0 here.

Comment 14

[Mihai Sucan [:msucan]]

Nigel: any progress on this bug? Anything I can help with?

Comment 15

[Nigel Babu [:nigelb]]

Yeah, I got stuck with the tests.  Let me ping you today and finish it.

Comment 16

[Mihai Sucan [:msucan]]

Nigel: did you get the tests working?

Comment 17

[Nigel Babu [:nigelb]]

Ugh, I lost the computer itself thanks to charger catching fire. Will reset the environment on my mac and try again.

Comment 18

[Mihai Sucan [:msucan]]

Nigel: sorry to hear about that. No worries! Please let me know how I can help you with writing the tests. Thanks!

Comment 19

[Rob Campbell [:rc] (:robcee)]

this hasn't been touched in a month. Anything we can do here?

Comment 20

[Nigel Babu [:nigelb]]

Really sorry about the delays, I'm working on this today.

Comment 21

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v6

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 648940 [details] [diff] [review]
patch-v6

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

Thanks for your patch Jen! Glad to see that all tests pass.

Font sizes are a bit weird at this point. I think you still need to update some css. Timestamps, for example, do not change their size when I zoom in/out. Can you please look into this? Please let me know if you need any help.

More comments below.

::: browser/devtools/webconsole/test/browser_webconsole_change_font_size.js
@@ +18,5 @@
> +  }, true);
> +}
> +
> +function testFontSizeChange(hud) {
> +  Services.prefs.setIntPref("devtools.webconsole.fontSize", 10);

You might want to do this before the web console is open, so you can see if the given font size is applied correctly, after the web console opens.

@@ +23,5 @@
> +  let inputNode = hud.jsterm.inputNode;
> +  inputNode.style.fontSize = 10;
> +  inputNode.focus();
> +
> +  EventUtils.synthesizeKey("=", { accelKey: true, ctrlKey: true });

You can have only accelKey: true and it should work on all systems.

::: browser/devtools/webconsole/webconsole.js
@@ +2482,5 @@
>     * @param nsIDOMEvent aEvent
>     */
>    keyPress: function JSTF_keyPress(aEvent)
>    {
> +    if (aEvent.ctrlKey || aEvent.metaKey) {

This changes existing keyboard shortcuts.

Instead of this approach, please let if (aEvent.ctrlKey) {....} as is.

Add after the if a new block:

if (aEvent.accelKey) [
  ... the new zoom shortcuts ...
}


Even better: this keyboard event handler only works when you are inside the jsterm input. Maybe you could add a keypress event handler in WebConsoleFrame and have it listen for the entire WebConsole iframe. What do you think? That would allow Ctrl++ and Ctrl+- to work when you focus the output node.

@@ +2935,5 @@
> +   *        The size of the font change. An unmatched size assumes a font reset.
> +   */
> +  changeFontSize: function JST_changeFontSize(size)
> +  {
> +    let defaultFontSize = this.hud.owner.chromeDocument.defaultView

this.hud.window

@@ +2943,5 @@
> +    // As a reasonable minimum, 10px is the smallest we'll set the font size to.
> +    let minFontSize = 10;
> +
> +    try {
> +      fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");

What happens if you have two web consoles open and you try to have different zoom levels in each? Shouldn't this only happen at open? When you zoom in/out, it should increase/decrease based on the current font size being used.

@@ +2947,5 @@
> +      fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> +    }
> +    catch(e) {}
> +
> +    if (size === '+' || size === '-') {

nits: please use double quotes and non-strict comparisons, to match current file coding style.

::: browser/themes/gnomestripe/devtools/webconsole.css
@@ +17,5 @@
>  .webconsole-timestamp {
>    color: GrayText;
>    margin-top: 0;
>    margin-bottom: 0;
> +  font: 12px monospace;

Since bug 704181 is now fixed, please no longer change font families.

(this comment applies to all places where font families have changed in this patch)

Comment 23

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v7

Comment 24

[Mihai Sucan [:msucan]]

Comment on attachment 649636 [details] [diff] [review]
patch-v7

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

This is much nicer. Thank you!

General comment: the number that shows how many times a message is repeated does not increase its size when I zoom in/out. Could this be fixed?

More comments follow below.

::: browser/devtools/webconsole/test/browser_webconsole_change_font_size.js
@@ +21,5 @@
> +
> +function testFontSizeChange(hud) {
> +  let inputNode = hud.jsterm.inputNode;
> +  let outputNode = hud.jsterm.outputNode;
> +  outputNode.style.fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");

Why do you do this here?

::: browser/devtools/webconsole/webconsole.js
@@ +173,5 @@
>    this._cssNodes = {};
>    this._outputQueue = [];
>    this._pruneCategoriesQueue = {};
>    this._networkRequests = {};
> +  this._keyPress = this.keyPress.bind(this);

Please make the keyPress method private. _onKeyPress. Then you can bind:

this._onKeyPress = this._onKeyPress.bind(this);

@@ +328,5 @@
> +    consoleWrapper.addEventListener("keypress", this._keyPress, false);
> +
> +    try {
> +      let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> +      this.outputNode.style.fontSize = fontSize + "px";

Please validate the user config. Make sure the minimum font size is respected.

Also add the new pref to browser/app/profile/firefox.js with a description.

Default pref value should be 0. When it's 0, you do not apply the font size pref - just let the system default.

@@ +895,5 @@
> +   * @param nsIDOMEvent aEvent
> +   */
> +  keyPress: function WCF_keyPress(aEvent)
> +  {
> +    if (aEvent.ctrlKey || aEvent.metaKey || aEvent.accelKey) {

Are the ctrlKey and metaKey prop checks here needed? Afaik only accelKey should be enough...

@@ +949,5 @@
> +    if (size == "+" || size == "-") {
> +      fontSize = parseInt(fontSize, 10);
> +
> +      if (isNaN(fontSize)) {
> +        fontSize = defaultFontSize;

This is confusing for me.

defaultFontSize is set to the computed font size of the outputNode, then to the outputNode.style.fontSize.

fontSize is set to defaultFontSize.

then you check if fontSize is a number, so if it's invalid you reset to defaultFontSize. Isn't it the same?

I think you want something like...
if outputNode.style.fontSize
   fontSize = outputNode.style.fontSize
else
   fontSize = getcomputedstyle-for-outputNode.fontsize

then parseInt(). If it's not a number, just reset to "" (no specific font size for the elements).

@@ -2496,5 @@
>                                             this.inputNode.value.length);
>            aEvent.preventDefault();
>            break;
> -        default:
> -          break;

Is this change needed here?

Comment 25

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v8

A couple of notes:

- I still wasn't able to trigger cmd++ or cmd+- while only calling accelKey. Still required metaKey for anything to register. But I will leave it checking only ctrlKey and accelKey as you suggested earlier.

- I was not able to trigger cmd+- using Ci.nsIDOMKeyEvent["DOM_VK_MINUS"] but it works as the numerical key code.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 651267 [details] [diff] [review]
patch-v8

>+    try {
>+      let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
>+
>+      if (fontSize > 0 && fontSize < MIN_FONT_SIZE) {

This doesn't make sense. Also, I'm not sure what exception you're catching here...

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 651267 [details] [diff] [review]
patch-v8

>--- a/browser/themes/gnomestripe/devtools/webconsole.css
>+++ b/browser/themes/gnomestripe/devtools/webconsole.css

> .webconsole-timestamp {
>   color: GrayText;
>   margin-top: 0;
>   margin-bottom: 0;
>-  font: 12px "DejaVu Sans Mono", monospace;
>+  font: 1em "DejaVu Sans Mono", monospace;
> }

Why are you specifying 1em here rather than just using font-family?

> .webconsole-msg-repeat {
>   margin: 2px 0;
>   padding-left: 4px;
>   padding-right: 4px;
>   color: white;
>   background-color: red;
>   border-radius: 40px;
>   font: message-box;
>-  font-size: 10px;
>+  font-size: 1em;
>   font-weight: 600;
> }

Why are you specifying 1em here rather than just dropping font-size?

Comment 28

[Jen Fong-Adwent [:ednapiranha]]

(In reply to Dão Gottwald [:dao] from comment #26)
> Comment on attachment 651267 [details] [diff] [review]
> patch-v8
> 
> >+    try {
> >+      let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> >+
> >+      if (fontSize > 0 && fontSize < MIN_FONT_SIZE) {
> 
> This doesn't make sense. Also, I'm not sure what exception you're catching
> here...

I set a minimum font size so that a font can avoid having an unreasonable default such as 1px. As mentioned by msucan, if the font size is 0, we ignore it and use the system default.

Comment 29

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v9

Removed the 1em assignment in the CSS files and had to put a check for metaKey back in onKeyPress, since my tests only passed if that was present.

Comment 30

[Dão Gottwald [:dao]]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #28)
> (In reply to Dão Gottwald [:dao] from comment #26)
> > Comment on attachment 651267 [details] [diff] [review]
> > patch-v8
> > 
> > >+    try {
> > >+      let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> > >+
> > >+      if (fontSize > 0 && fontSize < MIN_FONT_SIZE) {
> > 
> > This doesn't make sense. Also, I'm not sure what exception you're catching
> > here...
> 
> I set a minimum font size so that a font can avoid having an unreasonable
> default such as 1px.

You're checking that the pref is _below_ the minimum.

Comment 31

[Mihai Sucan [:msucan]]

Comment on attachment 651376 [details] [diff] [review]
patch-v9

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

Jen, thanks for your patch updates!

Dão, thanks for your comments on the patch.

Jen: I looked into the problem with the accelKey attribute. I believe at this point it makes sense to get rid of the JS-based approach. Just add a xul:key element in webconsole.xul. Look at how the other commands are implemented in CommandController. To see how page zoom is done, look at browser/base/content/browser-sets.inc. There search for key_fullZoom. You'll need to add new strings to webconsole.dtd.

::: browser/app/profile/firefox.js
@@ +1104,5 @@
>  pref("devtools.webconsole.filter.info", true);
>  pref("devtools.webconsole.filter.log", true);
>  
> +// Font size based on the system default for the webconsole
> +pref("devtools.webconsole.fontSize", 0);

Comment should say:

// Text size in the Web Console. Use 0 for the system default size.

::: browser/devtools/webconsole/webconsole.js
@@ +176,5 @@
>    this._cssNodes = {};
>    this._outputQueue = [];
>    this._pruneCategoriesQueue = {};
>    this._networkRequests = {};
> +  this._keyPress = this._onKeyPress.bind(this);

s/_keyPress/_onKeyPress/

@@ +328,5 @@
>      this._initPositionUI();
>      this._initFilterButtons();
>  
> +    let consoleWrapper = doc.querySelector(".hud-console-wrapper");
> +    consoleWrapper.addEventListener("keypress", this._keyPress, false);

Ditto.

@@ +331,5 @@
> +    let consoleWrapper = doc.querySelector(".hud-console-wrapper");
> +    consoleWrapper.addEventListener("keypress", this._keyPress, false);
> +
> +    try {
> +      let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");

You added the preference to firefox.js. You no longer need to have the try-catch wrapping.

@@ +334,5 @@
> +    try {
> +      let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> +
> +      if (fontSize > 0 && fontSize < MIN_FONT_SIZE) {
> +        this.outputNode.style.fontSize = MIN_FONT_SIZE + "px";

This doesn't make sense.

You set the outputNode.style.fontSize only if it's below MIN_FONT_SIZE.

Also, shouldn't you set the font size for the inputNode as well?

@@ +901,5 @@
> +   *
> +   * @private
> +   * @param nsIDOMEvent aEvent
> +   */
> +  _onKeyPress: function WCF_onKeyPress(aEvent)

s/WCF_onKey/WCF__onKey/

@@ +942,5 @@
> +   * Increase, decrease or reset the font size.
> +   *
> +   * @private
> +   * @param string size
> +   *        The size of the font change. An unmatched size assumes a font reset.

Please explain which values are allowed.

@@ +944,5 @@
> +   * @private
> +   * @param string size
> +   *        The size of the font change. An unmatched size assumes a font reset.
> +   */
> +  _changeFontSize: function WCF__changeFontSize(size)

nit: arguments need to be prefixed with "a". (please follow the existing code style).

Comment 32

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v10

I seem to have everything set up but the controls are ignored when testing. Not sure what I could be missing?

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 652443 [details] [diff] [review]
patch-v10

>+    let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
>+
>+    if (fontSize !== 0) {

!=

>+      if (fontSize < MIN_FONT_SIZE) {
>+        this.outputNode.style.fontSize = MIN_FONT_SIZE + "px";
>+        this.completeNode.style.fontSize = MIN_FONT_SIZE + "px";
>+        this.inputNode.style.fontSize = MIN_FONT_SIZE + "px";
>+      else {
>+        this.outputNode.style.fontSize = fontSize + "px";
>+        this.completeNode.style.fontSize = fontSize + "px";
>+        this.inputNode.style.fontSize = fontSize + "px";
>+      }

how about:

fontSize = Math.max(MIN_FONT_SIZE, fontSize);

this.outputNode.style.fontSize = fontSize + "px";
this.completeNode.style.fontSize = fontSize + "px";
this.inputNode.style.fontSize = fontSize + "px";

>-  font: 12px "DejaVu Sans Mono", monospace;
>+  font: "DejaVu Sans Mono", monospace;

Any reason why you're using font rather than font-family here?

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 652443 [details] [diff] [review]
patch-v10

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

Thanks for your update Jen! This update is on the right path.

You also need to update the CommandController in webconsole.js. CommandController implements the actual commands. From CommandController you can call changeFontSize() as needed, for each command.

Please let me know if you need further help with this.

Below I have some additional comments...

::: browser/devtools/webconsole/webconsole.js
@@ +330,5 @@
>  
> +    let fontSize = Services.prefs.getIntPref("devtools.webconsole.fontSize");
> +
> +    if (fontSize !== 0) {
> +      if (fontSize < MIN_FONT_SIZE) {

Please use Math.max(), as pointed out by Dão.

::: browser/devtools/webconsole/webconsole.xul
@@ +19,5 @@
>  
>    <commandset id="editMenuCommands"/>
> +  <commandset id="cmd_fullZoomReduce"/>
> +  <commandset id="cmd_fullZoomEnlarge"/>
> +  <commandset id="cmd_fullZoomReset"/>

You do not need additional commandsets here.

@@ +21,5 @@
> +  <commandset id="cmd_fullZoomReduce"/>
> +  <commandset id="cmd_fullZoomEnlarge"/>
> +  <commandset id="cmd_fullZoomReset"/>
> +
> +  <command id="cmd_fontSizeEnlarge" oncommand="cmd_changeFontSize('+');"/>

Here you put oncommand="goDoCommand('cmd_fontSizeEnlarge')". Similar for the other commands.

@@ +42,5 @@
> +#endif
> +
> +#ifdef XP_WIN
> +  <key id="key_fontSizeReduce" key="&fontEnlarge.commandkey2;"
> +       command="key_fontSizeEnlarge" modifiers="accel"/>

Wrong command id here.

Comment 35

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v11

Hey Mihai, I wasn't able to successfully get any <key> command to work in webconsole.xul. I even tried resorting to oncommand on an alert() and it still wasn't picking up the keypress. Any ideas what I am missing?

Comment 36

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v12

Sorry, updating with deletion of some lines but same problem as mentioned before :)

Comment 37

[Mihai Sucan [:msucan]]

Comment on attachment 653353 [details] [diff] [review]
patch-v12

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

Thanks for your update. From a quick look, this seems to be a problem with the <key> elements in the <keyset> and the keycodes in the DTD. See below...

::: browser/devtools/webconsole/webconsole.js
@@ +3374,5 @@
> +   * @param string size
> +   *        The size of the font change. Accepted values are "+" and "-".
> +   *        An unmatched size assumes a font reset.
> +   */
> +  changeFontSize: function CommandController_changeFontSize(aSize)

Please keep the CommandController lightweight. This method needs to go back to the WebConsoleFrame object. In the command controller you only call this.owner.changeFontSize() with the correct arguments. Sorry for the confusion!

@@ +3440,5 @@
>        case "cmd_selectAll":
>          this.selectAll();
>          break;
> +      case "cmd_fontSizeEnlarge":
> +        this.changeFontSize("+");

As explained above: this.owner.changeFontSize("+");

::: browser/devtools/webconsole/webconsole.xul
@@ +52,5 @@
> +#ifdef XP_WIN
> +    <key id="key_fontSizeEnlarge" keycode="&fontEnlarge.commandkey2;"
> +         command="key_fontSizeEnlarge" modifiers="accel"/>
> +#endif
> +  </keyset>

This keyset is confusing for me. Can you please keep it simple? Just copy what others did in browser-sets.inc. Put all those key_fullZoom* <key> elements. Then copy the strings from browser.dtd [1] and put them in webconsole.dtd.

[1] browser/locales/en-US/chrome/browser/browser.dtd

Comment 38

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v12

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 653750 [details] [diff] [review]
patch-v12

>+    if (fontSize !== 0) {

As mentioned earlier, use !=.

>+      if (aSize == "+") {
>+        fontSize += 1;
>+      }
>+      else if (aSize == "-") {
>+        fontSize -= 1;
>+      }

You already checked earlier that aSize is either "+" or "-", so the "else if" should just be an "else":

if (aSize == "+") {
  fontSize++;
} else {
  fontSize--;
}

Comment 40

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v13

Comment 41

[Mihai Sucan [:msucan]]

Comment on attachment 653762 [details] [diff] [review]
patch-v13

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

Thanks for your update Jen! Also thanks for your great patience with us! This is very near r+.

General comments:

- Just noticed that .jsterm-wrapper-node doesn't exist - the same goes for .jsterm-output-node, .jsterm-output-line and .jsterm-scroll-to-node. Please entirely remove these styles from the webconsole.css files.

- The .jsterm-input-node and .jsterm-complete-node styles continue to have font-size 12px when I reset to the default size. The webconsole.css files give these values - please remove the 12px font-size definition.

- Also noticed that the default font size is bigger than what current users expect in the Web Console. Please put a font-size 0.85em for .hud-output-node and for the jsterm input and complete nodes.

I have more comments below...

::: browser/devtools/webconsole/webconsole.js
@@ +151,5 @@
>  
>  // The preference prefix for all of the Web Console filters.
>  const FILTER_PREFS_PREFIX = "devtools.webconsole.filter.";
>  
> +// Minimum font size

nit: period at the end.

@@ +536,5 @@
> +   */
> +  changeFontSize: function WCF_changeFontSize(aSize)
> +  {
> +    let fontSize = this.window
> +                   .getComputedStyle(this.consoleToolbar, null)

Why do you use consoleToolbar here? Why not outputNode?

::: browser/devtools/webconsole/webconsole.xul
@@ +30,5 @@
> +    <key id="key_fullZoomEnlarge" key="&fullZoomEnlargeCmd.commandkey;"  command="cmd_fullZoomEnlarge" modifiers="accel"/>
> +    <key                          key="&fullZoomEnlargeCmd.commandkey2;" command="cmd_fullZoomEnlarge" modifiers="accel"/>
> +    <key                          key="&fullZoomEnlargeCmd.commandkey3;" command="cmd_fullZoomEnlarge" modifiers="accel"/>
> +    <key id="key_fullZoomReset"   key="&fullZoomResetCmd.commandkey;"    command="cmd_fullZoomReset"   modifiers="accel"/>
> +    <key                          key="&fullZoomResetCmd.commandkey2;"   command="cmd_fullZoomReset"   modifiers="accel"/>

nit: please remove the unneeded whitespace that aligns attributes that resulted from copy/paste from browser-sets.inc.

::: browser/locales/en-US/chrome/browser/devtools/webConsole.dtd
@@ +78,5 @@
>  <!ENTITY btnClear.label        "Clear">
>  <!ENTITY btnClear.tooltip      "Clear the Web Console output">
>  <!ENTITY btnClose.tooltip      "Close the Web Console">
> +
> +<!ENTITY fullZoomEnlargeCmd.accesskey   "I">

Is this used anywhere?

(same question for the other .accesskey entities)

::: browser/themes/gnomestripe/devtools/webconsole.css
@@ -62,5 @@
>    color: white;
>    background-color: red;
>    border-radius: 40px;
>    font: message-box;
> -  font-size: 10px;

The base font-size was 12px and now by removing this line, the node that shows how many times a message was repeated will have the same size as the message.

Please change this to font-size: 0.9em so it becomes smaller than whatever the parent font-size is.

Comment 42

[Jen Fong-Adwent [:ednapiranha]]

Attachment: patch-v14

Comment 43

[Mihai Sucan [:msucan]]

Comment on attachment 653809 [details] [diff] [review]
patch-v14

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

Thank you very much Jen! I'll test this patch on my Mac OS (not just Ubuntu) and I will land it. Hopefully I'll get to do this tomorrow.

Comment 44

[Rob Campbell [:rc] (:robcee)]

Attachment: windows screenshot with defaults

Comment 45

[Mihai Sucan [:msucan]]

Thank you Rob.

Here's a green try push:
https://tbpl.mozilla.org/?tree=Try&rev=9029615b4d51

Comment 46

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] patch-v14b

Did font-size adjustments for each system (Linux, Mac and Windows).

Landed:
https://hg.mozilla.org/integration/fx-team/rev/8707f5ddeda3

Thank you Jen! Really glad to see this bug getting fixed.

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8707f5ddeda3