Closed
Bug 1385817
Opened 7 years ago
Closed 7 years ago
Enable more ESLint rules for accessible/ (automatically fix)
Categories
(Firefox :: General, enhancement)
Firefox
General
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
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 6•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8892554 -
Flags: review?(surkov.alexander) → review?(yzenevich)
Reporter | ||
Comment 7•7 years ago
|
||
(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 8•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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 10•7 years ago
|
||
mozreview-review |
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 11•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2439d45c93be https://hg.mozilla.org/mozilla-central/rev/f3cdf0e12b8a https://hg.mozilla.org/mozilla-central/rev/46fa29da7f94
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in
before you can comment on or make changes to this bug.
Description
•