980006 - Add MDN tooltips for CSS properties

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Will Bamberg [:wbamberg]]

We could add tooltips to the Inspector's Rules view, that shows summary documentation from MDN for CSS properties, and includes a link to visit the full page on MDN.

There's some previous discussion here: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/zOvAau_6x7k

And a demo here: https://github.com/wbamberg/devtools-mdn-tooltips.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Copied from my message to the devtools group ( which for some reason isn't on google ), the requirements I see are:

1. that the content is up-to-date, eg we don't ship inaccurate content with older versions of Firefox ( so building content into Firefox that we cannot update is a non-starter )

2. that we build in some way to track usage so we can measure in the aggregate how many people use this feature.

3. that some small piece of content on MDN can be identified by us as being good to include as pop-up content, and we can access it deterministically ( through both a path and a revision, so future revisions don't affect it )

4. that some collection of pop-up content can be queried by Firefox either when we build or on request from Firefox, and a serialised corpus of help info is cached in Firefox so that the feature works when I'm on a plane, or bad wifi. 

Personally I think the smart thing to do is concentrate on help text and leave code samples as a follow-up so we don't block on major MDN changes.

Comment 2

[Luke Crouch [:groovecoder]]

Also, aside from just how cool this would be ...

1. What, and whose, problem are we trying to solve?
2. How will we know if we succeed?

This will really help us understand the best way to implement.

Comment 3

[Will Bamberg [:wbamberg]]

(In reply to Luke Crouch [:groovecoder] from comment #2)

> 1. What, and whose, problem are we trying to solve?

Web developers who need some help with understanding how to use a particular CSS property.

If possible, we should be able to provide quick help 'inline', without breaking out of what they are doing. That's why it's presented in a tooltip rather than just loading the whole page. But sometimes people will need to know more than the tooltip can tell them, so it will need to contain a link to the complete page for those cases.

So the trick is to present enough useful information in the tooltip that a lot of the time people will not have to visit the page (and break out of what they are doing).

My feeling is that the tooltip ought to contain:

1) title of the thing (currently just CSS properties, but eventually other things too)
2) short summary
3) short, commented, code sample

For (2), we can get this directly using the MDN API like this: https://developer.mozilla.org/en-US/docs/Web/CSS/height?raw&summary. I think that for security reasons we want to treat this as text, not html. I think at the moment it isn't.

For (3), there's no way to get this directly. The closest thing is to get something like https://developer.mozilla.org/en-US/docs/Web/CSS/color?raw&section=Syntax and then parse the code sample (it's always after "Formal syntax").

But I'd like to be able to access code sample directly, as we can with summary. For example: https://developer.mozilla.org/en-US/docs/Web/CSS/height?raw&sample. That's easier, more explicit, and seems more reliable.

This would mean a change in the MDN content, to mark up these sections with an ID, and a change in Kuma to understand this and present it in the API.

On the content side, we'd also want to go through the code samples to make sure they're consistent and suitable for presentation in a tooltip, but we can do this incrementally after the machinery is in place to access the sample.

> 2. How will we know if we succeed?

I don't know the answer to this.

> This will really help us understand the best way to implement.

Comment 4

[Will Bamberg [:wbamberg]]

Attachment: 0001-Add-MDN-tooltips-for-CSS-properties.git.patch

Here's a patch that adds tooltips to the Inspector's Rules view.

The tooltips are shown on a context menu item click. The menu item is only shown when the target is a CSS property name in the Rules view. When you click the item, we fetch docs for the property from MDN and display it in a tooltip.

I think a context menu item is a much better (less potentially annoying) UI than display-on-hover.

I've tried to follow the pattern for the other tooltips as far as I could, but might have misunderstood things.

Having done this, I'd very much like to cache MDN lookups. Apart from enabling offline operation, it would be way more efficient (the docs don't change very often, so we could easily cache them for a week or so without much problem). This seems like it would be quite easy, except that I'm not sure which API is recommended for persisting data (see also https://bugzilla.mozilla.org/show_bug.cgi?id=943306#c14).

It would also be good to parse the examples so we can syntax-highlight them. But this is tricky because they are quite inconsistently written on MDN (also because it would be nice for the examples to include comments, and as far as I could see the "css-parsing-utils" doesn't handle comments).

I'm independently working on improving the MDN content.

Anyway, I'd love some feedback on whether this looks like a good approach, and how close it is to an actual review.

No tests, yet. We could have tests like:

* given a property, get the docs from MDN:
** where the page can be retrieved successfully and is properly formatted
** where the page can be retrieved successfully and isn't properly formatted
** where we couldn't retrieve the page (e.g. no connectivity)

* given a docs structure, create and initialize a tooltip and check that its contents are correct

* given a docs structure, create, initialize, and show a tooltip, and check it displays and its contents are correct

I'm not sure how to write tests that need connectivity, or how to write tests that test that a tooltip is actually shown, or that a menu item is shown in the right contexts. I can look at the existing devtools tests, but any pointers would be really welcome.

Comment 5

[Will Bamberg [:wbamberg]]

Some email feedback from pbrosset:

***

Here are some remarks, in no particular order:
- I really like the fact that this is an opt-in thing. I agree, display-on-hover would be too distracting.
- I like that this is something you turn on from within the ctx-menu. I can imagine having it too in the main settings panel, but for now it's fine there.
- However, it would be more logical to me if you didn't have to do this per property. Why not a global context-menu in the rule-view that turns this mode on for all properties? In fact, I realized that if you turn it on for the "color" property for instance, and then mouse over another property further down, it still shows the tooltip, but over the color property. I think this behavior may be a little weird.
- This is related to my previous point: it's good that the tooltip doesn't close when you move your mouse, because that allows for copy/pasting and clicking inside the popup, which is important, but that means we can't have the tooltip appear for any property on hover. So, what about this idea (and I guess we'd need some UX advice for this): when you choose MDN docs in the ctx-menu, then that makes little (?) icons appear next to all property names? And then you'd have to click on each of the icons to make the tooltip appear. In fact, I can even see this more globally, something you'd choose to enable from the settings tab, and when done, the help icons would appear anywhere there's an MDN tooltip we want to show. How does that sound? Would that be too much? We could keep this only for the rule-view to start with though.
- It's a good idea that you added a new type of tooltip for this (the new xhtml frame thing) because it has special formatting. Do you think we could make it a little more generic and call it mdn-docs-tooltip-frame.xhtml instead? I don't see why we couldn't use it for HTML or JS docs in the future (would the 2 section summary and syntax still be enough then?).
- Same remark for Tooltip.setCssDocsContent, it's great that you added that function, that will make it a lot easier to add mdn tooltips elsewhere in the devtools, but I would make it more generic again, call it setMdnDocsContent
- Same for CssDocsWidget.
- I would merge CssDocsWidget and mdn-docs.js into one file (even if keeping exports.getCssDocs = getCssDocs).

About the syntax highlighting, I suggest using codemirror for the code examples. It's good because it supports lot's of different languages and we already use in the devtools (for the debugger, markup-view and style-editor). See /browser/devtools/sourceeditor/editor.js. There is some jsdoc comment above the Editor class in this file.

When it comes to storage, there has been some discussions about this in the team lately, and yes, that comment you linked to is probably the right solution, but according to what I heard, there seems to at least be some gotchas. I suggest you talk to Brian Grinstead about this.

About tests, we never connect to external servers during tests, but the test system does start a local server we can use. So I suggest making BASE_MDN_PAGE something you can actually change at runtime, so that the first thing your tests would do is change it to the URL of the local test server.
I can give some pointers to existing tests.

Comment 6

[Will Bamberg [:wbamberg]]

(In reply to Will Bamberg [:wbamberg] from comment #5)
> Some email feedback from pbrosset:
> 

Thanks pbrosset!

> ***
> 
> Here are some remarks, in no particular order:
> - I really like the fact that this is an opt-in thing. I agree,
> display-on-hover would be too distracting.
> - I like that this is something you turn on from within the ctx-menu. I can
> imagine having it too in the main settings panel, but for now it's fine
> there.
> - However, it would be more logical to me if you didn't have to do this per
> property. Why not a global context-menu in the rule-view that turns this
> mode on for all properties? In fact, I realized that if you turn it on for
> the "color" property for instance, and then mouse over another property
> further down, it still shows the tooltip, but over the color property. I
> think this behavior may be a little weird.

I'm not sure I understand this. Are you talking about the behaviour in this patch, because I don't see that. If you click "show the docs" when over "color", then you get the popup, and until it is dismissed hovering over another property does nothing. Then after the popup is dismissed, if you mouse over a property, still nothing: the popup only reappears if you select the menu item, and then it's in the right place. Or am I missing something?

> - This is related to my previous point: it's good that the tooltip doesn't
> close when you move your mouse, because that allows for copy/pasting and
> clicking inside the popup, which is important, but that means we can't have
> the tooltip appear for any property on hover. So, what about this idea (and
> I guess we'd need some UX advice for this): when you choose MDN docs in the
> ctx-menu, then that makes little (?) icons appear next to all property
> names? And then you'd have to click on each of the icons to make the tooltip
> appear. In fact, I can even see this more globally, something you'd choose
> to enable from the settings tab, and when done, the help icons would appear
> anywhere there's an MDN tooltip we want to show. How does that sound? Would
> that be too much? We could keep this only for the rule-view to start with
> though.

This is a neat idea, although as you suggest, it might make the rule-view a bit cluttered. I'd welcome UX advice for sure, and wider feedback from users.

> - It's a good idea that you added a new type of tooltip for this (the new
> xhtml frame thing) because it has special formatting. Do you think we could
> make it a little more generic and call it mdn-docs-tooltip-frame.xhtml
> instead? I don't see why we couldn't use it for HTML or JS docs in the
> future (would the 2 section summary and syntax still be enough then?).
> - Same remark for Tooltip.setCssDocsContent, it's great that you added that
> function, that will make it a lot easier to add mdn tooltips elsewhere in
> the devtools, but I would make it more generic again, call it
> setMdnDocsContent
> - Same for CssDocsWidget.

I did consider doing this (calling them MDN instead of CSS) but I wasn't sure, as you say, whether the formatting of different sorts of content would be similar enough. But I can make this change now, and if we decide that they are not similar enough when we add more, we can think again.

(I think the next one if would be good to do would be Web APIs. I was going something with getDeviceStorage the other day and had to keep switching between the MDN page and the devtools.)

> - I would merge CssDocsWidget and mdn-docs.js into one file (even if keeping
> exports.getCssDocs = getCssDocs).

OK, thanks. I was keeping them separate because I thought someone might want to get the docs without necessarily wanting to put them in a tooltip. But as long as the function is a separate export then they can do this anyway.

> About the syntax highlighting, I suggest using codemirror for the code
> examples. It's good because it supports lot's of different languages and we
> already use in the devtools (for the debugger, markup-view and
> style-editor). See /browser/devtools/sourceeditor/editor.js. There is some
> jsdoc comment above the Editor class in this file.

I will take a look.

> When it comes to storage, there has been some discussions about this in the
> team lately, and yes, that comment you linked to is probably the right
> solution, but according to what I heard, there seems to at least be some
> gotchas. I suggest you talk to Brian Grinstead about this.

OK. Do you think persisting the doc fragments is needed in this patch, or can it be added as v2?

> About tests, we never connect to external servers during tests, but the test
> system does start a local server we can use. So I suggest making
> BASE_MDN_PAGE something you can actually change at runtime, so that the
> first thing your tests would do is change it to the URL of the local test
> server.

Yup, sounds good.

> I can give some pointers to existing tests.

Thanks!

Comment 7

[Will Bamberg [:wbamberg]]

(In reply to Will Bamberg [:wbamberg] from comment #6)
> (In reply to Will Bamberg [:wbamberg] from comment #5)
> > Some email feedback from pbrosset:
> > 
> 
> Thanks pbrosset!
> 
> > In fact, I realized that if you turn it on for
> > the "color" property for instance, and then mouse over another property
> > further down, it still shows the tooltip, but over the color property. I
> > think this behavior may be a little weird.
> 
> I'm not sure I understand this. Are you talking about the behaviour in this
> patch, because I don't see that. If you click "show the docs" when over
> "color", then you get the popup, and until it is dismissed hovering over
> another property does nothing. Then after the popup is dismissed, if you
> mouse over a property, still nothing: the popup only reappears if you select
> the menu item, and then it's in the right place. Or am I missing something?

Oh wait, I do see that now. it seems like I have to scroll for it to be apparent. Yes, that looks like a bug.

Comment 8

[Will Bamberg [:wbamberg]]

Comment on attachment 8565083 [details] [diff] [review]
0001-Add-MDN-tooltips-for-CSS-properties.git.patch

bgrins, it would be great to have your feedback on what would be a good UI for this thing. Options we have mentioned so far:

* what's implemented now (apart from some bug on hover): a context menu item click for each property lookup

* display the tooltip on hover, like the other tooltips, but I think noone likes this

* have a setting toggleable in the context menu or in devtools settings, that makes the tooltip display on hover

* have a setting toggleable in the context menu or in devtools settings, that adds a little icon next to property names, that shows the tooltip when you click it (if I understand correctly)

Display-on-hover is maybe problematic for this tooltip, since it could take a second to fetch the content, and I feel like display-on-hover works better when you can display the content instantly.

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Will Bamberg [:wbamberg] from comment #8)
...
> * have a setting toggleable in the context menu or in devtools settings,
> that adds a little icon next to property names, that shows the tooltip when
> you click it (if I understand correctly)

I think I prefer this option, but I'm unsure if it is dramatically better than the current patch. I think doing this on hover is potentially a poor user experience, especially with poor networks or offline.

> Display-on-hover is maybe problematic for this tooltip, since it could take
> a second to fetch the content, and I feel like display-on-hover works better
> when you can display the content instantly.

I'd love to have some form of offline caching for this feature.

Comment 10

[Tim Nguyen :ntim]

When I originally tried the demo addon, the tooltip really annoyed me. If this is going to be implemented, please be smart about how the tooltip will appear, or please add an option to disable those tooltips.

Comment 11

[Patrick Brosset <:pbro>]

UX behavior:

It looks like so far, everyone agrees that this should be a help mode you need to turn on rather than being always available.
As for the show-on-hover behavior, it's problematic because:
- if it automatically shows and hides as you move your mouse over properties, that means it's hard to click in the tooltip or select text in it,
- if it shows on hover, but then stays visible until you click outside, it's better, but may still be annoying for people that wanted to have the help mode ON just for one property, because they will keep on seeing hard to dismiss tooltips as soon as they move their mouse,
- if it shows on hover, then stays visible until you click outside, *and* doesn't come back unless you re-activate the help mode again (which is what's in your patch Will), then it's better yet, but will be annoying for people that wanted help for more than just 1 property :)
So it looks like if it was a help mode you active and de-activate globally via right-click and, once activated, showed help icons next to all properties, this would solve these issues.
The help icons would be hidden by default. People wanting to learn about properties now and then would make them visible via right-click, and would then be able to click next to any of the properties they need info about.
NI? shorlander to get some UX advice about this.

Caching:

I do agree this is important, but let's keep this for a separate patch (or follow-up bug). We can even decide that this feature is hidden behind a pref until we have caching in place, if we want to.
There's enough work to do in the patch as it is.

Will, I'll review the patch you attached.

Comment 12

[Tim Nguyen :ntim]

I think a context menu item is good enough. It's not annoying and doesn't need a help mode.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8565083 [details] [diff] [review]
0001-Add-MDN-tooltips-for-CSS-properties.git.patch

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

Nice work Will.
Apart from the comments I made by email already (comment 5, especially about making the Css thing more generic), I have made a few more below.
There are some things you can fix already, and some others we'd need some input from UX first.

::: browser/devtools/shared/mdn-docs.js
@@ +2,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I've got a couple of comments about this file:

- I like when there is a comment block at the top of a file, that explains what's the responsibility of the file, and maybe gives a short code usage example. That helps a lot when reading the file for the first time.
- I would refactor a little bit the getCssDocs function into 2 functions: getMdnDocs and getCssDocs. The more generic getMdnDocs would do the XHR bit and return a promise that resolves on onLoaded and rejects onError. The more specific getCssDocs would call that and chain a .then() to parse the document. The document parsing seems to be the part we can't share. 
If we do that, it's easy to create getHtmlDocs, getJsDocs, etc...

@@ +12,5 @@
> +
> +/**
> + * The following 3 functions are taken from MDN, and enable us to get
> + * the next sibling that is not a comment or whitespace.
> + */

Do you think there's a chance that we'll want to keep these functions updated with changes made on MDN?
If so, then the comment should say so.
If we don't really care about this (and since they are really low level DOM utility stuff, I'd say we don't), then I'd remove the comment and change their names to be camelcased, for more consistency.

@@ +14,5 @@
> + * The following 3 functions are taken from MDN, and enable us to get
> + * the next sibling that is not a comment or whitespace.
> + */
> +function is_all_ws( node ) {
> +  // Use ECMA-262 Edition 3 String and RegExp features

This comment doesn't seem useful.

@@ +33,5 @@
> +
> +/**
> + * Throw if the node is undefined or not what we expect.
> + */
> +function check(node, tagName) {

Not sure about the name and the fact that it throws.
Why not something like:

function hasTagName(node, tagName) {
  return node && node.tagName && node.tagName.toLowerCase() == tagName.toLowerCase();
}

@@ +34,5 @@
> +/**
> + * Throw if the node is undefined or not what we expect.
> + */
> +function check(node, tagName) {
> +  if (!node || node.tagName != tagName) {

I'd do
node.tagName.toLowerCase() != tagName.toLowerCase()
just in case. That makes the function easier to use.

@@ +45,5 @@
> + * element in the #Summary section of the document.
> + *
> + * It's expected to be a <P> element.
> + */
> +function getSummary(responseDocument) {

With the changes to the check function, the getSummary and getSyntax functions should be changed too. And instead of throwing, they could just return null.

@@ +94,5 @@
> + *
> + */
> +function getCssDocs(cssProperty) {
> +
> +  function _getCssDocs(resolve, reject) {

Using Promise.jsm promise implementation instead of DOM promises (as suggested below) will make this extra function scope useless, making the getCssDocs function less indented, easier to read.

@@ +106,5 @@
> +    xhr.responseType = "document";
> +    xhr.send();
> +
> +    function parseDocsFromResponse(responseDocument, resolve, reject) {
> +      try {

Again here, if you change the check function to return a boolean and getSummary and getSyntax to return a node or null, you'll need to change the code here. Remove the try catch.

@@ +128,5 @@
> +      reject("Error loading remote docs page at " + pageUrl + " : " + xhr.status);
> +    }
> +  }
> +
> +  return new Promise(_getCssDocs);

We discussed in the team recently about using DOM Promises [1] but decided against for the time being, because of the lack of tooling.
Also for consistency reasons, it's probably better to stick to using Promise.jsm (you can find usage examples all over the devtools code, here for instance [2]).

[1] https://groups.google.com/forum/#!searchin/mozilla.dev.developer-tools/Usage$20of$20DOM$20Promises/mozilla.dev.developer-tools/wljrsA3Pk6k/8SbccwmogoUJ
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#148

::: browser/devtools/shared/widgets/CssDocsWidget.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

As said in comment 5, I would merge this into mdn-docs.js, even if you keep on exporting the same symbols.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +function CssDocsWidget(tooltipDocument) {

We need a comment above this class' constructor. The term widget is very generic and used in many places, so you need to explain here what is the intended usage of this widget, and what it basically does.
It's important here because this widget can really only work in a special environment: within a document that has some expected DOM nodes.
In particular, you should mention the css-docs-frame.xhtml html page.

@@ +11,5 @@
> +module.exports.CssDocsWidget = CssDocsWidget;
> +
> +/**
> + * This is called just before the tooltip is displayed.
> + * It updates the tooltip's content from the cssDocs object.

Looks like this comment should be just before the updateUI function, not before the prototype definition.
Also, I think it should be rephrased something like: "Update the owner document's content with MDN docs data".

@@ +14,5 @@
> + * This is called just before the tooltip is displayed.
> + * It updates the tooltip's content from the cssDocs object.
> +*/
> +CssDocsWidget.prototype = {
> +  updateUI: function(cssDocs) {

Something like 
updateUI: function({propertyName, summary, syntax, url}) {...
makes it clearer what the function expects as arguments.
Also, maybe call the function loadDocs or something.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +861,5 @@
> +
> +  /**
> +   * Set up a CSS docs tooltip.
> +   *
> +   * This is called when the tooltip is first constructed.

This alone doesn't set up a tooltip. Consumers of this class first have to create the class, attach the tooltip somewhere, show it, and then set the content. They can do this either by calling tooltip.content = <some XUL/HTML content> or by using one of the set<Something>Content.

So a better comment would be:
"Set the content of this tooltip to the MDN docs widget"

@@ +875,5 @@
> +   * UI with new content each time the tooltip is shown.
> +   */
> +  setCssDocsContent: function() {
> +
> +    let def = promise.defer();

This will be the 3rd function in this class that needs to create an iframe, give it a size, load a URL, wait for the load event. Time to refactor a little bit.
Can you extract this part to a new function like:
setIframeContent({width, height}, url)
and make it return a promise that resolve to the inner content window of the iframe.

This way setColorPickerContent, setCubicBezierContent and setCssDocsContent will be able to do:

return this.setIframeContent({width, height}, url).then(win => {
  ...
  return widget;
});

@@ +894,5 @@
> +      // get the localized string for the link text
> +      let linkToMdn = win.document.getElementById("visit-mdn-page");
> +      linkToMdn.textContent = l10n.strings.GetStringFromName("docsTooltip.visitMDN");
> +
> +      // listen for clicks and open in the browser window instead

It looks to me like this functionality (mdn links click) should be handled by the widget, in the widget file. 
After all, the function here just creates an iframe, loads the mdn doc page in, creates a widget, and initialize it with the inner document.
It shouldn't really be concerned about a lot of the details of what's going on in the iframe.

@@ +1537,4 @@
>  });
>  
>  /**
> + * Tooltip for displaying docs for CSS properties from MDN.

Not sure this extra class is needed. We have similar classes for the color picker or the animation timing functions for example, but that's because they are sub classes of SwatchBasedEditorTooltip.
In fact, thinking about it, if we decide to have a help icon that you need to click on to display the tooltip, we could keep this class and make it inherit from SwatchBasedEditorTooltip too.
But with the current behavior, I don't think it's needed because it only wraps this:

let mdnTooltip = new Tooltip(doc, {
  consumeOutsideClick: true,
  closeOnKeys: [ESCAPE_KEYCODE, RETURN_KEYCODE],
  noAutoFocus: false 
});
this.widget = mdnTooltip.setCssDocsContent();

Which the consumer could call instead.

@@ +1552,5 @@
> +
> +module.exports.CssDocsTooltip = CssDocsTooltip;
> +
> +CssDocsTooltip.prototype = {
> +/**

nit: this comment block isn't indented properly.

::: browser/devtools/shared/widgets/css-docs.css
@@ +11,5 @@
> +  padding: 0;
> +}
> +
> +header {
> +  position: absolute;

It looks like both the header and #content don't really need the absolute positioning. footer needs to be at the bottom, so that's fine.
In fact, you could use flexbox here, to make sure the header is always at the top and has a fixed height, the footer always at the bottom and with a fixed height, and the #content takes the remaining available space nicely, with overflow:auto for scrolling.

::: browser/devtools/styleinspector/rule-view.js
@@ +26,4 @@
>  const PREF_UA_STYLES = "devtools.inspector.showUserAgentStyles";
>  const PREF_DEFAULT_COLOR_UNIT = "devtools.defaultColorUnit";
>  
> +const PROPERTY_NAME_CLASS = "ruleview-propertyname";

Can you make sure all occurrences of "ruleview-propertyname" are replaced with PROPERTY_NAME_CLASS in this file? I think there are 2 more in getNodeInfo and newProperty.

@@ +1250,4 @@
>      var showOrig = Services.prefs.getBoolPref(PREF_ORIG_SOURCES);
>      this.menuitemSources.setAttribute("checked", showOrig);
>  
> +    this.menuitemShowMdnDocs.hidden = !this.doc.popupNode.parentNode.classList.contains(PROPERTY_NAME_CLASS);

We try to keep line length to under 80 chars. So the idea is to try and cut long lines are places that make sense, so that it's easy to read. Of course if something is like 90 chars and would look awful if you cut it, then it's fine.

In this case, maybe something like this would work:

this.menuitemShowMdnDocs.hidden = !this.doc.popupNode.parentNode
                                  .classList.contains(PROPERTY_NAME_CLASS);
(not sure if bugzilla will keep the pre-formatting, but .classList is supposed to be aligned below !this.doc.)

@@ +1419,5 @@
> +  _onShowMdnDocs: function() {
> +    let cssPropertyName = this.doc.popupNode.textContent;
> +    let anchor = this.doc.popupNode.parentNode;
> +    let cssDocsTooltip = this.tooltips.cssDocs;
> +    mdnDocs.getCssDocs(cssPropertyName).then(

I don't think I'm a fan of doing this part here.
Why not hiding the details of retrieving the docs into the mdn docs widget?
Here you'd just pass the property name, therefore reducing the amount of code here and removing the need to import mdn-docs here.

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +307,5 @@
>  
> +    if (this.cssDocs) {
> +      this.cssDocs.destroy();
> +    }
> +

I think you also need to hide this new tooltip in _onPreviewTooltipTargetHover and _onNewSelection, to make sure it doesn't get in the way in these edge cases.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +55,5 @@
>  #LOCALIZATION NOTE: Used in the image preview tooltip when the image could not be loaded
>  eventsTooltip.openInDebugger=Open in Debugger
> +
> +# LOCALIZATION NOTE (docsTooltip.visitMDN): Link to visit the MDN documentation
> +# page for a particular CSS property.

The link label doesn't say that this is necessarily about a CSS property, so perhaps the comment should say "Link to visite a MDN documentation page"

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +100,5 @@
>  # the rule view context menu "Show original sources" entry.
>  ruleView.contextmenu.showOrigSources.accessKey=O
>  
> +# LOCALIZATION NOTE (ruleView.contextmenu.showMdnDocs): Text displayed in the rule view
> +# context menu.

It helps l10n to be a bit more precise in these comments, like: "Text displayed in the rule view context menu to enable MDN docs tooltips".

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #12)
> I think a context menu item is good enough. It's not annoying and doesn't
> need a help mode.
I meant: the context menu *is* the help mode. Scenario:
- right-click anywhere in the rule-view
- select "show MDN docs"
-> help icons appear next to property names
- click on an icon
-> the MDN tooltip appears on the icon, containing docs about the property name
- click outside the tooltip
-> the MDN tooltip disappears
- right click anywhere in the rule-view
- select "hide MDN docs"
-> help icons disappear

Comment 15

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> (In reply to Tim Nguyen [:ntim] from comment #12)
> > I think a context menu item is good enough. It's not annoying and doesn't
> > need a help mode.
> I meant: the context menu *is* the help mode. Scenario:
> - right-click anywhere in the rule-view
> - select "show MDN docs"
> -> help icons appear next to property names
> - click on an icon
> -> the MDN tooltip appears on the icon, containing docs about the property
> name
> - click outside the tooltip
> -> the MDN tooltip disappears
> - right click anywhere in the rule-view
> - select "hide MDN docs"
> -> help icons disappear

I didn't think of the context menu that way. 
I was thinking we could show a "Show MDN docs" context menu item when the users right-clicks a CSS property, and when the user clicks that context menu item, it directly shows a MDN tooltip.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #15)
> I didn't think of the context menu that way. 
> I was thinking we could show a "Show MDN docs" context menu item when the
> users right-clicks a CSS property, and when the user clicks that context
> menu item, it directly shows a MDN tooltip.
Yeah that works too, but requires 2 clicks instead of 1 (once the help icons are displayed). The advantage is that it doesn't clutter the rule-view UI at all.

Comment 17

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> (In reply to Tim Nguyen [:ntim] from comment #15)
> > I didn't think of the context menu that way. 
> > I was thinking we could show a "Show MDN docs" context menu item when the
> > users right-clicks a CSS property, and when the user clicks that context
> > menu item, it directly shows a MDN tooltip.
> Yeah that works too, but requires 2 clicks instead of 1 (once the help icons
> are displayed). The advantage is that it doesn't clutter the rule-view UI at
> all.

Yeah, but usually, most users just want to check 1 property quickly. I doubt they need to check multiple properties at once.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8565083 [details] [diff] [review]
0001-Add-MDN-tooltips-for-CSS-properties.git.patch

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

I agree display on hover doesn't really make sense for this feature, both because of the load delay and because there's a 'read more' link that would be impossible to click if the tooltip disappeared when you hovered away.

My first thought was some sort of modifier when clicking the property name (like alt+click) would open up the tooltip, but that isn't discoverable at all, so I don't think it's good.

The right-click-to-show-a-tooltip thing as it is now is sort of a weird interaction, especially if the tooltip is slow to load.  But it works and stays out of the way.  I think this could be improved if the tooltip opened immediately with some text like 'Loading documentation' that got swapped out once the request finished.  This is my vote for a v1 of this feature.

Please, let's not add a toggleable preference for this.  Asking someone to flip some advanced preference in order to get documentation about CSS properties seems cruel and would limit the usefulness of this.

Comment 19

[Codacoder]

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Please, let's not add a toggleable preference for this.  Asking someone to
> flip some advanced preference in order to get documentation about CSS
> properties seems cruel and would limit the usefulness of this.

Please DO make it a preference - anything that pups up and distracts my attention from what I'm actually trying to do (something the tool is incapable of knowing in advance) is noise.  I'm happy for the pref to be opt-out (default on) but please PLEASE make it so. 

rant
The debugger source pane is the biggest culprit in this regard - I'm looking at code, thinking about logic and needing to take special care of where I rest my mouse to avoid the tool dev throwing up a dump of whatever is under my mouse.  Yet, in the watch pane, where I might be actually interested in thinking about the content of memory, nothing like this happens. Inappropriate, misplaced and mistimed functionality is noise, however well-intentioned it may have been.
/rant

Comment 20

[Codacoder]

Trying to be a little more constructive...

How about a one-line slide-up toaster bar (bottom of window) with a clickable [more...] link (which might actually launch MDN itself) ?

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Russ from comment #19)
> Please DO make it a preference - anything that pups up and distracts my
> attention from what I'm actually trying to do (something the tool is
> incapable of knowing in advance) is noise.  I'm happy for the pref to be
> opt-out (default on) but please PLEASE make it so. 

I should clarify - I don't necessarily mind it being preffable as long as it's on by default.  Given the current UI (a context menu item when right clicking on the property name) I don't see a reason why it would annoy someone enough to want it off, but it could be behind a pref.  I wouldn't want to add that to our (already crowded) options panel though.

Comment 22

[Codacoder]

Sorry, Brian - I seem to have conflated your message with the popup-on-hover solution (hence the rant).  My bad.

Yes, context-menu is good.  Least it shows willful intent (unlike my rant target!)

Comment 23

[Will Bamberg [:wbamberg]]

Thanks for the feedback, Brian. I agree with this.

> I think this could be improved if the tooltip opened
> immediately with some text like 'Loading documentation' that got swapped out
> once the request finished.  This is my vote for a v1 of this feature.

My worry here is with timing. I get the tooltip visible in about half a second, to a second at the outside. If I put up "loading documentation", then usually I won't have time to digest it before it's replaced, and there will just be a weird flicker effect. However, I can see that this feature would be frustrating on a much slower network.

(related: at the moment I display nothing if I could not load the docs. Maybe I should put up the tooltip with an apologetic message, in this case?)

Comment 24

[Codacoder]

(In reply to Will Bamberg [:wbamberg] from comment #23)
> Thanks for the feedback, Brian. I agree with this.
> 
> > I think this could be improved if the tooltip opened
> > immediately with some text like 'Loading documentation' that got swapped out
> > once the request finished.  This is my vote for a v1 of this feature.
> 
> My worry here is with timing. I get the tooltip visible in about half a
> second, to a second at the outside. If I put up "loading documentation",
> then usually I won't have time to digest it before it's replaced, and there
> will just be a weird flicker effect. However, I can see that this feature
> would be frustrating on a much slower network.

1 - display "Searching..."
2 - wait for search result to be returned (async)
3 - when result ready, apply 600ms fade to 1.
4 - fade in result

If result takes, I dunno, longer than 5 secs? display "Oh noes! teh internetz is slow!"

Comment 25

[Brian Grinstead [:bgrins]]

Something like Comment 24 seems reasonable.  Or just open up a popup with a spinner gif so that there isn't much to process.

Having to do a little bit of trickery with a super quick network is better than the popup not showing up immediately on right click.  If it sometimes doesn't show up right away (or at all) it feels broken / flaky.  Of course if you have the results in a local cache you could skip the loading splash screen altogether.

Comment 26

[Will Bamberg [:wbamberg]]

Attachment: mdn-tooltips.patch

Thanks for the review Patrick!

I think I've made almost all the changes you suggested.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13)
> Comment on attachment 8565083 [details] [diff] [review]
> 0001-Add-MDN-tooltips-for-CSS-properties.git.patch
> 
> Review of attachment 8565083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +1537,4 @@
> >  });
> >  
> >  /**
> > + * Tooltip for displaying docs for CSS properties from MDN.
> 
> Not sure this extra class is needed. We have similar classes for the color
> picker or the animation timing functions for example, but that's because
> they are sub classes of SwatchBasedEditorTooltip.
> In fact, thinking about it, if we decide to have a help icon that you need
> to click on to display the tooltip, we could keep this class and make it
> inherit from SwatchBasedEditorTooltip too.
> But with the current behavior, I don't think it's needed because it only
> wraps this:
> 
> let mdnTooltip = new Tooltip(doc, {
>   consumeOutsideClick: true,
>   closeOnKeys: [ESCAPE_KEYCODE, RETURN_KEYCODE],
>   noAutoFocus: false 
> });
> this.widget = mdnTooltip.setCssDocsContent();
> 
> Which the consumer could call instead.

I didn't do this: I've kept the extra class in Tooltip.js. The reason was that "the consumer" in this case is the menu item in rule-view.js, and it didn't seem right for that to be involved with the widget object. As it is, the menu item just has to call "show" and the tooltip takes care of everything. In fact later on you said this:

> @@ +1419,5 @@
> > +  _onShowMdnDocs: function() {
> > +    let cssPropertyName = this.doc.popupNode.textContent;
> > +    let anchor = this.doc.popupNode.parentNode;
> > +    let cssDocsTooltip = this.tooltips.cssDocs;
> > +    mdnDocs.getCssDocs(cssPropertyName).then(
> 
> I don't think I'm a fan of doing this part here.
> Why not hiding the details of retrieving the docs into the mdn docs widget?
> Here you'd just pass the property name, therefore reducing the amount of
> code here and removing the need to import mdn-docs here.

... so in the spirit of that comment (which makes a lot of sense to me) it doesn't seem right to have _onShowMdnDocs having to interact with the widget.

Some other things:

* I haven't used CodeMirror to do syntax highlighting, for a few reasons:
- I was worried that it seemed too heavyweight just to do syntax highlighting. Do we have any idea of the cost of loading the editor?
- the code samples aren't actually valid CSS, since they don't include a selector. So I don't think we would get proper syntax highlighting anyway, if we just throw them as-is at CodeMirror. I'm not sure what we could do about this (change the content of the samples? somehow fake the content? tell CodeMirror not to be so clever?).
- I could not get the editor to work :(. I could initialize it and call setText, but could not see anything. I might need some help with that, if we decide to do it after all.

* I haven't done anything about persisting the doc fragments. I could have a go at that, but this patch is already quite big, and I think I agree with you that it should be a V2 thing.

* in terms of combining things and generalizing them (i.e. using "MDN-docs" rather than "CSS-docs", to leave room for JS etc): I think it's hard to be sure what will change and what will not, if/when we add support for more things. The general approach I've taken is to give files and (most) classes the generalized name, so MdnDocsWidget and so on, but to have CSS-specific functions, as they are clearly doing CSS-specific things.

* MdnDocsWidget.js has now duplicated the "L10N utility class" from Tooltip.js. I wasn't sure where would be a good shared place for it.

* I'm now displaying the tooltip immediately and using the devtools throbber while the content loads.

* you mentioned how the MDN URL needs to be configurable for tests. I've done this by adding a new export in MdnDocsWidget.js called "setBaseUrl". I'm not sure if this is the best approach, but I didn't like the idea of passing extra arguments. Would that be better though? Or would a pref be better?

* I think perhaps we should have a security review for this, since I'm getting content from an untrusted page and inserting it into a chrome context.

Comment 27

[Patrick Brosset <:pbro>]

Comment on attachment 8572904 [details] [diff] [review]
mdn-tooltips.patch

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

Hi Will, thanks for this patch, awesome work. I like the feature a lot actually. We can iterate on the UX later if we want, but right now, this is a pretty effective way of getting access to docs without being in the way of using the rule-view. Displaying the tooltip immediately sure helped a lot.

Playing with the feature, I found the following minor issues:
- there's no doc nor error message for some properties, try -moz-border-left-colors for instance.
- the throbber sometimes disappear long before the doc actually appears, like 1 second before. Not a big deal though.
- I see some weird flickering right when the doc appears, but it's random (in shape and occurrence) and I know we've had similar weirdness with xul panels before, so I'm going to blame this on panels.

About using CodeMirror, no problem, let's start with what you have. I don't know either what's the perf cost of loading CM, and you're right, because the samples aren't valid CSS, we wouldn't get proper highlighting anyway.
Do the code samples have a consistent syntax though? If they are always a string containing lines with 'name:value /* comment */', then it'd be easy to split that and syntax highlight manually. But I'm guessing the code samples can come in various forms.

Agreed to move doc persistence to v2. That means we need to handle offline gracefully in v1, with an error message. You've done this, so perfect.

About the security review, I agree that one should be done. I have never had to do this in the past, so I don't know how to do this and who to ping. Perhaps Mark Goodwin?

I've made some comments below, nothing major, but I'd like to take a final look once addressed though.
Only thing missing is new tests. These can be done in a separate patch if you'd prefer keeping this one small.
Let me know. I'll cancel the review for now, but if you want to work on a new patch for the tests, I'll R+ this one.

::: browser/devtools/shared/widgets/MdnDocsWidget.js
@@ +14,5 @@
> + * It's split into two parts:
> + *
> + * - functions like getCssDocs that just fetch content from MDN,
> + * without any constraints on what to do with the content. If you
> + * don't want to embed the content in some custom way, use this.

s/don't want/want

@@ +19,5 @@
> + *
> + * - the MdnDocsWidget class, that manages and updates a tooltip
> + * document whose content is taken from MDN. If you want to embed
> + * the content in a tooltip, use this in conjunction with Tooltip.js.
> + */

Thanks for adding this comment, this helps a lot.

Some remarks concerning the organization of this file:
- We usually put helper functions at the bottom, so that someone opening the file would first see the interesting functions. So please move isAllWhiteSpace, isIgnorable, ..., up to getSyntax (maybe others?) to the bottom of the file.
- We usually put exports.thing right after thing, so please move the exports definitions right after the corresponding symbols.

@@ +29,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +
> +// see https://developer.mozilla.org/en-US/docs/MDN/Kuma/API#Document_parameters
> +const URL_POSTFIX = "?raw&macros";
> +var BASE_MDN_PAGE = "https://developer.mozilla.org/docs/Web/CSS/";

This variable's name should say CSS somewhere. Thanks for generalizing the other things I mentioned, but this one is specifically for CSS docs and is only used from the css doc fetching function.

@@ +197,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function makeDocsPageUrl(propertyName) {

Rename to something like makeCssDocsPageUrl

@@ +202,5 @@
> +  return BASE_MDN_PAGE + propertyName;
> +}
> +
> +/**
> + * Use a different URL for MDN pages. Used only for testing.

This should work fine for testing, thanks.

@@ +207,5 @@
> + *
> + * @param {string} baseUrl
> + * The baseURL to use.
> + */
> +function setBaseUrl(baseUrl) {

And therefore, something like setBaseCssUrl here.

@@ +228,5 @@
> + * The promise is rejected with an error message if
> + * we could not load the page.
> + */
> +function getCssDocs(cssProperty) {
> +  let deferred = Promise.defer();

You don't need to create an extra promise here. You could rewrite the function like so:

function getCssDocs(cssProperty) {
  let pageUrll = makeDocsPageUrl(cssProperty) + URL_POSTFIX;
  return getMdnPage(pageUrl).then(responseDocument => {
    return {
      summary: getSummary(responseDocument),
      syntax: getSyntax(responseDocument)
    };
  }, e => promise.reject(e.status));
}

@@ +267,5 @@
> +function MdnDocsWidget(tooltipDocument) {
> +  this.tooltipDocument = tooltipDocument;
> +
> +  // get the localized string for the link text
> +  let linkToMdn = tooltipDocument.getElementById("visit-mdn-page");

You could store linkToMdn on the instance (this.linkToMdn = ...) so that you can reuse it later in initializeDocument instead of re-querying it from the DOM.

@@ +301,5 @@
> +   *
> +   * @param {string} propertyName
> +   * The name of the CSS property for which we need to display help.
> +   */
> +  loadCssDocs: function(propertyName) {

Renaming the widget to MdnDocsWidget and having this css-specialized function makes sense. The widget is generic, and may have 1 or more specialized functions to load MDN content in its parent document. I like that.

@@ +310,5 @@
> +     * MDN docs content to be retrieved.
> +     */
> +    function initializeDocument(propertyName) {
> +      // set property name heading
> +      let propertyNameHeading = tooltipDocument.getElementById("property-name");

This DOM element could also be retrieved once only in the widget constructor and then just used here.
Same remark for the mdn link, summary, syntax and property-info.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +756,2 @@
>     */
> +  setIFrameContent: function({width, height}, url) {

Thanks for refactoring this part. That helps simplifying the file.

::: browser/devtools/styleinspector/rule-view.js
@@ +25,4 @@
>  const PREF_UA_STYLES = "devtools.inspector.showUserAgentStyles";
>  const PREF_DEFAULT_COLOR_UNIT = "devtools.defaultColorUnit";
>  
> +const PROPERTY_NAME_CLASS = "ruleview-propertyname";

nit: no need for an empty line before this const.

@@ +1249,5 @@
>      var showOrig = Services.prefs.getBoolPref(PREF_ORIG_SOURCES);
>      this.menuitemSources.setAttribute("checked", showOrig);
>  
> +    this.menuitemShowMdnDocs.hidden = !this.doc.popupNode.parentNode
> +                                      .classList.contains(PROPERTY_NAME_CLASS);

Another way, although this is fine, is this:

this.ctxMenuNodeInfo = this.getNodeInfo(this.doc.popupNode.parentNode);
this.menuitemShowMdnDocs.hidden = !this.ctxMenuNodeInfo ||
                                  this.ctxMenuNodeInfo.type !== overlays.VIEW_NODE_PROPERTY_TYPE;

Which then makes it possible to reuse this.ctxMenuNodeInfo in _onShowMdnDocs:

_onShowMdnDocs: function() {
  if (!this.ctxMenuNodeInfo) {
    return;
  }

  let cssPropertyName = this.ctxMenuNodeInfo.value.property;
  ...

Comment 28

[Luke Crouch [:groovecoder]]

Re: caching, are these requests made with regular HTTP cache negotiation headers so we at least have that?

Also, can we add this to the MDN link target href?

?utm_source=mozilla&utm_medium=firefox-inspector&utm_campaign=default

That will let us segment the traffic that comes from this feature to better understand their unique behavior on MDN.

Comment 29

[Will Bamberg [:wbamberg]]

Thanks again Patrick!

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #27)
> Comment on attachment 8572904 [details] [diff] [review]
> mdn-tooltips.patch
> 
> Review of attachment 8572904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Will, thanks for this patch, awesome work. I like the feature a lot
> actually. We can iterate on the UX later if we want, but right now, this is
> a pretty effective way of getting access to docs without being in the way of
> using the rule-view. Displaying the tooltip immediately sure helped a lot.
> 
> Playing with the feature, I found the following minor issues:
> - there's no doc nor error message for some properties, try
> -moz-border-left-colors for instance.
> - the throbber sometimes disappear long before the doc actually appears,
> like 1 second before. Not a big deal though.
> - I see some weird flickering right when the doc appears, but it's random
> (in shape and occurrence) and I know we've had similar weirdness with xul
> panels before, so I'm going to blame this on panels.

Yes, I see this, too. I don't understand why the fade-in is not consistent. It seems to depend on the property we are looking at. But I don't know what I can do about it, since it doesn't seem to be anything in this code.

> 
> About using CodeMirror, no problem, let's start with what you have. I don't
> know either what's the perf cost of loading CM, and you're right, because
> the samples aren't valid CSS, we wouldn't get proper highlighting anyway.
> Do the code samples have a consistent syntax though? If they are always a
> string containing lines with 'name:value /* comment */', then it'd be easy
> to split that and syntax highlight manually. But I'm guessing the code
> samples can come in various forms.
> 

We can (and should I think) define the syntax quite precisely, but we will need to fail gracefully in cases where the docs don't follow it. (I'm going to work on making the content more consistent in parallel with adding this feature in the devtools, but this will be an ongoing thing.)

I was tempted to try to implement syntax highlighting myself, but it feels like the sort of thing that seems easier than it is. If I have time I will give it a try. I think it would be very nice to have.

> Agreed to move doc persistence to v2. That means we need to handle offline
> gracefully in v1, with an error message. You've done this, so perfect.
> 
> About the security review, I agree that one should be done. I have never had
> to do this in the past, so I don't know how to do this and who to ping.
> Perhaps Mark Goodwin?

I've asked Mark, and he's going to take a look.

> I've made some comments below, nothing major, but I'd like to take a final
> look once addressed though.
> Only thing missing is new tests. These can be done in a separate patch if
> you'd prefer keeping this one small.
> Let me know. I'll cancel the review for now, but if you want to work on a
> new patch for the tests, I'll R+ this one.
> 

I think I'll make a single patch for the whole thing, that includes tests and the changes you've listed.

Comment 30

[Patrick Brosset <:pbro>]

Currently running a try build so people can already try this out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=317f3210d217
Jeff (who is cc'd) wanted to.

Comment 31

[Will Bamberg [:wbamberg]]

Removing the dependency on bug 1142146. Luke, just let me know if you decide it should block.

Comment 32

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Currently running a try build so people can already try this out:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=317f3210d217
> Jeff (who is cc'd) wanted to.
Hey, turns out that try build shows devtools tests are still passing! So at least, your patch doesn't break any existing feature :)
Plus, we now have firefox builds for all platforms! Here's the build dir for macos 10.8 http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-317f3210d217/try-macosx64/

Comment 33

[Will Bamberg [:wbamberg]]

Attachment: mdn-tooltips-3.patch

Some things:

1) I think I've done everything you mentioned in comment 27, except this:

>> + * The promise is rejected with an error message if
>> + * we could not load the page.
>> + */
>> +function getCssDocs(cssProperty) {
>> +  let deferred = Promise.defer();
>
> You don't need to create an extra promise here. You could rewrite the function like so:
>
> function getCssDocs(cssProperty) {
>   let pageUrll = makeDocsPageUrl(cssProperty) + URL_POSTFIX;
>   return getMdnPage(pageUrl).then(responseDocument => {
>     return {
>       summary: getSummary(responseDocument),
>       syntax: getSyntax(responseDocument)
>     };
>   }, e => promise.reject(e.status));
> }

With that code, I get an error:

> ReferenceError: promise is not defined

Sorry if I just being slow-witted, but I wasn't sure what `promise` was supposed to be here. IMHO this version is harder to read, but this might just be me. I'll happily change it if you can tell me what it ought to be.

I've also added the extra parameters Luke asked for in comment 28.

2) There are 2 test files:
/browser/devtools/shared/test/browser_mdn-docs.js - tests the widget
/browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs.js - tests the context menu item and basic tooltip show/hide

The second of these is largely stolen from browser_styleinspector_context-menu-copy-color_01.js. That file has this at the start:

> // Test is slow on Linux EC2 instances - Bug 1137765
> requestLongerTimeout(2);

I'm not sure if I should be doing this or not.

Actually, both these tests initialize the rule view, then iterate through every node checking whether the context menu item is visible when that node isPopupNode. So maybe it would be more efficient to have a single test that checks all the context menu items? I'm not sure if this is worth doing in this patch, I think it might get a bit involved.

I didn't use separate unit tests for the getCssDocs XHR stuff, because that code is covered by the widget tests. But let me know if I should.

There are a few things I know I haven't covered:
* panel being hidden when you click outside the panel - I experimented a bit with EventUtils.synthesizeMouse but didn't understand how to generate a mouse click *outside* an element.
* panel not being hidden when you click inside the panel - I'm not sure how you can test a negative here

I'm hoping that this stuff is covered by generic panel tests.

I'm probably missing more things though.

3) Memory leak testing: I haven't done anything specific for this, do I need to?

4) There is a bug: if you show the tooltip, then switch to a different tab using the keyboard (Ctrl+Tab), then the panel is not dismissed. You seem to be able to fix the problem with code like this, in CssDocsTooltip.show():

***
// hide the tooltip if the user switches tabs while it's open
let browserWindow = Services.wm.getMostRecentWindow(BROWSER_WINDOW);
let tooltip = this;

function hideOnTabSwitch() {
  browserWindow.removeEventListener("TabSelect", hideOnTabSwitch, false);
  tooltip.hide();
}

browserWindow.addEventListener("TabSelect", hideOnTabSwitch, false);
***

However, this seems to be a general problem for all tooltips (e.g. try hovering over a background image URL, then Ctrl-Tab to a different tab), so I wonder if we should fix it somewhere else, maybe in style-inspector-overlays.js? It's much more apparent for this tooltip because there's a common use case where you show the tooltip, click the "Visit MDN" link, that opens the tab in the background, then Ctrl-Tab to the MDN page.

That's all I can think of for now.

Comment 34

[Patrick Brosset <:pbro>]

(In reply to Will Bamberg [:wbamberg] from comment #33)
> With that code, I get an error:
> 
> > ReferenceError: promise is not defined
Sorry, my bad, it's 'Promise' with a capital P.
> Sorry if I just being slow-witted, but I wasn't sure what `promise` was
> supposed to be here. IMHO this version is harder to read, but this might
> just be me. I'll happily change it if you can tell me what it ought to be.
I don't personally find it harder to read, but I'm ok to stick with your version.

> The second of these is largely stolen from
> browser_styleinspector_context-menu-copy-color_01.js. That file has this at
> the start:
> 
> > // Test is slow on Linux EC2 instances - Bug 1137765
> > requestLongerTimeout(2);
> 
> I'm not sure if I should be doing this or not.
No you should not. This should only really be added in few cases, when we have no other choice. Let's get rid of it and see if it works fine on TRY.

> * panel being hidden when you click outside the panel - I experimented a bit
> with EventUtils.synthesizeMouse but didn't understand how to generate a
> mouse click *outside* an element.
You should instead just simulate a click on an element of the rule-view that's outside of the panel. Just choose any element really. You could click on a property value for example.

> * panel not being hidden when you click inside the panel - I'm not sure how
> you can test a negative here
This is a rather generic tooltip test that I don't think you should worry about. Your feature relies on a shared Tooltip widget and you should assume that its basic features are tested (if they're not, then that belongs to another bug anyway).

> 3) Memory leak testing: I haven't done anything specific for this, do I need
> to?
I don't know :) I haven't done this myself. You should ask on IRC#devtools.

> 4) There is a bug: if you show the tooltip, then switch to a different tab
> using the keyboard (Ctrl+Tab), then the panel is not dismissed. You seem to
> be able to fix the problem with code like this, in CssDocsTooltip.show():
> 
> ***
> // hide the tooltip if the user switches tabs while it's open
> let browserWindow = Services.wm.getMostRecentWindow(BROWSER_WINDOW);
> let tooltip = this;
> 
> function hideOnTabSwitch() {
>   browserWindow.removeEventListener("TabSelect", hideOnTabSwitch, false);
>   tooltip.hide();
> }
> 
> browserWindow.addEventListener("TabSelect", hideOnTabSwitch, false);
> ***
> 
> However, this seems to be a general problem for all tooltips (e.g. try
> hovering over a background image URL, then Ctrl-Tab to a different tab), so
> I wonder if we should fix it somewhere else, maybe in
> style-inspector-overlays.js? It's much more apparent for this tooltip
> because there's a common use case where you show the tooltip, click the
> "Visit MDN" link, that opens the tab in the background, then Ctrl-Tab to the
> MDN page.
It is indeed a general problem for all tooltips, and I don't think we have that bug filed, so could you please file it, and put there the TabSelect listener code? I don't think your patch needs to be blocked on this however.

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 8581041 [details] [diff] [review]
mdn-tooltips-3.patch

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

Nice. I think the main tests are in place. I only have a few comments regarding refactoring and splitting tests. Other than this, I'm happy with the patch.
I'll take just one last look when the comments are addressed, and we should be good to go then.

::: browser/devtools/shared/test/browser_mdn-docs.js
@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Very nice test, I like the comments and the code is easy to read.
One general comment though, it helps maintainability if tests are small and test one thing only. They're even easier to read and debug and they run faster (which helps a lot when running on our test infra).
So I'd like you to split this in several test files:
- one that tests the basics,
- one that tests the missing page case,
- one that tests the various missing elements cases

@@ +59,5 @@
> +  yield promiseTab("about:blank");
> +  let [host, win, doc] = yield createHost("bottom", MDN_DOCS_TOOLTIP_FRAME);
> +  let widget = new MdnDocsWidget(win.document);
> +
> +  // test all the basics

I think a good rule of thumb in tests is: whenever you feel like adding a line comment like this one (and others below), add an 'info("...")' instead, with the same comment.
It's important because when tests start to fail intermittently on remote servers, all we have at our disposal to debug them are logs, and the first step is figuring out which line the test fails exactly. So commenting each important step with info() statement is good.
So just wrap these comments in info()

@@ +79,5 @@
> +  yield testNoSummaryOrSyntax(widget);
> +
> +  host.destroy();
> +  gBrowser.removeCurrentTab();
> +

nit: empty line.

@@ +92,5 @@
> + * - mdn link text is correct and onclick behavior is correct
> + */
> +function* testTheBasics(widget) {
> +  info("Test all the basic functionality in the widget");
> +  let doc = widget.docElements;

doc isn't the best name here as this variable name is usually used for document. Maybe change it to let widgetElements, or just don't define it and always use widget.docElements.
In fact, you could perhaps rename docElements to elements in MdnDocsWidget.js

@@ +123,5 @@
> +
> +  info("Check that MDN link text is correct and onclick behavior is correct");
> +  yield checkMdnLink(BASIC_TESTING_PROPERTY, widget);
> +
> +  function* checkMdnLink(testProperty, widget) {

This function isn't used anywhere else, maybe we don't need that function at all and just keep its code.
If the goal is to separate it out from the testTheBasics test step function, then fine, but maybe put it outside of the function, at the bottom of this file. Same for checkLinkClick.
This would help people adding new test stop functions to this file later and reuse these if necessary.

@@ +180,5 @@
> +  }
> +
> +}
> +
> +function* testNonExistentPage(widget) {

This function and the next 4 functions look very similar. They look like excellent candidates for a little bit of refactoring.
We often use this approach in devtools tests:

const TEST_DATA = [{
  desc: "Test a property for which we don't have a page",
  docsPageUrl: "i-dont-exist.html",
  expectedContents: {
    propertyName: "i-dont-exist.html",
    summary: ERROR_MESSAGE,
    syntax: ""
  }
}, {
  desc: "Test a property whose syntax section is specified using an ID",
  docsPageUrl: SYNTAX_BY_ID,
  expectedContents: {
    propertyName: SYNTAX_BY_ID,
    summary: BASIC_EXPECTED_SUMMARY,
    syntax: BASIC_EXPECTED_SYNTAX
  }
}, {
  ...
}];

And then:

for (let {desc, docsPageUrl, expectedContents} of TEST_DATA) {
  info(desc);
  yield widget.loadCssDocs(docsPageUrl);
  checkTooltipContents(widget.docsElements, expectedContents);
}

This makes it super easy to add new test cases later, and makes it easier to read the code too.
So maybe you could just split this file in 2: one for the basics of loading and interacting with the tooltip, and another test with these cases.

::: browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs.js
@@ +26,5 @@
> +const PROPERTYNAME = "color";
> +
> +add_task(function* () {
> +
> +  const TEST_DOC = '<html>                                              \

template strings with `` are multiline and don't need \ at the end of each line, so maybe use that instead.

@@ +41,5 @@
> +  yield selectNode("div", inspector);
> +
> +  yield testMdnContextMenuItemVisibility(view);
> +
> +  yield testShowAndHideMdnTooltip(view);

This should be separated out in a second test. To keep this one small and easy to maintain, fast to run.

@@ +43,5 @@
> +  yield testMdnContextMenuItemVisibility(view);
> +
> +  yield testShowAndHideMdnTooltip(view);
> +
> +  yield clearCurrentNodeSelection(inspector);

This looks like this was stolen from browser_styleinspector_context-menu-copy-color_01.js, which is fine, but since I don't remember why this is there, I'd like if this was removed, and we test without it. If it really needs to be here, we should investigate why and either fix the cause or add a comment here saying why it's needed.

@@ +63,5 @@
> +
> +  let root = rootElement(view);
> +  for (let node of iterateNodes(root)) {
> +    info("Setting " + node + " as popupNode");
> +    if (view.doc) {

What is this if/else about? Isn't the rule-view document always available at view.doc?

@@ +73,5 @@
> +
> +    info("Updating context menu state");
> +    view._contextMenuUpdate();
> +    let isVisible = !view.menuitemShowMdnDocs.hidden;
> +    let shouldBeVisible = isPropertyNameNode(node);

You should also test that right-clicking on a selector or a value that look like a property name doesn't show the menu item:

padding {
  color: red;
}

or 

selector {
  content: "margin";
}

@@ +93,5 @@
> +
> +  info("Setting the popupNode for the MDN docs tooltip");
> +  let root = rootElement(view);
> +  let propertyNameSpan = root.querySelector(PROPERTYNAME_SELECTOR);
> +  if (view.doc) {

When can view.doc be falsy?
I see now that this came from browser_styleinspector_context-menu-copy-color_01.js, but it looks odd, and so again, we should either explain in the code why it's here or get rid of it.

@@ +109,5 @@
> +
> +  info("Quick check that the tooltip contents are set");
> +  let tooltipDocument = cssDocs.tooltip.content.contentDocument;
> +  let h1 = tooltipDocument.getElementById("property-name");
> +  is(PROPERTYNAME, h1.textContent, "The MDN docs tooltip h1 is correct");

The correct parameter order is: is(actual, expected, info), so is(h1.textContent, PROPERTYNAME, "The MDN docs tooltip h1 is correct");
Not a big deal, but it helps reading logs for failed tests.

@@ +138,5 @@
> +  }
> +}
> +
> +/**
> + * Returns the root element for the given view, rule or computed.

Why computed? The tooltip only works in the rule-view so far, right? And this test only opens the rule-view anyway.

Comment 36

[Will Bamberg [:wbamberg]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> (In reply to Will Bamberg [:wbamberg] from comment #33)
> 
> > 4) There is a bug: if you show the tooltip, then switch to a different tab
> > using the keyboard (Ctrl+Tab), then the panel is not dismissed. You seem to
> > be able to fix the problem with code like this, in CssDocsTooltip.show():
> > 
> > ***
> > // hide the tooltip if the user switches tabs while it's open
> > let browserWindow = Services.wm.getMostRecentWindow(BROWSER_WINDOW);
> > let tooltip = this;
> > 
> > function hideOnTabSwitch() {
> >   browserWindow.removeEventListener("TabSelect", hideOnTabSwitch, false);
> >   tooltip.hide();
> > }
> > 
> > browserWindow.addEventListener("TabSelect", hideOnTabSwitch, false);
> > ***
> > 
> > However, this seems to be a general problem for all tooltips (e.g. try
> > hovering over a background image URL, then Ctrl-Tab to a different tab), so
> > I wonder if we should fix it somewhere else, maybe in
> > style-inspector-overlays.js? It's much more apparent for this tooltip
> > because there's a common use case where you show the tooltip, click the
> > "Visit MDN" link, that opens the tab in the background, then Ctrl-Tab to the
> > MDN page.
> It is indeed a general problem for all tooltips, and I don't think we have
> that bug filed, so could you please file it, and put there the TabSelect
> listener code? I don't think your patch needs to be blocked on this however.

-> filed bug 1146490. I'll get to the other things as soon as I can.

Comment 38

[Patrick Brosset <:pbro>]

Flagging this as a "medium" difficulty feature for devedition 40 primarily because a lot (most) of the work has been done already, otherwise I'd have flagged this as hard probably.
Will, do you think you'll have some time to work on this in the coming weeks?

Comment 39

[Luke Crouch [:groovecoder]]

Do we measure the number of Style Editor uses-per-hour so we can prepare the MDN back-end for the proper scale of additional requests?

Comment 40

[Brian Grinstead [:bgrins]]

(In reply to Luke Crouch [:groovecoder] from comment #39)
> Do we measure the number of Style Editor uses-per-hour so we can prepare the
> MDN back-end for the proper scale of additional requests?

Jeff, to be clear the tooltips are actually in the Inspector panel (in the rule view)

Comment 41

[Will Bamberg [:wbamberg]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #38)
> Flagging this as a "medium" difficulty feature for devedition 40 primarily
> because a lot (most) of the work has been done already, otherwise I'd have
> flagged this as hard probably.
> Will, do you think you'll have some time to work on this in the coming weeks?

Yes. I will have a new patch in the next couple of days.

Comment 42

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Brian Grinstead [:bgrins] from comment #40)
> (In reply to Luke Crouch [:groovecoder] from comment #39)
> > Do we measure the number of Style Editor uses-per-hour so we can prepare the
> > MDN back-end for the proper scale of additional requests?

The measure we have is whether the toolbox has been opened on a given date, this measure is 'DEVTOOLS_INSPECTOR_OPENED_BOOLEAN', see http://telemetry.mozilla.org/#filter=aurora%2F38%2FDEVTOOLS_INSPECTOR_OPENED_BOOLEAN%2Fsaved_session%2FFirefox%2FWINNT&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

That graph isn't too useful, but I have a graph that does daily counts and we peak on Wednesdays at ~ 90,000 times per day.

> Jeff, to be clear the tooltips are actually in the Inspector panel (in the
> rule view)iterm

Yeah - to be clear this feature is exposed in the inspector's ruleview, which gets much more use from developers than the style inspector. 

I don't think you can realistically expect 7500 hits an hour, every hour, but I do think when we launch the feature on June 2 there will be a pretty big spike.

Are you considering some sort of caching mechanism for this data? Ideally we'd be able to just fetch a json blob occasionally and not have to worry about your server load.

Comment 43

[Luke Crouch [:groovecoder]]

We filed https://bugzilla.mozilla.org/show_bug.cgi?id=1142146 to chase down why the regular HTTP 304 caching isn't happening. We should probably make that bug block this one? And I can put it into our Inbox for next week.

Comment 44

[Will Bamberg [:wbamberg]]

Attachment: new patch

Thanks! I've done everything you suggested, I think, except:

>> * panel being hidden when you click outside the panel - I experimented a bit
>> with EventUtils.synthesizeMouse but didn't understand how to generate a
>> mouse click *outside* an element.
>
> You should instead just simulate a click on an element of the rule-view that's outside of the panel. Just choose > any element really. You could click on a property value for example.

I tried this, with code like:

function* testShowAndHideWithMouseClick(view) {
  yield testShowMdnTooltip(view);

  info("Simulate clicking outside the tooltip");
  info("Find an element outside the tooltip");
  let root = rootElement(view);
  let propertyValueNode = root.querySelector(PROPERTYVALUE_SELECTOR);
  let cssDocs = view.tooltips.cssDocs;
  let onHidden = cssDocs.tooltip.once("hidden");
  info("Click the element");
  EventUtils.synthesizeMouseAtCenter(propertyValueNode, {button: 0, type: "click"}, view.doc.defaultView);
  yield onHidden;
  ok(true, "The MDN docs tooltip was hidden on clicking an element outside the tooltip");
}

...and got an error like:

34 INFO TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs-02.js | Uncaught exception - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js :: doApply :: line 161"  data: no]
Stack trace:
    doApply@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:161:10
    wrapPrivileged/callTrap@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:185:31
    synthesizeMouseAtPoint@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:273:26
    synthesizeMouse@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:232:1
    synthesizeMouseAtCenter@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:340:1
    testShowAndHideWithMouseClick@chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs-02.js:101:1

...which I don't understand. Is it fair to say that this is also a generic panel test, and therefore doesn't need a test here? Otherwise I'm happy to add this, but would need some help understanding how to use this API.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #35)
> Comment on attachment 8581041 [details] [diff] [review]
> mdn-tooltips-3.patch
> 
> Review of attachment 8581041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. I think the main tests are in place. I only have a few comments
> regarding refactoring and splitting tests. Other than this, I'm happy with
> the patch.
> I'll take just one last look when the comments are addressed, and we should
> be good to go then.
> 
> ::: browser/devtools/shared/test/browser_mdn-docs.js
> @@ +1,4 @@
> > +/* vim: set ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> 
> Very nice test, I like the comments and the code is easy to read.
> One general comment though, it helps maintainability if tests are small and
> test one thing only. They're even easier to read and debug and they run
> faster (which helps a lot when running on our test infra).
> So I'd like you to split this in several test files:
> - one that tests the basics,
> - one that tests the missing page case,
> - one that tests the various missing elements cases

As you suggested later on, I've now got:

browser/devtools/shared/test/browser_mdn-docs-01.js
- test everything, with a "normal" docs page
browser/devtools/shared/test/browser_mdn-docs-02.js 
- test that we can also handle different docs page structures, pieces missing, pages that don't exist at all.
> 
> @@ +59,5 @@
> > +  yield promiseTab("about:blank");
> > +  let [host, win, doc] = yield createHost("bottom", MDN_DOCS_TOOLTIP_FRAME);
> > +  let widget = new MdnDocsWidget(win.document);
> > +
> > +  // test all the basics
> 
> I think a good rule of thumb in tests is: whenever you feel like adding a
> line comment like this one (and others below), add an 'info("...")' instead,
> with the same comment.
> It's important because when tests start to fail intermittently on remote
> servers, all we have at our disposal to debug them are logs, and the first
> step is figuring out which line the test fails exactly. So commenting each
> important step with info() statement is good.
> So just wrap these comments in info()

Done.

> 
> @@ +79,5 @@
> > +  yield testNoSummaryOrSyntax(widget);
> > +
> > +  host.destroy();
> > +  gBrowser.removeCurrentTab();
> > +
> 
> nit: empty line.
> 
> @@ +92,5 @@
> > + * - mdn link text is correct and onclick behavior is correct
> > + */
> > +function* testTheBasics(widget) {
> > +  info("Test all the basic functionality in the widget");
> > +  let doc = widget.docElements;
> 
> doc isn't the best name here as this variable name is usually used for
> document. Maybe change it to let widgetElements, or just don't define it and
> always use widget.docElements.
> In fact, you could perhaps rename docElements to elements in MdnDocsWidget.js


Good suggestion! I did that.

> 
> @@ +123,5 @@
> > +
> > +  info("Check that MDN link text is correct and onclick behavior is correct");
> > +  yield checkMdnLink(BASIC_TESTING_PROPERTY, widget);
> > +
> > +  function* checkMdnLink(testProperty, widget) {
> 
> This function isn't used anywhere else, maybe we don't need that function at
> all and just keep its code.
> If the goal is to separate it out from the testTheBasics test step function,
> then fine, but maybe put it outside of the function, at the bottom of this
> file. Same for checkLinkClick.
> This would help people adding new test stop functions to this file later and
> reuse these if necessary.

I've added it to the end of the file.

> 
> @@ +180,5 @@
> > +  }
> > +
> > +}
> > +
> > +function* testNonExistentPage(widget) {
> 
> This function and the next 4 functions look very similar. They look like
> excellent candidates for a little bit of refactoring.
> We often use this approach in devtools tests:
> 
> const TEST_DATA = [{
>   desc: "Test a property for which we don't have a page",
>   docsPageUrl: "i-dont-exist.html",
>   expectedContents: {
>     propertyName: "i-dont-exist.html",
>     summary: ERROR_MESSAGE,
>     syntax: ""
>   }
> }, {
>   desc: "Test a property whose syntax section is specified using an ID",
>   docsPageUrl: SYNTAX_BY_ID,
>   expectedContents: {
>     propertyName: SYNTAX_BY_ID,
>     summary: BASIC_EXPECTED_SUMMARY,
>     syntax: BASIC_EXPECTED_SYNTAX
>   }
> }, {
>   ...
> }];
> 
> And then:
> 
> for (let {desc, docsPageUrl, expectedContents} of TEST_DATA) {
>   info(desc);
>   yield widget.loadCssDocs(docsPageUrl);
>   checkTooltipContents(widget.docsElements, expectedContents);
> }
> 
> This makes it super easy to add new test cases later, and makes it easier to
> read the code too.
> So maybe you could just split this file in 2: one for the basics of loading
> and interacting with the tooltip, and another test with these cases.
> 

Done.

> :::
> browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-
> docs.js
> @@ +26,5 @@
> > +const PROPERTYNAME = "color";
> > +
> > +add_task(function* () {
> > +
> > +  const TEST_DOC = '<html>                                              \
> 
> template strings with `` are multiline and don't need \ at the end of each
> line, so maybe use that instead.

Done.

> 
> @@ +41,5 @@
> > +  yield selectNode("div", inspector);
> > +
> > +  yield testMdnContextMenuItemVisibility(view);
> > +
> > +  yield testShowAndHideMdnTooltip(view);
> 
> This should be separated out in a second test. To keep this one small and
> easy to maintain, fast to run.

Done.

> 
> @@ +43,5 @@
> > +  yield testMdnContextMenuItemVisibility(view);
> > +
> > +  yield testShowAndHideMdnTooltip(view);
> > +
> > +  yield clearCurrentNodeSelection(inspector);
> 
> This looks like this was stolen from
> browser_styleinspector_context-menu-copy-color_01.js, which is fine, but
> since I don't remember why this is there, I'd like if this was removed, and
> we test without it. If it really needs to be here, we should investigate why
> and either fix the cause or add a comment here saying why it's needed.


I took this out. Yes, much of this test was stolen from the other context menu test. There's definitely scope for refactoring context menu tests.

> 
> @@ +63,5 @@
> > +
> > +  let root = rootElement(view);
> > +  for (let node of iterateNodes(root)) {
> > +    info("Setting " + node + " as popupNode");
> > +    if (view.doc) {
> 
> What is this if/else about? Isn't the rule-view document always available at
> view.doc?

Also stolen, also now removed.

> @@ +73,5 @@
> > +
> > +    info("Updating context menu state");
> > +    view._contextMenuUpdate();
> > +    let isVisible = !view.menuitemShowMdnDocs.hidden;
> > +    let shouldBeVisible = isPropertyNameNode(node);
> 
> You should also test that right-clicking on a selector or a value that look
> like a property name doesn't show the menu item:
> 
> padding {
>   color: red;
> }
> 
> or 
> 
> selector {
>   content: "margin";
> }

Done.

> 
> @@ +93,5 @@
> > +
> > +  info("Setting the popupNode for the MDN docs tooltip");
> > +  let root = rootElement(view);
> > +  let propertyNameSpan = root.querySelector(PROPERTYNAME_SELECTOR);
> > +  if (view.doc) {
> 
> When can view.doc be falsy?
> I see now that this came from
> browser_styleinspector_context-menu-copy-color_01.js, but it looks odd, and
> so again, we should either explain in the code why it's here or get rid of
> it.

Also stolen, also now removed.

> @@ +109,5 @@
> > +
> > +  info("Quick check that the tooltip contents are set");
> > +  let tooltipDocument = cssDocs.tooltip.content.contentDocument;
> > +  let h1 = tooltipDocument.getElementById("property-name");
> > +  is(PROPERTYNAME, h1.textContent, "The MDN docs tooltip h1 is correct");
> 
> The correct parameter order is: is(actual, expected, info), so
> is(h1.textContent, PROPERTYNAME, "The MDN docs tooltip h1 is correct");
> Not a big deal, but it helps reading logs for failed tests.
> 

Done.

> @@ +138,5 @@
> > +  }
> > +}
> > +
> > +/**
> > + * Returns the root element for the given view, rule or computed.
> 
> Why computed? The tooltip only works in the rule-view so far, right? And
> this test only opens the rule-view anyway.

Also stolen, also now removed.

Comment 45

[Will Bamberg [:wbamberg]]

More things:

Doc structure
*************
At the Hack on MDN weekend in Berlin, we did some work to (re)define the structure of CSS property pages, and updated the 100 most popular CSS properties with that structure. I've updated the DOM-scraping code to handle that structure. The updates to the content make the tooltip content much better.

Localized pages
***************
In the original version of this patch, I naively thought that I could get versions of pages in the browser's language just by omitting the locale from the URL.

But this doesn't actually work: although I do get translated pages, the heading IDs themselves are the same as the heading text content, so DOM scraping code that uses getElementById("Summary") won't work for a French page (it would need getElementById("R.C3.A9sum.C3.A9")). Ugh.

So I can think of 2 options:

1. give up on fetching translated pages entirely, and just use en-US in the URL.
2. try to scrape the DOM without reference to IDs. I don't love this approach, because it's even less explicit than the original.

Based on discussion with the MDN team, the recommendation is to go with (1) in the first iteration of this patch. The main reason for this is that while en-US pages are by and large well-structured, other locales are quite variable (although some locales such as fr are good).

We (the MDN team) are going to put some work into improving the structure of all pages, and we can let in other locales when we have done this.

But please let me know if you think this is a blocker.

Syntax highlighting
*******************
Having played with a version that has syntax highlighting, I'd really like to get that in: maybe not in this bug, but before Firefox 40. I fully understand if you think my quick and dirty syntax highlighter is too fragile. Luckily it looks like bug 1152033 will be what we need, although I've not tried it yet. Trying this out will be the next thing for me.

Caching
*******
I appreciate Luke's concern about server load and his desire to investigate Kuma's caching in bug 1142146. I'm a bit worried, though, that the server-side change could break this code. This isn't a thing I understand at all well, but testing it out a bit, here's what I see.

If I send (chrome-privileged, x-domain) XHR requests to a server that supports cache negotiations, then in the (Browser Toolbox) Network Monitor I see 304 responses coming back from the server, but in my XHR code, I see 200 responses, and the response data is there. So it looks as if the caching is completely transparent to my code. If that's true, this is great, and means things will just work when Kuma starts sending 304 responses.

Still, if bug 1142146 does block this one, can we expect bug 1142146 to land in time for us to ship this feature in Firefox 40?

Comment 46

[Patrick Brosset <:pbro>]

Comment on attachment 8591995 [details] [diff] [review]
new patch

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

Looks good to me now.
I only have a few nits.

Also, some comments about the info in comment 45:
(In reply to Will Bamberg [:wbamberg] from comment #45)
> Doc structure
> *************
> At the Hack on MDN weekend in Berlin, we did some work to (re)define the
> structure of CSS property pages, and updated the 100 most popular CSS
> properties with that structure. I've updated the DOM-scraping code to handle
> that structure. The updates to the content make the tooltip content much
> better.
Awesome!
> Localized pages
> ***************
> In the original version of this patch, I naively thought that I could get
> versions of pages in the browser's language just by omitting the locale from
> the URL.
> 
> But this doesn't actually work: although I do get translated pages, the
> heading IDs themselves are the same as the heading text content, so DOM
> scraping code that uses getElementById("Summary") won't work for a French
> page (it would need getElementById("R.C3.A9sum.C3.A9")). Ugh.
> 
> So I can think of 2 options:
> 
> 1. give up on fetching translated pages entirely, and just use en-US in the
> URL.
> 2. try to scrape the DOM without reference to IDs. I don't love this
> approach, because it's even less explicit than the original.
> 
> Based on discussion with the MDN team, the recommendation is to go with (1)
> in the first iteration of this patch. The main reason for this is that while
> en-US pages are by and large well-structured, other locales are quite
> variable (although some locales such as fr are good).
> 
> We (the MDN team) are going to put some work into improving the structure of
> all pages, and we can let in other locales when we have done this.
> 
> But please let me know if you think this is a blocker.
Not a blocker, I agree with the approach.
> Syntax highlighting
> *******************
> Having played with a version that has syntax highlighting, I'd really like
> to get that in: maybe not in this bug, but before Firefox 40. I fully
> understand if you think my quick and dirty syntax highlighter is too
> fragile. Luckily it looks like bug 1152033 will be what we need, although
> I've not tried it yet. Trying this out will be the next thing for me.
Yes please not in this bug. The bug has a lot of comments now and the patch is big, it's getting hard for me to know what has changed since last time. Can you file a follow-up bug? We can flag it with the [devedition-40] whiteboard and ping Jeff so he can also weight in on the priority for this.

Finally, what about the security review you mentioned a while ago (comment 29)?

::: browser/devtools/shared/test/browser_mdn-docs-01.js
@@ +52,5 @@
> +  yield testTheBasics(widget);
> +
> +  host.destroy();
> +  gBrowser.removeCurrentTab();
> +

nit: get rid of this empty line.

@@ +124,5 @@
> +  * actual page. So we ignore that first load event, and keep
> +  * listening until "load" is triggered for a different URI.
> +  */
> +function checkLinkClick(link) {
> +

nit: indentation is off by 1 space in this function.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +762,2 @@
>     */
> +  setIFrameContent: function({width, height}, url) {

Unfortunately, a new type of tooltip landed since you made this change: css filters.
So, could you please also refactor setFilterContent to make use of this new setIFrameContent helper, to avoid code duplication there too?

::: browser/devtools/shared/widgets/mdn-docs.css
@@ +1,1 @@
> +

This file needs the header license like the one here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/filter-widget.css#1

::: browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs-01.js
@@ +16,5 @@
> +
> +"use strict";
> +
> +const {setBaseCssDocsUrl} = devtools.require("devtools/shared/widgets/MdnDocsWidget");
> +Cu.import("resource://gre/modules/Promise.jsm");

Promise.jsm is already imported in browser/devtools/styleinspector/test/head.js which runs before your test, so you can remove this line.

::: browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs-02.js
@@ +17,5 @@
> +
> +"use strict";
> +
> +const {setBaseCssDocsUrl} = devtools.require("devtools/shared/widgets/MdnDocsWidget");
> +Cu.import("resource://gre/modules/Promise.jsm");

Promise.jsm is already imported in browser/devtools/styleinspector/test/head.js which runs before your test, so you can remove this line.

@@ +46,5 @@
> +  setBaseCssDocsUrl(TEST_URL_ROOT);
> +
> +  info("Setting the popupNode for the MDN docs tooltip");
> +  let root = rootElement(view);
> +  let propertyNameSpan = root.querySelector(PROPERTYNAME_SELECTOR);

We already have a helper function in head.js to do this, which you could use like this:
let {nameSpan} = getRuleViewProperty(view, "element", color);
although I'm not sure it works with the "element" selector, but it's worth a try as it would remove the need for these 2 lines and the PROPERTYNAME_SELECTOR const.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +55,4 @@
>  #LOCALIZATION NOTE: Used in the image preview tooltip when the image could not be loaded
>  eventsTooltip.openInDebugger=Open in Debugger
>  
> +# LOCALIZATION NOTE (docsTooltip.visitMDN): Link to visit a documentation page on MDN.

Can you also say where is this link displayed?

Comment 47

[Will Bamberg [:wbamberg]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #46)
> Comment on attachment 8591995 [details] [diff] [review]
> new patch
> 
> Review of attachment 8591995 [details] [diff] [review]:
> -----------------------------------------------------------------
> Finally, what about the security review you mentioned a while ago (comment
> 29)? 

Mark, did you get a chance to have a proper look at this patch?

Comment 48

[Luke Crouch [:groovecoder]]

Re: MDN caching ... we're very worried about the potential 20% spike of requests this could send to MDN's document view, because it's a very heavy view. When crawlers hit MDN document views heavily, we see slow-downs for other users.

AIUI, Firefox 40 lands in Dev Edition on May 12?

:wbamberg - looks like you're just using a '?raw&macros' request and parsing the entire content? Is it too late to change this to request some specific MDN API endpoints for the summary and syntax content? e.g., https://developer.mozilla.org/en-US/docs/Web/CSS/vertical-align?raw&macros&section=Summary and https://developer.mozilla.org/en-US/docs/Web/CSS/vertical-align?raw&macros&section=Syntax ?

(It also looks like you're appending the utm_* url parameters to the API request. Please move them to the linkToMdn href value instead. We want to track the click-thrus to MDN, not the requests to the API.)

Comment 49

[Will Bamberg [:wbamberg]]

Update from Luke on the Kuma caching issue:

> "We're going to work with WebOps to check out the cache
> setup on our load balancer and decide by next Friday if
> we should try to fix LB, mdn code, or client. Is that a
> good decision timeline for you? Can the feature be perf
> off or on?"

My worry is that every week this gets delayed is another week that it doesn't get tested in Nightlies before hitting the Dev Edition. Firefox 40 branches on May 11th.

As things stand it isn't controlled by a pref, although I imagine it's not too difficult to add this.

Comment 50

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Will Bamberg [:wbamberg] from comment #49)
...
> As things stand it isn't controlled by a pref, although I imagine it's not
> too difficult to add this.

The feature should be controlled by a pref - this can be done in a followup bug though.

Comment 51

[Luke Crouch [:groovecoder]]

Since it's 2 bugs deep, here's the WebOps bug for investigating MDN caching:

https://bugzilla.mozilla.org/show_bug.cgi?id=1155308

Comment 52

[Luke Crouch [:groovecoder]]

Jeff - where are you getting your 90k/day weekly peak and 7,500/hr max estimates?

Last week's aurora 39 telemetry [1] shows peaks in the 11k-13k/day range, and aurora 38 (before it went to beta) also shows peaks in the 16k-20k/day range. [2] Those peaks would be 520/hr to 750/hr - an order of magnitude smaller than 7,500/hr.

If we're realistically looking at 520-750 requests per hour, we don't need to worry nearly so much about the MDN caching & response time.

[1] http://telemetry.mozilla.org/#filter=aurora%2F39%2FDEVTOOLS_INSPECTOR_OPENED_BOOLEAN%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph
[2] http://telemetry.mozilla.org/#filter=aurora%2F38%2FDEVTOOLS_INSPECTOR_OPENED_BOOLEAN%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

Comment 53

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Luke Crouch [:groovecoder] from comment #52)
> Jeff - where are you getting your 90k/day weekly peak and 7,500/hr max
> estimates?
> 
> Last week's aurora 39 telemetry [1] shows peaks in the 11k-13k/day range,
> and aurora 38 (before it went to beta) also shows peaks in the 16k-20k/day
> range. [2] Those peaks would be 520/hr to 750/hr - an order of magnitude
> smaller than 7,500/hr.
> 
> If we're realistically looking at 520-750 requests per hour, we don't need
> to worry nearly so much about the MDN caching & response time.
> 
> [1]
> http://telemetry.mozilla.org/
> #filter=aurora%2F39%2FDEVTOOLS_INSPECTOR_OPENED_BOOLEAN%2Fsaved_session%2FFir
> efox&aggregates=multiselect-all!
> Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph
> [2]
> http://telemetry.mozilla.org/
> #filter=aurora%2F38%2FDEVTOOLS_INSPECTOR_OPENED_BOOLEAN%2Fsaved_session%2FFir
> efox&aggregates=multiselect-all!
> Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

( as a writer-turned-programmer-turned-PM, this struck at the very heart of my imposter syndrome - that I was getting stats very wrong somehow )

The dashboard is a bit misleading, because it's only giving you the number of sessions per day that included data related to this measure, and not actually anything about the data itself.

To sum it up:

11:01 AM <mreid> it happens in 20k sessions per day
11:01 AM <mreid> but it happens 90k times

The dashboard is using the submissions() method to get that 20k number ( example date is March 25th for Aurora 38 ). This is not the useful part of this histogram, the count() method is the actually meaningful part - the buckets of counts. See:

http://telemetry.mozilla.org/docs.html#Telemetry.Histogram.count

vs

http://telemetry.mozilla.org/docs.html#Telemetry.Histogram.submissions

Comment 54

[Luke Crouch [:groovecoder]]

I did some load-testing in https://bugzilla.mozilla.org/show_bug.cgi?id=1155308#c1 up to 180-300 requests *per second* before (I think) I tripped a single IP auto-block threshold on our load balancer. I watched New Relic and saw no spike in requests hitting python, despite the load balancer reporting 94k+ requests. So it looks like our load balancer is correctly serving cached responses in front of the MDN backend.

So, :wbamberg - what we need to do is spot-check these requests specifically to see that they're being served by the cache before hitting the back-end too. Can we do that with a certain Nightly build with some kind of user-agent identifier?

Comment 55

[Will Bamberg [:wbamberg]]

(In reply to Luke Crouch [:groovecoder] from comment #54)
> I did some load-testing in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1155308#c1 up to 180-300
> requests *per second* before (I think) I tripped a single IP auto-block
> threshold on our load balancer. I watched New Relic and saw no spike in
> requests hitting python, despite the load balancer reporting 94k+ requests.
> So it looks like our load balancer is correctly serving cached responses in
> front of the MDN backend.
> 

That sounds encouraging.

> So, :wbamberg - what we need to do is spot-check these requests specifically
> to see that they're being served by the cache before hitting the back-end
> too. Can we do that with a certain Nightly build with some kind of
> user-agent identifier?

You can figure out which Firefox version is asking from the UA string, it should contain "Firefox/40". I don't know if you can figure out that the requests are coming from this feature.

Since Jeff wants this to be preffable anyway, can we do this:
(1) land this patch as soon as pbrosset is happy with it, so it gets exercised in Nightly
(2) make it preffable in a follow-up bug before Firefox 40 branches: then if it looks like being a problem, we have an escape route

Comment 56

[Will Bamberg [:wbamberg]]

Attachment: mdn-tooltips-4.patch

pbrosset, I think I've addressed all your nits from comment 46. A couple of other changes too:

(1) I've fixed the utm_* url parameters, as Luke requested in comment 48

(2) For a while I was seeing intermittent problems with this feature: quite occasionally, the tooltip seemed to get "stuck" on a particular property, for instance "color", so even if you context-clicked on a different property name, the panel would only show you the docs for "color".

I managed to dig into it a bit today. I'm using getNodeInfo() in _contextMenuUpdate(), to get info about the node that was context-clicked, and storing the result in ctxMenuNodeInfo. Sometimes getNodeInfo() would fail at the first occurrence of:

  sheetHref: prop.rule.domRule.href [0]

with an error like:

  TypeError: sheet is null

That would then mean that the ctxMenuNodeInfo object didn't get updated, so the tooltip would show you the docs for the old node info object.

I don't think this is an error I have introduced, since I haven't touched this code, and when we get into this state, the same error occurs when getNodeInfo is called from other places.

In fact, in my original patch I wasn't using getNodeInfo, but code review comments in comment 27 suggested I could change to use it "although this [i.e. my original way] is fine". So for the time being I have changed this patch to to old way, and haven't seen this error recur.

[0] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1374
[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/styles.js#1234

If this patch looks good, and Luke's happy with the suggestion in comment 55, is the next step a try build?

Comment 57

[Patrick Brosset <:pbro>]

Comment on attachment 8595640 [details] [diff] [review]
mdn-tooltips-4.patch

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

Thanks for the new patch.
I see all my comments have been addressed.
The interdiff doesn't show me everything I guess, but what it does show is fine, and I've also gone through the code one last time, didn't see anything left to comment about.
Also tested locally one last time, worked fine.

This is good to go from my point of view.

Comment 58

[Patrick Brosset <:pbro>]

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

Comment 59

[Will Bamberg [:wbamberg]]

There are a couple of failures.

One, that seems to fail in all platforms, in "browser_telemetry_button_responsive.js". This test passed for me when I ran devtools tests locally, and it it's not obvious what in my changes could have caused that test to start failing.

The other is linked to an intermittent, I guess, but it's a bit worrying that it's only appeared in e10s tests, and it is close to code I've touched (rule-view's popupNode).

If you have any pointers for either of those, I'd really appreciate it. Otherwise I will try to understand them tomorrow.

Comment 60

[Patrick Brosset <:pbro>]

(In reply to Will Bamberg [:wbamberg] from comment #59)
> One, that seems to fail in all platforms, in
> "browser_telemetry_button_responsive.js". This test passed for me when I ran
> devtools tests locally, and it it's not obvious what in my changes could
> have caused that test to start failing.
That one is weird. It's seemingly unrelated. I'm not seeing any stack traces in the logs that could point to your patch. The only thing is that it opens the inspector, which is where your popup is, but apart from this I don't see any link. I do see one stack trace a bit before the assertion failure, and it's related to gcli, for which a major change was done yesterday. So one option is that at the time I pushed to try, I may have had a buggy changeset in my repo, that was later backed-out or fixed.

> The other is linked to an intermittent, I guess, but it's a bit worrying
> that it's only appeared in e10s tests, and it is close to code I've touched
> (rule-view's popupNode).
This test (browser_ruleview_content_02.js) is configured *not* to run with e10s. See the browser.ini file in the same directory: it has a skip-if = e10s.
So I don't know what happened there, maybe a bug in the test runner. But it's definitely not related to your patch.

I'll update, and test both locally again, and if ok, I'll just push to try again.

Comment 61

[Patrick Brosset <:pbro>]

So the telemetry test failure is a real problem. The trick is that it doesn't occur on the test alone, but only if you first run the mdn-docs test first.

For instance, this fails:
./mach mochitest-devtools browser/devtools/shared/test

Or this:
./mach mochitest-devtools browser/devtools/shared/test/ --start-at browser_mdn-docs-02.js --end-at browser_telemetry_button_responsive.js

But this works fine:
./mach mochitest-devtools browser/devtools/shared/test/browser_telemetry_button_responsive.js

Comment 62

[Patrick Brosset <:pbro>]

Attachment: mdn-tooltips-5.patch

Turns out the 2 mdn tests that run before the failing telemetry test were causing the problem.
I haven't investigated why exactly, but importing Promise.jsm was the reason.

Here's an updated patch with the fix.
And a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6f55f766e91

Comment 63

[Patrick Brosset <:pbro>]

Interdiff of my changes: https://bugzilla.mozilla.org/attachment.cgi?oldid=8595640&action=interdiff&newid=8597077&headers=1

Comment 64

[Patrick Brosset <:pbro>]

Attachment: mdn-tooltips-6.patch

Of course! I understand now the other failure with e10s. The test that wasn't supposed to even run with e10s was running now because the |skip-if| condition that was supposed to be right after it in browser.ini, got moved down to after the 2 new mdn tests you added.

So, here's yet another patch.
And a new try build (just with e10s, since the last one was all green for the rest): https://treeherder.mozilla.org/#/jobs?repo=try&revision=597739eaf62e

Comment 65

[Patrick Brosset <:pbro>]

Try is green now, so the patch is good to land from my point of view.
Will, I haven't fully followed the discussion around the expected MDN traffic, but if/when that's resolved, feel free to add the checkin-needed keyword.

Comment 66

[Will Bamberg [:wbamberg]]

Thanks so much for resolving those test failures Patrick!

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Please make sure future patches contain commit information.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 68

[Ryan VanderMeulen [:RyanVM]]

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

Comment 69

[Will Bamberg [:wbamberg]]

Ryan: sorry, I will do that.

Comment 70

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a926d4e8ad0

Comment 71

[Luke Crouch [:groovecoder]]

FWIW, we just checked with :fox2mike that most of these new requests are served by the load balancer, so the MDN backend should be "safe" from them.

Comment 72

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Luke Crouch [:groovecoder] from comment #71)
> FWIW, we just checked with :fox2mike that most of these new requests are
> served by the load balancer, so the MDN backend should be "safe" from them.

\o/ glad this worked out.

Comment 73

[Patrick Brosset <:pbro>]

This is done, but I'm adding the devtools-ux flag to put this on the radar for a UX review. There was some discussions about the best user interaction to show MDN tooltips. The current solution is:
- right click on a property name e.g. line-height
- select the MDN menu item from the contextual menu
- the MDN tooltip appears over the property name
- dismiss the tooltip by clicking anywhere outside.