Closed Bug 1470922 Opened 6 years ago Closed 6 years ago

Enable CodeMirror JSTerm on Nightly except for people using accessible technology

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [boogaloo-mvp])

Attachments

(3 files, 1 obsolete file)

Bug 1470922 - Enable CodeMirror JSTerm on Nightly except for people using accessible technology; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1470922 - Make sure jsterm tests run in both old and new jsterm; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1470922 - Fix tests failing with the new Jsterm; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
We should look at how the Accessibility panel checks if the user has a screen reader.
Hey Nicolas, do you want this added to [boogaloo-mvp]
Flags: needinfo?(nchevobbe)
(In reply to Marco Mucci [:MarcoM] from comment #1)
> Hey Nicolas, do you want this added to [boogaloo-mvp]

Yes :)
Flags: needinfo?(nchevobbe)
Whiteboard: [boogaloo-mvp]
Priority: -- → P2
Blocks: 1473805
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Marco, would you have some time to apply patch from https://bugzilla.mozilla.org/attachment.cgi?id=8994479 and tell us if the console is working well for you ?
Hopefully a better CodeMirror version will be out soon-ish so we don't have to keep the old input around.
Flags: needinfo?(mzehe)
Hm, will this give me the new code mirror or the old one, or is this to test whether I am *not* getting CodeMirror?
Flags: needinfo?(mzehe) → needinfo?(nchevobbe)
Sorry, I should have been more clear. I want to test that you won't get CodeMirror so your workflow is not broken.
As far as I know, the "new" codeMirror is still in development (even if they're looking for people to test what they started).
Flags: needinfo?(nchevobbe)
Comment on attachment 8994479 [details]
Bug 1470922 - Enable CodeMirror JSTerm on Nightly except for people using accessible technology; .

https://reviewboard.mozilla.org/r/259032/#review266068

::: devtools/client/webconsole/webconsole-output-wrapper.js:240
(Diff revision 1)
>          serviceContainer,
>          hud,
>          onFirstMeaningfulPaint: resolve,
>          closeSplitConsole: this.closeSplitConsole.bind(this),
> -        jstermCodeMirror: store.getState().prefs.jstermCodeMirror,
> +        jstermCodeMirror: store.getState().prefs.jstermCodeMirror
> +          && Services.appinfo.accessibilityEnabled,

Shouldn't this be `!Services.appinfo.accessibilityEnabled`? In that we don't want to enable the current CM with accessibility tools.
Attachment #8994479 - Flags: review?(bgrinstead)
Comment on attachment 8994480 [details]
Bug 1470922 - Make sure jsterm tests run in both old and new jsterm; .

https://reviewboard.mozilla.org/r/259034/#review266070
Attachment #8994480 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #8)
> 
> Shouldn't this be `!Services.appinfo.accessibilityEnabled`? In that we don't
> want to enable the current CM with accessibility tools.

Right 🤦‍♀️

Marco, hold on, I'll pushed a correct version this time
Yup, works. :)
Comment on attachment 8994479 [details]
Bug 1470922 - Enable CodeMirror JSTerm on Nightly except for people using accessible technology; .

https://reviewboard.mozilla.org/r/259032/#review266098

LGTM. Uber nit: these changesets should be reversed in order so that the tests will still run in both modes in case of bisection.
Attachment #8994479 - Flags: review?(bgrinstead) → review+
(In reply to Marco Zehe (:MarcoZ) from comment #13)
> Yup, works. :)

Great !
(In reply to Brian Grinstead [:bgrins] from comment #14)
> Comment on attachment 8994479 [details]
> Bug 1470922 - Enable CodeMirror JSTerm on Nightly except for people using
> accessible technology; .
> 
> https://reviewboard.mozilla.org/r/259032/#review266098
> 
> LGTM. Uber nit: these changesets should be reversed in order so that the
> tests will still run in both modes in case of bisection.

Sure. I'll update the review, push to TRY and then land if there's no failure.
Thank you Brian & Marco
mh, I see quite a few failures in the TRY push. Let me see what's happening before review Brian
Attachment #8994821 - Flags: review?(bgrinstead)
Depends on: 1478410
Comment on attachment 8994822 [details]
Bug 1470922 - Fix tests failing with the new Jsterm; .

https://reviewboard.mozilla.org/r/259362/#review266698
Attachment #8994822 - Flags: review?(bgrinstead) → review+
Attachment #8994821 - Attachment is obsolete: true
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb2a99260093
Make sure jsterm tests run in both old and new jsterm; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/c30835ae0523
Enable CodeMirror JSTerm on Nightly except for people using accessible technology; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/57b9025b5573
Fix tests failing with the new Jsterm; r=bgrins.
Backed out 3 changesets (bug 1470922) for devtools failures at devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js

Backout: https://hg.mozilla.org/integration/autoland/rev/3b5e11e7817ea017aa26fd06a1689c6937ed8845

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=57b9025b55730c919dd77338b97eee2293c739df

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190556371&repo=autoland&lineNumber=1752

16:00:49     INFO -  266 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js | Split console is shown. -
16:00:49     INFO -  267 INFO Waiting for pause.
16:00:49     INFO -  268 INFO Generating mouse click in tab.
16:00:49     INFO -  Buffered messages logged at 15:59:24
16:00:49     INFO -  269 INFO Console message: [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
16:00:49     INFO -  See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
16:00:49     INFO -  Date: Fri Jul 27 2018 15:59:20 GMT+0000 (Coordinated Universal Time)
16:00:49     INFO -  Full Message: TypeError: jsterm.inputNode is null
16:00:49     INFO -  Full Stack: test/testConsole<@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js:67:5
16:00:49     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:41
16:00:49     INFO -  promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
16:00:49     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
16:00:49     INFO -  promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
16:00:49     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
16:00:49     INFO -  TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
16:00:49     INFO -  asyncFunction@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:246:14
16:00:49     INFO -  test/<@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js:29:5
16:00:49     INFO -  process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:928:23
16:00:49     INFO -  walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
16:00:49     INFO -  Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:11
16:00:49     INFO -  schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
16:00:49     INFO -  completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:713:7
16:00:49     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:315:13
16:00:49     INFO -  resolve@resource://devtools/shared/base-loader.js -> resource://devtools/shared/deprecated-sync-thenables.js:38:40
16:00:49     INFO -  then@resource://devtools/shared/base-loader.js -> resource://devtools/shared/deprecated-sync-thenables.js:18:43
16:00:49     INFO -  then@resource://devtools/shared/base-loader.js -> resource://devtools/shared/deprecated-sync-thenables.js:56:9
16:00:49     INFO -  _handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
16:00:49     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
16:00:49     INFO -  resolve@resource://devtools/shared/base-loader.js -> resource://devtools/shared/deprecated-sync-thenables.js:38:40
16:00:49     INFO -  then@resource://devtools/shared/base-loader.js -> resource://devtools/shared/deprecated-sync-thenables.js:18:43
16:00:49     INFO -  resolve@resource://devtools/shared/base-loader.js -> resource://devtools/shared/deprecated-sync-thenables.js:73:11
16:00:49     INFO -  onEvent@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/head.js:376:7
16:00:49     INFO -  emit@resource://devtools/shared/base-loader.js -> resource://devtools/shared/event-emitter.js:178:37
16:00:49     INFO -  emit@resource://devtools/shared/base-loader.js -> resource://devtools/shared/event-emitter.js:255:29
16:00:49     INFO -  _renderSourceText/<@chrome://devtools/content/debugger/debugger-view.js:551:7
16:00:49     INFO -  setTimeout handler*_renderSourceText@chrome://devtools/content/debugger/debugger-view.js:550:5
16:00:49     INFO -  renderSourceText@chrome://devtools/content/debugger/debugger-view.js:476:5
16:00:49     INFO -  onReducerEvents/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/debugger/content/utils.js:43:7
16:00:49     INFO -  subscribeToStore/</</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:93:17
16:00:49     INFO -  subscribeToStore/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:92:15
16:00:49     INFO -  subscribeToStore/<@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/redux/non-react-subscriber.js:90:11
16:00:49     INFO -  dispatch@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/redux.js:416:7
16:00:49     INFO -  waitUntilService/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/redux/middleware/wait-service.js:59:20
16:00:49     INFO -  promiseMiddleware/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/redux/middleware/promise.js:15:14
16:00:49     INFO -  " {file: "chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js" line: 67 column: 0 source: "67"}]
16:00:49     INFO -  Buffered messages logged at 16:00:04
16:00:49     INFO -  270 INFO Longer timeout required, waiting longer...  Remaining timeouts: 1
16:00:49     INFO -  Buffered messages finished
16:00:49    ERROR -  271 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js | Test timed out -
16:00:49     INFO -  272 INFO Removing tab.
16:00:49     INFO -  273 INFO Tab removed and finished closing.
16:00:49     INFO -  274 INFO finish() was called, cleaning up...
16:00:49     INFO -  275 INFO Forcing GC/CC after debugger test.
16:00:50     INFO -  Not taking screenshot here: see the one that was previously logged
16:00:50    ERROR -  276 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js | A promise chain failed to handle a rejection: TypeError: jsterm.inputNode is null - stack: test/testConsole<@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js:67:5
16:00:50     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:41
16:00:50     INFO -  promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
16:00:50     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
16:00:50     INFO -  promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
16:00:50     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
16:00:50     INFO -  TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
16:00:50     INFO -  asyncFunction@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:246:14
16:00:50     INFO -  test/<@chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js:29:5
16:00:50     INFO -  process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:928:23
16:00:50     INFO -  walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
16:00:50     INFO -  Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:11
16:00:50     INFO -  schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
16:00:50     INFO -  completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:713:7
16:00:50     INFO -  _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:315:13
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44fd3a35a89a
Make sure jsterm tests run in both old and new jsterm; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/56eb2c6930b0
Enable CodeMirror JSTerm on Nightly except for people using accessible technology; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/d01677a667bc
Fix tests failing with the new Jsterm; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/44fd3a35a89a
https://hg.mozilla.org/mozilla-central/rev/56eb2c6930b0
https://hg.mozilla.org/mozilla-central/rev/d01677a667bc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1479797
Depends on: 1482875
Depends on: 1481443
Blocks: 1491776
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: