Closed Bug 1463669 Opened 2 years ago Closed Last year

Evaluated code in the output should be syntax highlighted

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox62 wontfix, firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: nchevobbe, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [boogaloo-mvp])

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1458831
Severity: normal → enhancement
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [boogaloo-mvp]
Product: Firefox → DevTools
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?
(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
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: P2 → P1
When you enter JS into the console, we can now syntax highlight it in
the output when CodeMirror is enabled.
Attachment #8996019 - Attachment is obsolete: true
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.
Attachment #9002616 - Flags: review+
For some reason, the test is failing on Linux
(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
Yeah, I can confirm the test fails when running all of the webconsole/ mochitests at once. Will look into it.
Narrowed this down to a failure only when it runs alongside browser_console_consolejsm_output.js.
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.
Doing `await pushPref("accessibility.force_disabled", 1);` in browser_console_consolejsm_output.js seems to do the trick. Awaiting try results.
Blocks: 1485510
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
(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 ?
https://hg.mozilla.org/mozilla-central/rev/360245d5c4a8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(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?
Flags: needinfo?(yzenevich)
(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.
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
(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?
(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
Flags: needinfo?(surkov.alexander)

Added a statement saying that the input from the command line and the output are syntax highlighted as appropriate.

Also adding a note about the fact that syntax highlighting will not be visible if Accessibility features have been enabled.

Flags: needinfo?(bgrinstead)

(In reply to Irene Smith from comment #21)

Added a statement saying that the input from the command line and the output are syntax highlighted as appropriate.

Also adding a note about the fact that syntax highlighting will not be visible if Accessibility features have been enabled.

Thanks Irene. Linking to the section here for future reference: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/The_command_line_interpreter#Defining_variables.

Flags: needinfo?(bgrinstead)
You need to log in before you can comment on or make changes to this bug.