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

VERIFIED FIXED in Firefox 41


4 years ago
11 months ago


(Reporter: beberveiga, Assigned: beberveiga)


41 Branch
Firefox 41

Firefox Tracking Flags

(firefox41 fixed)


(Whiteboard: [bugday-20150826])


(1 attachment, 2 obsolete attachments)



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


4 years ago
Component: Untriaged → Developer Tools: Scratchpad


4 years ago
Attachment #8609136 - Flags: review?(past)
Comment on attachment 8609136 [details] [diff] [review]

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.
> + */
> +/* 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.
> + */
> +/* 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.
Attachment #8609136 - Flags: review?(past)
Assignee: nobody → contact
Ever confirmed: true

Comment 3

4 years ago
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.
Attachment #8609136 - Attachment is obsolete: true
Attachment #8617557 - Flags: review?(past)
Comment on attachment 8617557 [details] [diff] [review]

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.
Attachment #8617557 - Flags: review?(past) → review+
Comment on attachment 8622801 [details] [diff] [review]

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.
Attachment #8622801 - Flags: review?(past) → review+
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
QA Whiteboard: [good first verify]

Comment 9

4 years ago
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
QA Whiteboard: [good first verify] → [testday-20150821]
QA Whiteboard: [testday-20150821] → [good first verify][testday-20150821]
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!
Whiteboard: [bugday-20150826]


11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.