1167478 - Menu items "Larger Font" and "Smaller Font" should be disabled if the current font size is equals to "MAXIMUM_FONT_SIZE" or "MINIMUM_FONT_SIZE" constants

Status: VERIFIED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Willian Gustavo Veiga]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150520030205

Steps to reproduce:

1. Open scratchpad.
2. "View" -> "Larger Font" until it reaches the maximum font size.


Actual results:

When the editor font size reached the maximum size, "Larger Font" menu item is still enabled.


Expected results:

"Larger Font" menu item should have been disabled. The same applies for the "Smaller Font" menu item.

Comment 1

[Willian Gustavo Veiga]

Attachment: 1167478-menu-items-larger-font-and-smaller-font-should-be-disabled-if-the-current-font-size-is-equals-to-maximum_font_size-or-minimum_font_size-constants-v1.patch

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8609136 [details] [diff] [review]
1167478-menu-items-larger-font-and-smaller-font-should-be-disabled-if-the-current-font-size-is-equals-to-maximum_font_size-or-minimum_font_size-constants-v1.patch

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

This is nice. I have a few comments and a couple of tests fail (remember to run scratchpad tests with and without --e10s), but this is close to landing.

::: browser/devtools/scratchpad/scratchpad.js
@@ -251,5 @@
>    },
>  
> -  /**
> -   * Check or uncheck view menu items according to stored preferences.
> -   */

Don't remove the comment from the existing method please.

@@ +254,5 @@
>      this._updateViewMenuItem(SHOW_LINE_NUMBERS, "sp-menu-line-numbers");
>      this._updateViewMenuItem(WRAP_TEXT, "sp-menu-word-wrap");
>      this._updateViewMenuItem(SHOW_TRAILING_SPACE, "sp-menu-highlight-trailing-space");
> +    this._disableViewMenuItem(MINIMUM_FONT_SIZE, "sp-cmd-smaller-font");
> +    this._disableViewMenuItem(MAXIMUM_FONT_SIZE, "sp-cmd-larger-font");

I'd call this _updateViewFontMenuItem, as I would expect a "disable" method to actually disable the menu item.

::: browser/devtools/scratchpad/test/browser_scratchpad_disable_view_menu_items.js
@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* Bug N */

Instead of the bug number we need a short description of the purpose of the test here.

@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* Bug N */
> +
> +// only finish() when correct number of tests are done

I'm sure you used one of the existing tests as inspiration, but the style of this test is needlessly complex. See browser_scratchpad_inspect_primitives.js for an example of an easier to write (and follow) style.

@@ +8,5 @@
> +var count = 0;
> +function done()
> +{
> +  if (++count == expected) {
> +    finish();

The test has to reset the font size to normal when it's done. See how every scratchpad test has a huge font size when you run every test in browser/devtools/scratchpad.

Comment 3

[Willian Gustavo Veiga]

Attachment: 1167478-menu-items-larger-font-and-smaller-font-should-be-disabled-if-the-current-font-size-is-equals-to-maximum_font_size-or-minimum_font_size-constants-v2.patch

I've made the changes but the tests are not passing yet. There is a race condition problem that I cannot detect.

This patch is what I've got so far.

Thank you Panos Astithas.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8617557 [details] [diff] [review]
1167478-menu-items-larger-font-and-smaller-font-should-be-disabled-if-the-current-font-size-is-equals-to-maximum_font_size-or-minimum_font_size-constants-v2.patch

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

::: browser/devtools/scratchpad/test/browser_scratchpad_disable_view_menu_items.js
@@ +43,5 @@
> +  menu.doCommand();
> +
> +  ok(cmd.hasAttribute('disabled') === false, 'Command "sp-cmd-larger-font" is enabled.');
> +
> +  Services.prefs.clearUserPref('devtools.scratchpad.editorFontSize');

The problem is that you are clearing the font size pref and assume that it has an effect on the already open scratchpad. That is not the case. You have to avoid clearing the pref in the end of the first test and the beginning of the second test, and also start the second loop from MAXIMUM_FONT_SIZE, as that is where you have left it before.

r=me with these changes.

Comment 5

[Willian Gustavo Veiga]

Attachment: 1167478-menu-items-larger-font-and-smaller-font-should-be-disabled-if-the-current-font-size-is-equals-to-maximum_font_size-or-minimum_font_size-constants-v3.patch

Now it's working, even with "--e10s"!
Thank you again Panos.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8622801 [details] [diff] [review]
1167478-menu-items-larger-font-and-smaller-font-should-be-disabled-if-the-current-font-size-is-equals-to-maximum_font_size-or-minimum_font_size-constants-v3.patch

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

Looks good, but you don't need to ask for review again when a patch is r+ with comments, if you address the comments.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ca6bc23357aa

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca6bc23357aa

Comment 9

[Sunny]

I have reproduced this bug on Nightly 41.0a1 (2015-05-21) with my Windows 7, 64 Bit!

The bug's fix is verified on Latest Nightly 43.0a1!

Build ID 	20150821030204
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

Comment 10

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce this bug on Nightly 41.0a1 (2015-05-21) with the instruction from comment 0 on Linux x64

This Bug is now verified as fixed on Latest Firefox Beta 41.0b4

Build ID: 20150824144923
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0

As it is also verified on Windows (Comment 9), Marking it as verified!