Will Bamberg [:wbamberg] Assignee
	Description • 9 years ago

The MDN tooltip that's being done in bug 980006 should syntax-highlight code examples.

We'll probably use the CSS tokenizer being done in bug 1152033.

	Will Bamberg [:wbamberg] Assignee
	Comment 1 • 9 years ago

Here's a patch that adds syntax highlighting using the CSS tokenizer. It's the simplest approach I could come up with, but it's not the most efficient, because it creates a separate node for every token (this seems to be around 30-70 nodes per property, depending on the code sample).

It would be more efficient to combine tokens that have the same syntax highlighting, and if you did that you'd probably get something more like 15-40 nodes per property. But the code would be more complex.

What do you think?

	Tim Nguyen :ntim
	Comment 2 • 9 years ago

(In reply to Will Bamberg [:wbamberg] from comment #1)
> Created attachment 8605077 [details] [diff] [review]
> bug1154469.patch
> 
> Here's a patch that adds syntax highlighting using the CSS tokenizer. It's
> the simplest approach I could come up with, but it's not the most efficient,
> because it creates a separate node for every token (this seems to be around
> 30-70 nodes per property, depending on the code sample).
> 
> It would be more efficient to combine tokens that have the same syntax
> highlighting, and if you did that you'd probably get something more like
> 15-40 nodes per property. But the code would be more complex.
> 
> What do you think?

You can just initiate a readonly Editor instance for each code block.

	Will Bamberg [:wbamberg] Assignee
	Comment 3 • 9 years ago

(In reply to Tim Nguyen [:ntim] (limited availability) from comment #2)
> (In reply to Will Bamberg [:wbamberg] from comment #1)
> > Created attachment 8605077 [details] [diff] [review]
> > bug1154469.patch
> > 
> > Here's a patch that adds syntax highlighting using the CSS tokenizer. It's
> > the simplest approach I could come up with, but it's not the most efficient,
> > because it creates a separate node for every token (this seems to be around
> > 30-70 nodes per property, depending on the code sample).
> > 
> > It would be more efficient to combine tokens that have the same syntax
> > highlighting, and if you did that you'd probably get something more like
> > 15-40 nodes per property. But the code would be more complex.
> > 
> > What do you think?
> 
> You can just initiate a readonly Editor instance for each code block.

Using Code Mirror, you mean? I looked into this back in bug 980006 (see bug 980006, comment 27), and didn't use it, mainly because the code samples in the CSS pages aren't complete valid CSS, because they don't include selectors. So CM wouldn't parse them in the way we would want.

Also, I don't know the cost of initializing a CM instance, but it feels like overkill for syntax-highlighting a few lines of CSS. But of course I've not measured this.

I do agree, though, that we ought to have a common way to highlight code across all the tools. At the moment the highlighting is different in editors and in places like the rule view.

	Patrick Brosset <:pbro>
	Comment 4 • 9 years ago

Comment on attachment 8605077 [details] [diff] [review]
bug1154469.patch

Review of attachment 8605077 [details] [diff] [review]:
-----------------------------------------------------------------

This looks nice. I was about to say we should move this into a shared module so that it can be reused, but since it's quite specific to the code samples returned by mdn right now, we don't have to do this yet.

I did find one problem, see this screenshot: https://dl.dropboxusercontent.com/u/714210/bug1154469-syntax-highlighting.png
Perhaps we should be using the line breaks as a hint that the next ident is a property name, because not all syntax examples end with ;

Also, maybe we could use this parsing phase to "prettify" the css a little bit by removing extra whitespaces and adding semicolon where they're missing.

Other than this, I like the approach, it makes sense.
Only a few nits and comments in the code below.

::: browser/devtools/shared/widgets/MdnDocsWidget.js
@@ +66,5 @@
> + * of all other types ("number", "url", "percentage", ...) are considered
> + * to be part of a property value, and are appended as SPAN nodes with
> + * a different color class.
> + *
> + * @param {doc} document

invalid jsdoc syntax, should be:
@param {Document} doc

@@ +69,5 @@
> + *
> + * @param {doc} document
> + * Used to create nodes.
> + *
> + * @param {syntaxText} string

invalid jsdoc syntax, should be:
@param {String} syntaxText

@@ +70,5 @@
> + * @param {doc} document
> + * Used to create nodes.
> + *
> + * @param {syntaxText} string
> + * The CSS input. This is assumed to consist of a series of 

nit: trailing whitespace

@@ +73,5 @@
> + * @param {syntaxText} string
> + * The CSS input. This is assumed to consist of a series of 
> + * CSS declarations, with trailing semicolons.
> + *
> + * @param {syntaxSection} DOM node

invalid jsdoc syntax, should be:
@param {DOMNode} syntaxSection

@@ +100,5 @@
> +  function updateIdentClass(tokenText) {
> +    if (tokenText === ":") {
> +      identClass = PROPERTY_VALUE_COLOR;
> +    }
> +    else if (tokenText === ";") {

nit: else if on the same line as the closing if }

@@ +119,5 @@
> +      case "symbol":
> +        updateIdentClass(tokenText);
> +        return doc.createTextNode(tokenText);
> +      case "whitespace":
> +      case "comment":

I think comment nodes should be treated as ident nodes and given the css class "COMMENT_COLOR" so they appear in grey, as they should.

@@ +123,5 @@
> +      case "comment":
> +        return doc.createTextNode(tokenText);
> +      default:
> +        return createStyledNode(tokenText, PROPERTY_VALUE_COLOR);
> +      }

nit: } should be indented with 4 spaces only.

@@ +127,5 @@
> +      }
> +  }
> +
> +  let identClass = PROPERTY_NAME_COLOR;
> +  let lexer = DOMUtils.getCSSLexer(syntaxText);

nit: can you move these 2 variable declarations to the top of populatePropertySyntax, before the createStyledNode helper?

@@ +250,4 @@
>      linkToMdn: tooltipDocument.getElementById("visit-mdn-page")
>    };
>  
> +  this.doc = tooltipDocument;

This makes me realize we don't have a destroy method in the MdnDocsWidget class. We should.
Can you add the method:

destroy: function() {
  this.elements = null;
  this.doc = null;
}

@@ +307,5 @@
>  
>        // clear docs summary and syntax
>        elements.summary.textContent = "";
> +      while (elements.syntax.firstChild) {
> +        elements.syntax.removeChild(elements.syntax.firstChild);

Or: elements.syntax.firstChild.remove();

	Patrick Brosset <:pbro>
	Comment 5 • 9 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> I think comment nodes should be treated as ident nodes and given the css
> class "COMMENT_COLOR" so they appear in grey, as they should.
Sorry, I meant the "theme-comment" class.

	Will Bamberg [:wbamberg] Assignee
	Comment 6 • 9 years ago

Thanks for taking a look Patrick.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> Comment on attachment 8605077 [details] [diff] [review]
> bug1154469.patch
> 
> Review of attachment 8605077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks nice. I was about to say we should move this into a shared module
> so that it can be reused, but since it's quite specific to the code samples
> returned by mdn right now, we don't have to do this yet.
> 
> I did find one problem, see this screenshot:
> https://dl.dropboxusercontent.com/u/714210/bug1154469-syntax-highlighting.png
> Perhaps we should be using the line breaks as a hint that the next ident is
> a property name, because not all syntax examples end with ;


I did see this, but didn't do anything about it because:

* as far as I know, this code sample isn't valid CSS
* this code sample is wrong according to the style guide we have for CSS pages (which specifically mandate that lines should end in a semicolon)

So I feel that changing the code in this case would be making the code more complex in order to address deficiencies that ought to be fixed in the content. Given that we are dealing with a Wiki, there's always the possibility that it could contain garbage, and in general I think we should address this by fixing the content, not by special-casing the code.

Also, as far as I know it is permitted to split a CSS property value over a line break, like:

border: 2px
solid green;

... so if we treat a line break as a hint that we should be looking for a property name next, we will incorrectly highlight this valid CSS.

> 
> Also, maybe we could use this parsing phase to "prettify" the css a little
> bit by removing extra whitespaces and adding semicolon where they're missing.
> 

Again, I'm a bit reluctant to make the code cleverer (=more fragile) to address problems we can fix in content.

But, that's just my thought process here, and if I haven't convinced you, then I'll happily go ahead and make these changes.

	Patrick Brosset <:pbro>
	Comment 7 • 9 years ago

(In reply to Will Bamberg [:wbamberg] from comment #6)
> Also, as far as I know it is permitted to split a CSS property value over a
> line break, like:
> 
> border: 2px
> solid green;
> 
> ... so if we treat a line break as a hint that we should be looking for a
> property name next, we will incorrectly highlight this valid CSS.
You're right, I thought about this too, this may be a problem often for long values like linear-gradient background-images or multiple box-shadows.

I agree with you, let's go with the simple implementation for now and focus on fixing the content when needed.

	Tim Nguyen :ntim
	Comment 8 • 9 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> Comment on attachment 8605077 [details] [diff] [review]
> bug1154469.patch
> 
> Review of attachment 8605077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks nice. I was about to say we should move this into a shared module
> so that it can be reused, but since it's quite specific to the code samples
> returned by mdn right now, we don't have to do this yet.
Actually, it may be nice to do so, there are some places that probably could get syntax highlighting, I'm thinking particularly of the code snippets in the fonts inspector panel.

	Will Bamberg [:wbamberg] Assignee
	Comment 9 • 9 years ago

Here's a new patch, with (I hope) your comments addressed, and some tests.

I do agree that we should have a separate module for syntax highlighting, but would like to land a patch for this, then maybe we could use it as the basis for a more general patch. But whichever you prefer.

	Patrick Brosset <:pbro>
	Comment 10 • 9 years ago

Comment on attachment 8627514 [details] [diff] [review]
syntax-highlighter.diff

Review of attachment 8627514 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Thanks!

::: browser/devtools/shared/test/head.js
@@ +333,5 @@
> +    info("Check that node has the expected type");
> +    if (expected.type == "text") {
> +      ok(actual.nodeType == 3, "Check that node is a text node");
> +    }
> +    else {

Please put else on the same line as the closing } for if.

::: browser/devtools/shared/widgets/MdnDocsWidget.js
@@ +78,5 @@
> + * @param {DOM node} syntaxSection
> + * This is the parent for the output nodes. Generated nodes
> + * are appended to this as children.
> + */
> +function syntaxHighlightCSS(doc, syntaxText, syntaxSection) {

I would try and make this function a little bit more generic so it can potentially be moved more easily to a shared place in the future. By more generic I mean renaming slightly the function and reducing the number of arguments:

function appendSyntaxHighlightedCSS(cssText, parentEl) {
  let doc = parentEl.ownerDocument;

  ...
}

	Will Bamberg [:wbamberg] Assignee
	Comment 11 • 9 years ago

I've made your changes and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc729715480.

I made one other change, in the test code, to fix an e10s-only test failure:

In browser_mdn-docs-03.js, add_task():

-  let doc = gBrowser.selectedBrowser.contentDocument;
+  let doc = gBrowser.selectedTab.ownerDocument;

https://hg.mozilla.org/try/file/4fc729715480/browser/devtools/shared/test/browser_mdn-docs-03.js#l266

	Will Bamberg [:wbamberg] Assignee
	Comment 12 • 9 years ago

Try is green, except for an intermittent which did not show up when I retriggered, and one that did:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc729715480

The one that did show up again is pinned as bug 1177730, and if I read that right, this bug was fixed yesterday afternoon by skipping the test, but the build I have missed that commit. So I think it ought to be OK to request checkin for my patch.

Does that sound OK Patrick?

	Will Bamberg [:wbamberg] Assignee
	Comment 14 • 9 years ago

After talking/thinking it over, requesting checkin.

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/e4aa08f60a6c

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e4aa08f60a6c