Nicolas Chevobbe [:nchevobbe] Reporter
	Description • 6 years ago

In Bug 983473, we'll have syntax highlighting in the JsTerm, which is great.
What would be even neater would be that the message that we print to the output should be higlighted too (at the moment, we only print the string in black).

This could be doable by asking the CodeMirror instance to highlight the evaluated string and putting it in the Message. The only thing I'm unsure of is how to pluck the generated html by CodeMirror into the React Message component without using innerHTML.

	Brian Grinstead [:bgrins] Assignee
	Comment 1 • 6 years ago

There's a runmode addon that lets you syntax highlight without using editor directly: https://codemirror.net/demo/runmode.html. It has an API that takes a string and a DOM node, so the nice part is we wouldn't have to read innerHTML of the editor and could potentially use it on other JS strings (like if you ran `console.log(function foo() {}.toSource());`);

Still not quite sure how that would integrate with react - maybe it should be a Custom Element so that React ignores what happens inside of it and we could transform it into highlighted text in the connectedCallback?

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 6 years ago

(In reply to Brian Grinstead [:bgrins] from comment #1)
> There's a runmode addon that lets you syntax highlight without using editor
> directly: https://codemirror.net/demo/runmode.html. It has an API that takes
> a string and a DOM node, so the nice part is we wouldn't have to read
> innerHTML of the editor and could potentially use it on other JS strings
> (like if you ran `console.log(function foo() {}.toSource());`);

Looks like the debugger uses runMode with dangerouslySetInnerHTML for syntax highlighting breakpoint text: https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/devtools/client/debugger/new/src/components/SecondaryPanes/Breakpoints/Breakpoint.js#138-153,179

	Brian Grinstead [:bgrins] Assignee
	Comment 4 • 6 years ago

When you enter JS into the console, we can now syntax highlight it in
the output when CodeMirror is enabled.

	Nicolas Chevobbe [:nchevobbe] Reporter
	Comment 5 • 6 years ago

Comment on attachment 9002616 [details]
Bug 1463669 - Enable syntax highlighting of input in the console output when possible;r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

	Nicolas Chevobbe [:nchevobbe] Reporter
	Comment 6 • 6 years ago

Pushed to TRY https://treeherder.mozilla.org/#/jobs?repo=try&revision=10952a489fa1bc80106a2ec9cf4386cf70edd80c

	Nicolas Chevobbe [:nchevobbe] Reporter
	Comment 7 • 6 years ago

For some reason, the test is failing on Linux

	Brian Grinstead [:bgrins] Assignee
	Comment 8 • 6 years ago

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> For some reason, the test is failing on Linux

It's failing on all platforms (https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfdda301685e02fb70589c899b717274c92934f7). Weird thing is it passes test-verify, so I'm thinking it must be related to running with some other test in the chunk: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30208137f6ccd6e2d3affd2b2d3d12a44e04f8d0

	Brian Grinstead [:bgrins] Assignee
	Comment 9 • 6 years ago

Yeah, I can confirm the test fails when running all of the webconsole/ mochitests at once. Will look into it.

	Brian Grinstead [:bgrins] Assignee
	Comment 10 • 6 years ago

Narrowed this down to a failure only when it runs alongside browser_console_consolejsm_output.js.

	Brian Grinstead [:bgrins] Assignee
	Comment 11 • 6 years ago

Interesting - somehow CM is being disabled once we `console.dir(document)` at https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/devtools/client/webconsole/test/mochitest/browser_console_consolejsm_output.js#36. Somehow this makes Services.appinfo.accessibilityEnabled=true so even when we flip the pref for the test it's not using CM.

For some reason, if we switch it to console.log(document) it doesn't end up flipping that value.

	Brian Grinstead [:bgrins] Assignee
	Comment 12 • 6 years ago

Doing `await pushPref("accessibility.force_disabled", 1);` in browser_console_consolejsm_output.js seems to do the trick. Awaiting try results.

	Brian Grinstead [:bgrins] Assignee
	Comment 13 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eef46bc8c87ab4fb44b2edb232dd2de022971897

	Pulsebot
	Comment 14 • 6 years ago

Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/360245d5c4a8
Enable syntax highlighting of input in the console output when possible;r=nchevobbe

	Nicolas Chevobbe [:nchevobbe] Reporter
	Comment 15 • 6 years ago

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Interesting - somehow CM is being disabled once we `console.dir(document)`
> at
> https://searchfox.org/mozilla-central/rev/
> 3fa761ade83ed0b8ab463acb057c2cf0b104689e/devtools/client/webconsole/test/
> mochitest/browser_console_consolejsm_output.js#36. Somehow this makes
> Services.appinfo.accessibilityEnabled=true so even when we flip the pref for
> the test it's not using CM.
> 
> For some reason, if we switch it to console.log(document) it doesn't end up
> flipping that value.

Do you find why it makes Services.appinfo.accessibilityEnabled true ?

	Raul Gurzau (:RaulG)
	Comment 16 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/360245d5c4a8

	Brian Grinstead [:bgrins] Assignee
	Comment 17 • 6 years ago

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > Interesting - somehow CM is being disabled once we `console.dir(document)`
> > at
> > https://searchfox.org/mozilla-central/rev/
> > 3fa761ade83ed0b8ab463acb057c2cf0b104689e/devtools/client/webconsole/test/
> > mochitest/browser_console_consolejsm_output.js#36. Somehow this makes
> > Services.appinfo.accessibilityEnabled=true so even when we flip the pref for
> > the test it's not using CM.
> > 
> > For some reason, if we switch it to console.log(document) it doesn't end up
> > flipping that value.
> 
> Do you find why it makes Services.appinfo.accessibilityEnabled true ?

My best guess was that there's some property on the document that turns on accessibility when it gets inspected (why `console.dir` causes the change but `console.log` doesn't). Looking again, I think it's document.accessibleNode. If I paste this into the Browser Console:

```
console.log("Before logging document.accessibleNode", Services.appinfo.accessibilityEnabled);
console.log(document.accessibleNode);
setTimeout(() => {
  console.log("After logging document.accessibleNode", Services.appinfo.accessibilityEnabled);
}, 1000)
```

Then the first log is false and the second is true. Yura, is this expected behavior when accessing the `accessibleNode` property? And if so, is there a way we could somehow prevent this from happening when the console is the one accessing the property?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 18 • 6 years ago

(In reply to Brian Grinstead [:bgrins] from comment #17)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> > (In reply to Brian Grinstead [:bgrins] from comment #11)
> > > Interesting - somehow CM is being disabled once we `console.dir(document)`
> > > at
> > > https://searchfox.org/mozilla-central/rev/
> > > 3fa761ade83ed0b8ab463acb057c2cf0b104689e/devtools/client/webconsole/test/
> > > mochitest/browser_console_consolejsm_output.js#36. Somehow this makes
> > > Services.appinfo.accessibilityEnabled=true so even when we flip the pref for
> > > the test it's not using CM.
> > > 
> > > For some reason, if we switch it to console.log(document) it doesn't end up
> > > flipping that value.
> > 
> > Do you find why it makes Services.appinfo.accessibilityEnabled true ?
> 
> My best guess was that there's some property on the document that turns on
> accessibility when it gets inspected (why `console.dir` causes the change
> but `console.log` doesn't). Looking again, I think it's
> document.accessibleNode. If I paste this into the Browser Console:
> 
> ```
> console.log("Before logging document.accessibleNode",
> Services.appinfo.accessibilityEnabled);
> console.log(document.accessibleNode);
> setTimeout(() => {
>   console.log("After logging document.accessibleNode",
> Services.appinfo.accessibilityEnabled);
> }, 1000)
> ```
> 
> Then the first log is false and the second is true. Yura, is this expected
> behavior when accessing the `accessibleNode` property? And if so, is there a
> way we could somehow prevent this from happening when the console is the one
> accessing the property?

Yes accessing accessibleNode would enable accessibility services. However this API is under a pref ("accessibility.AOM.enabled") and is not available by default. 

Update: I just noticed that this api is now enabled for privileged context (bug 1410482). My understanding is that the reflection on document happens from privileged JS context in the devtools server side and thus we access the accessibleNode object. Alex poses an issue since, afaik, devtools access to it (be that console or otherwise) is always done from context that is privilged.

	Brian Grinstead [:bgrins] Assignee
	Comment 19 • 6 years ago

(In reply to Yura Zenevich [:yzen] from comment #18)
> (In reply to Brian Grinstead [:bgrins] from comment #17)
> Update: I just noticed that this api is now enabled for privileged context
> (bug 1410482). My understanding is that the reflection on document happens
> from privileged JS context in the devtools server side and thus we access
> the accessibleNode object. Alex poses an issue since, afaik, devtools access
> to it (be that console or otherwise) is always done from context that is
> privilged.

In the Browser Console case, document itself is a chrome priv object, so I'm not surprised that it's visible. In the web console case, even though the devtools object inspection code is chrome priv I expect that the Debugger.Object inspection should show only what the web page is allowed to see. We should confirm this though - is inspecting `document` in a web page and confirming we don't see accessibleNode enough?

	alexander :surkov (:asurkov)
	Comment 20 • 6 years ago

(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Yura Zenevich [:yzen] from comment #18)
> > (In reply to Brian Grinstead [:bgrins] from comment #17)
> > Update: I just noticed that this api is now enabled for privileged context
> > (bug 1410482). My understanding is that the reflection on document happens
> > from privileged JS context in the devtools server side and thus we access
> > the accessibleNode object. Alex poses an issue since, afaik, devtools access
> > to it (be that console or otherwise) is always done from context that is
> > privilged.
> 
> In the Browser Console case, document itself is a chrome priv object, so I'm
> not surprised that it's visible. In the web console case, even though the
> devtools object inspection code is chrome priv I expect that the
> Debugger.Object inspection should show only what the web page is allowed to
> see.

I would say so.

> We should confirm this though - is inspecting `document` in a web page
> and confirming we don't see accessibleNode enough?

technically yes, but cc'ing Olli to double check it