Closed Bug 1385817 Opened 2 years ago Closed 2 years ago

Enable more ESLint rules for accessible/ (automatically fix)

Categories

(Firefox :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: standard8, Assigned: dbugs, Mentored)

Details

Attachments

(3 files)

Now that Android has some ESLint rules enabled, we should remove the disabling of some of the rules so that we get closer to the general mozilla configuration.

In accessible/.eslintrc.js, we should be able to automatically fix some of the rules (note: split into several blocks, as we probably want to split this to make review easier):

- quotes

- space-before-function-paren
- space-infix-ops
- no-multi-spaces
- spaced-comment

- no-else-return
- brace-style
Comment on attachment 8892552 [details]
Bug 1385817 - Enable the quotes ESLint rule for accessible/

https://reviewboard.mozilla.org/r/163554/#review168906

::: accessible/.eslintrc.js
(Diff revision 1)
>      // XXX These are rules that are enabled in the recommended configuration, but
>      // disabled here due to failures when initially implemented. They should be
>      // removed (and hence enabled) at some stage.
>      "brace-style": "off",
>      "consistent-return": "off",
> -    "quotes": "off",

is it Mozilla style guide? I sort of liked single quotes in JS: it's easier to type them and they look lighter
Comment on attachment 8892552 [details]
Bug 1385817 - Enable the quotes ESLint rule for accessible/

delegating this one to Yura, since he's more jsat and arounds guy than me
Attachment #8892552 - Flags: review?(surkov.alexander) → review?(yzenevich)
Comment on attachment 8892553 [details]
Bug 1385817 - Enable space related ESLint rules for accessible/

delegating this one to Yura, since he's more jsat and arounds guy than me
Attachment #8892553 - Flags: review?(surkov.alexander) → review?(yzenevich)
Attachment #8892554 - Flags: review?(surkov.alexander) → review?(yzenevich)
(In reply to alexander :surkov from comment #4)
> > -    "quotes": "off",
> 
> is it Mozilla style guide? I sort of liked single quotes in JS: it's easier
> to type them and they look lighter

So far we've enabled double-quotes for pretty much all of mozilla-central that we cover (including browser/ toolkit/ and services/). I've just looked, and it turns out it is in the style guide as well:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Literals

Generally we've been looking to harmonise the rules across mozilla-central to make it easier for developers & contributors.
Comment on attachment 8892552 [details]
Bug 1385817 - Enable the quotes ESLint rule for accessible/

https://reviewboard.mozilla.org/r/163554/#review169258

Looks good, just one question

::: accessible/jsat/AccessFu.jsm:94
(Diff revision 1)
>      }
>  
>      // Add stylesheet
> -    let stylesheetURL = 'chrome://global/content/accessibility/AccessFu.css';
> +    let stylesheetURL = "chrome://global/content/accessibility/AccessFu.css";
>      let stylesheet = Utils.win.document.createProcessingInstruction(
> -      'xml-stylesheet', 'href="' + stylesheetURL + '" type="text/css"');
> +      "xml-stylesheet", 'href="' + stylesheetURL + '" type="text/css"');

Curious about this case, will this trigger a linting error? wouldn't we want `href="${stylesheetURL}" type="text/css"` instead?

::: accessible/tests/mochitest/jsat/doc_content_integration.html:12
(Diff revision 1)
> -    var frameContents = '<html>' +
> -      '<head><title>such app</title></head>' +
> -      '<body>' +
> -      '<h1>wow</h1>' +
> -      '<ul>' +
> +    var frameContents = "<html>" +
> +      "<head><title>such app</title></head>" +
> +      "<body>" +
> +      "<h1>wow</h1>" +
> +      "<ul>" +
>        '<li><label><input type="checkbox">many option</label></li>' +

same here

::: accessible/tests/mochitest/jsat/doc_content_integration.html:14
(Diff revision 1)
> -      '<body>' +
> -      '<h1>wow</h1>' +
> -      '<ul>' +
> +      "<body>" +
> +      "<h1>wow</h1>" +
> +      "<ul>" +
>        '<li><label><input type="checkbox">many option</label></li>' +
> -      '</ul>' +
> +      "</ul>" +
>        '<label for="r">much range</label>' +

same here

::: accessible/tests/mochitest/jsat/doc_content_integration.html:15
(Diff revision 1)
> -      '<h1>wow</h1>' +
> -      '<ul>' +
> +      "<h1>wow</h1>" +
> +      "<ul>" +
>        '<li><label><input type="checkbox">many option</label></li>' +
> -      '</ul>' +
> +      "</ul>" +
>        '<label for="r">much range</label>' +
>        '<input min="0" max="10" value="5" type="range" id="r">' +

same here
Attachment #8892552 - Flags: review?(yzenevich) → review+
Comment on attachment 8892552 [details]
Bug 1385817 - Enable the quotes ESLint rule for accessible/

https://reviewboard.mozilla.org/r/163554/#review169258

> Curious about this case, will this trigger a linting error? wouldn't we want `href="${stylesheetURL}" type="text/css"` instead?

quotes allows the use of single quotes to avoid escaping double quotes:

http://eslint.org/docs/rules/quotes

However in this case, we could also switch to a template string, which I'll do before landing.
Comment on attachment 8892553 [details]
Bug 1385817 - Enable space related ESLint rules for accessible/

https://reviewboard.mozilla.org/r/163556/#review169268

looks good thanks
Attachment #8892553 - Flags: review?(yzenevich) → review+
Comment on attachment 8892554 [details]
Bug 1385817 - Enable brace-style and no-else-return ESLint rules for accessible/

https://reviewboard.mozilla.org/r/163558/#review169272

Looks good just the comments related to indentation.

::: accessible/jsat/AccessFu.jsm:777
(Diff revision 1)
>              // XXX: Fix for rtl
>              return;
> -          } else {
> -            target.blur();
>            }
> +            target.blur();

extra indentation here

::: accessible/jsat/AccessFu.jsm:790
(Diff revision 1)
>              // XXX: Fix for rtl
>              return;
> -          } else {
> -            target.blur();
>            }
> +            target.blur();

extra indentation here

::: accessible/jsat/AccessFu.jsm:802
(Diff revision 1)
>              // Don't blur content if caret is not at start of text area.
>              return;
> -          } else {
> -            target.blur();
>            }
> +            target.blur();

extra indentation here

::: accessible/jsat/Traversal.jsm:201
(Diff revision 1)
>        if (Utils.getState(aAccessible).contains(States.LINKED)) {
>          return Filters.IGNORE;
> -      } else {
> -        return Filters.MATCH;
>        }
> +        return Filters.MATCH;

extra indentation here

::: accessible/jsat/Traversal.jsm:290
(Diff revision 1)
>        if (Utils.getState(aAccessible).contains(States.LINKED)) {
>          return Filters.MATCH;
> -      } else {
> -        return Filters.IGNORE;
>        }
> +        return Filters.IGNORE;

same here

::: accessible/jsat/Traversal.jsm:409
(Diff revision 1)
>  
>        return moved;
> -    } else {
> -      return aVirtualCursor[aMethod](rule);
>      }
> +      return aVirtualCursor[aMethod](rule);

I'm guessing there are more more cases down the diff where we have extra indentatin when else block is removed.

::: accessible/tests/mochitest/events.js:610
(Diff revision 1)
>      return aEventSeq.idx != aEventSeq.length ? aEventSeq[aEventSeq.idx] : null;
>    }
>  
>    this.areExpectedEventsLeft =
> -    function eventQueue_areExpectedEventsLeft(aScenario)
> -  {
> +    function eventQueue_areExpectedEventsLeft(aScenario) {
> +    function scenarioHasUnhandledExpectedEvent(aEventSeq) {

need indentation here

::: accessible/tests/mochitest/jsat/test_content_integration.html:329
(Diff revision 1)
>                  ["dom.ipc.browser_frames.oop_by_default", true],
>                  ["dom.mozBrowserFramesEnabled", true],
>                  ["browser.pagethumbnails.capturing_disabled", true]
>                ]
> -            }, doTest) },
> +            }, doTest)
> +},

indentation is wrong here
Attachment #8892554 - Flags: review?(yzenevich) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2439d45c93be
Enable the quotes ESLint rule for accessible/ r=yzen
https://hg.mozilla.org/integration/autoland/rev/f3cdf0e12b8a
Enable space related ESLint rules for accessible/ r=yzen
https://hg.mozilla.org/integration/autoland/rev/46fa29da7f94
Enable brace-style and no-else-return ESLint rules for accessible/ r=yzen
You need to log in before you can comment on or make changes to this bug.