Closed Bug 1102464 Opened 10 years ago Closed 7 years ago

Implement a CSS variable tooltip

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: rajdeep.nanua)

References

(Blocks 1 open bug)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 file, 3 obsolete files)

Bug 1102464 - Implement CSS variable tooltip.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
Given markup like:

<style>
:root {
  --foo: red;
}

div {
  --foo: blue;
}

span {
  color: foo;
}
</style>
<div><span>foo</span></div>

It'd be great to see:

(1) what is the current assignment of foo when inspecting the span
(2) how did it get to be that way

Just brainstorming how this info could be displayed:
1) could be displayed in the rule view somewhere next to the value, or in a tooltip upon hovering the value
2) could be displayed in the computed view where we already have this sort of 'trace' feature.  We would probably want to show all of the variables in scope in the computed view if we did this, and maybe link to the particular one from the rule view.
Getting (2) to work in the computed view should hopefully hook into the normal machinery, since running this in the browser console returns what I'd expect it to:

let require = Cu.import('resource://gre/modules/devtools/Loader.jsm', {}).devtools.require;
let {CssLogic} = require("devtools/styleinspector/css-logic");
let cl = new CssLogic();
cl.highlight(document.documentElement);
cl.getPropertyInfo("--space-above-tabbar");
Depends on: 1102369
This all sounds very good to me.
See also bug 1145527.
Blocks: 1223058
Let's make this one the computed view bug, and bug 1145527 the rule view bug.
Component: Developer Tools: Inspector → Developer Tools: Computed Styles Inspector
Filter on CLIMBING SHOES
Severity: normal → enhancement
Priority: -- → P3
Whiteboard: [btpp-backlog]
Assignee: nobody → rajdeep.nanua
Status: NEW → ASSIGNED
Added the initial tooltip displaying logic. Currently, there is no tooltip for displaying text. So, the initial implementation shows "serif" font preview when hovering over css variables.
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review189974

Code so far looks pretty good.
Attachment #8912976 - Flags: review?(gl)
Attachment #8919466 - Attachment is obsolete: true
Attachment #8919466 - Flags: review?(gl)
Attachment #8922984 - Attachment is obsolete: true
Attachment #8922984 - Flags: review?(gl)
Attachment #8912976 - Flags: review?(gl) → review?(pbrosset)
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review201998

::: commit-message-3828e:1
(Diff revision 3)
> +Bug 1102464: Part 2: Show a trace of how a css variable got set in the inspector. r=gl

We need to fix up this commit message.

::: devtools/client/inspector/rules/rules.js:342
(Diff revision 3)
>          sheetHref: prop.rule.domRule.href,
>          textProperty: prop,
>          toggleActive: getShapeToggleActive(node),
>          point: getShapePoint(node)
>        };
> +    } else if ((classes.contains("ruleview-variable") || classes.contains("ruleview-variable-unmatched")) && prop) {

Eslint complains that Line 342 exceeds the maximum line length of 90.  max-len (eslint)

::: devtools/client/inspector/rules/rules.js:342
(Diff revision 3)
>          sheetHref: prop.rule.domRule.href,
>          textProperty: prop,
>          toggleActive: getShapeToggleActive(node),
>          point: getShapePoint(node)
>        };
> +    } else if ((classes.contains("ruleview-variable") || classes.contains("ruleview-variable-unmatched")) && prop) {

s/ruleview-variable-unmatched/ruleview-unmatched/variable

::: devtools/client/inspector/rules/test/browser_rules_variables_tooltip.js:1
(Diff revision 3)
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

We actually want to modify 

browser_rules_variables_01
browser_rules_variables_02

since the title attribute is gone.

::: devtools/client/inspector/rules/test/doc_variables_tooltip.html:1
(Diff revision 3)
> +<!DOCTYPE html>

This might not be needed anymore

::: devtools/client/inspector/shared/tooltips-overlay.js:180
(Diff revision 3)
>        let value = prop.value.toLowerCase();
>        if (value !== "inherit" && value !== "unset" && value !== "initial") {
>          tooltipType = TOOLTIP_FONTFAMILY_TYPE;
>        }
>      }
>  

Add a comment

// Variable preview tooltip

::: devtools/client/inspector/shared/tooltips-overlay.js:308
(Diff revision 3)
>      yield setImageTooltip(this.getTooltip("previewTooltip"), doc, imageUrl,
>        {hideDimensionLabel: true, hideCheckeredBackground: true,
>         maxDim, naturalWidth, naturalHeight});
>    }),
>  
> +  _setTextPreviewTooltip: Task.async(function* (text, nodeFront) {

Add a JSDoc like you see for _setFontPreviewTooltip.

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:14
(Diff revision 3)
> +const {LocalizationHelper} = require("devtools/shared/l10n");
> +const L10N = new LocalizationHelper("devtools/client/locales/inspector.properties");
> +
> +const XHTML_NS = "http://www.w3.org/1999/xhtml";
> +
> +//Default text tooltip max dimension

Add a space after //

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:30
(Diff revision 3)
> + *        The tooltip instance on which the image preview content should be set
> + * @param {Document} doc
> + *        A document element to create the HTML elements needed for the tooltip
> + * @param {String} text
> + *        Text to display in tooltip
> + * @param {Object} options

Seems like you copy this @param, but we don't actually need it.

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:48
(Diff revision 3)
> +    min-width: 100px;
> +    display: flex;
> +    flex-direction: column;
> +    text-align: center;`;
> +  let html = `
> +    <div style="flex: 1;

I think right now the padding is a bit too much and we should bump up the font size and font family to be consistent with what we see in the rule view properties.
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review202188

::: commit-message-3828e:1
(Diff revision 3)
> +Bug 1102464: Part 2: Show a trace of how a css variable got set in the inspector. r=gl

I agree with Gabriel. The commit message shoudl explain in details what this commit intends to change. You can have a short summary as the first line and then add details as extra lines.
Right now the message is just a copy of the bug title.

::: devtools/client/inspector/shared/tooltips-overlay.js:238
(Diff revision 3)
>        return true;
>      }
>  
> +    if (type === TOOLTIP_VARIABLE_TYPE && nodeInfo.value.value.startsWith("--")) {
> +      let variable = nodeInfo.value.variable;
> +      let nodeFront = inspector.selection.nodeFront;

`nodeFront` isn't needed.

::: devtools/client/inspector/shared/tooltips-overlay.js:308
(Diff revision 3)
>      yield setImageTooltip(this.getTooltip("previewTooltip"), doc, imageUrl,
>        {hideDimensionLabel: true, hideCheckeredBackground: true,
>         maxDim, naturalWidth, naturalHeight});
>    }),
>  
> +  _setTextPreviewTooltip: Task.async(function* (text, nodeFront) {

`nodeFront` isn't used in the body of this function, it should not be an argument.

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:9
(Diff revision 3)
> +const {LocalizationHelper} = require("devtools/shared/l10n");
> +const L10N = new LocalizationHelper("devtools/client/locales/inspector.properties");

L10N doesn't seem to be needed anywhere in this file. You can remove these 2 lines.

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:21
(Diff revision 3)
> + * Set the tooltip content of a provided HTMLTooltip instance to display an
> + * image preview matching the provided imageUrl.

Not an image, a piece of text.

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:61
(Diff revision 3)
> +  let height = LABEL_HEIGHT;
> +  let width = Math.max(CONTAINER_MIN_WIDTH, LETTER_WIDTH * text.length);

This is not going to always work fine. What if text wraps on multiple lines? What if there are longer characters?
Also, for very short text, having a 100px wide tooltip feels a bit weird to me.

It would be much better if we let the tooltip set its own size according to its content automatically instead of having to set it here.

::: devtools/client/shared/widgets/tooltip/TextTooltipHelper.js:64
(Diff revision 3)
> +
> +  // Calculate tooltip dimensions
> +  let height = LABEL_HEIGHT;
> +  let width = Math.max(CONTAINER_MIN_WIDTH, LETTER_WIDTH * text.length);
> +
> +  tooltip.setContent(div, {width, height});

Why not just use the tooltip's `setTextContent` function? It was made specificallt for displaying pieces of text in a tooltip.
I haven't used it lately, so I don't know if this will, work, but I suspect it could handle the auto-sizing nicely, you should try that.

In any case, iy seems so much simpler to me than creating this new helper file, and generating some HTML and CSS for a thing as simple as a text tooltip. Maybe we need something like this later. But for now, if we're only going to show text, I think you should try using `setTextContent` instead.
Attachment #8912976 - Flags: review?(pbrosset)
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review202188

> Why not just use the tooltip's `setTextContent` function? It was made specificallt for displaying pieces of text in a tooltip.
> I haven't used it lately, so I don't know if this will, work, but I suspect it could handle the auto-sizing nicely, you should try that.
> 
> In any case, iy seems so much simpler to me than creating this new helper file, and generating some HTML and CSS for a thing as simple as a text tooltip. Maybe we need something like this later. But for now, if we're only going to show text, I think you should try using `setTextContent` instead.

I'm not sure how to use setTextContent. We need to use tooltips of type Tooltip, whereas we are using HTMLTooltip in rest of tooltip-overlay.js. HTMLTooltip takes in an HTMLDocument, which we get from this.view.inspector.toolbox. Tooltip takes in an XULDocument and passing in the same inspector.toolbox doesn't work. There are some variable specific changes we are planning on making, such as adding a color swatch. So, I have renamed the helper file to VariableTooltipHelper
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review203578

Looks pretty good. I found the tooltip is black when the dark theme is on. We should also add some padding to the tooltip.
Attachment #8912976 - Flags: review?(gl)
Attachment #8912976 - Flags: review?(pbrosset)
Looking at this color swatch commit makes me think we should really display color swatches next to color variable definitions.
Today, we display color swatches here:
color: red;
We should also display them next to:
--foo: red;
If we find that the value is a valid color. 
From experience, variable are often colors, and we have a function that tests if a string is a color, so doing this should be fairly easy and very useful. 
Another idea that comes to mind is linking var usage to var definition. So you could have an icon/button/something next to var(--foo) that scrolls you down to here --foo is defined, so you can change it there.
Comment on attachment 8926712 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/197952/#review204180

::: devtools/client/shared/output-parser.js:235
(Diff revision 1)
> +        let swatch = this._createNode("span", {
> +          style: "background-color:" + varValue +
> +                "; width: 1em; height: 1em; border-radius: 50%; margin-left: 0.5em"
> +        });

I'm not a big fan of having to generate this markup here. We already show color swatches in other places of the code, which means we already have some html/css code somewhere for this. And this is duplicating it.

I would suggest a different approach: no changes to output-parser.js (after all it's not its job to worry about what will be in the tooltip, its job is to generate a DOM fragment to reprsent a CSS property value). Instead, use colorUtils.isValidCSSColor in VariableTooltipHelper to check if the value is a color, and if yes, generate the swatch.
Also, please use existing CSS classes to style the swatch, instead of harcoding the radius, margin, etc... in JS. This way if we decide to change how color swatches look in the future, we can do it in one go in the CSS file.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:38
(Diff revision 1)
>           style="flex: 1;
>                  display: flex;
>                  padding: ${PADDING}px;
>                  align-items: center;
>                  justify-content: center;
>                  min-height: 1px;">

We need to move this styling to a CSS file instead of here in JS.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:44
(Diff revision 1)
>                  ${text}
> +                ${swatch}

Not a big thing amd I can be convinced otherwise, but in the rule view we show:

property: swatch value;

but here we show:

varName = value swatch

Maybe for consistency, we should move the swatch before the value:

varName = swatch value
Comment on attachment 8926712 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/197952/#review204180

> I'm not a big fan of having to generate this markup here. We already show color swatches in other places of the code, which means we already have some html/css code somewhere for this. And this is duplicating it.
> 
> I would suggest a different approach: no changes to output-parser.js (after all it's not its job to worry about what will be in the tooltip, its job is to generate a DOM fragment to reprsent a CSS property value). Instead, use colorUtils.isValidCSSColor in VariableTooltipHelper to check if the value is a color, and if yes, generate the swatch.
> Also, please use existing CSS classes to style the swatch, instead of harcoding the radius, margin, etc... in JS. This way if we decide to change how color swatches look in the future, we can do it in one go in the CSS file.

I agree. My goal was to have swatch styling similar to the ruleview. But, the styling for ruleview swatches is in client/themes/rules.css. Variable tooltip doesn't pick up this styling because it is server side. I'm not sure if I should move the existing styling to a different css file that the tooltip picks up. 

So, for now I hardcoded the style. But, I do intend to change it once I figure out how to get VariableTooltipHelper to pick up ruleview-swatch styling in rules.css.
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review204780

::: devtools/client/inspector/rules/test/browser_rules_variables_02.js:36
(Diff revision 4)
> +  let previewTooltip = yield assertShowPreviewTooltip(view, unsetVar);
> +  yield assertTooltipHiddenOnMouseOut(previewTooltip, unsetVar);

I'm a bit scared that showing/hiding the tooltip every time is going to make the test quite a bit slower.
It's very good to test it once (in variables_01 for instance), but I would not do it again in this test at all.

::: devtools/client/shared/output-parser.js:238
(Diff revision 4)
>        secondOpts.class = options.unmatchedVariableClass;
>      } else {
>        // The variable name is not valid, mark it unmatched.
>        firstOpts.class = options.unmatchedVariableClass;
> -      firstOpts.title = STYLE_INSPECTOR_L10N.getFormatStr("rule.variableUnset",
> +      firstOpts["data-variable"] = STYLE_INSPECTOR_L10N.getFormatStr("rule.variableUnset",
>                                                            varName);

nit: please vertically align the parameter on this second line with the parameter on the first line.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:28
(Diff revision 4)
> + */
> +function setVariableTooltip(tooltip, doc, text) {
> +  // Create tooltip content
> +  let div = doc.createElementNS(XHTML_NS, "div");
> +  div.style.cssText = `
> +    height: 100%;

I tested without this 100% height and it looked fine, maybe it's not needed.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:29
(Diff revision 4)
> +function setVariableTooltip(tooltip, doc, text) {
> +  // Create tooltip content
> +  let div = doc.createElementNS(XHTML_NS, "div");
> +  div.style.cssText = `
> +    height: 100%;
> +    min-width: 10px;

Do we really need a min-width? The tooltip is only going to be displayed if we detected a css variable that starts with --, so there's always going to be some txt in the tooltip and this text is most probably always going to be more than 10px wide.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:30
(Diff revision 4)
> +  // Create tooltip content
> +  let div = doc.createElementNS(XHTML_NS, "div");
> +  div.style.cssText = `
> +    height: 100%;
> +    min-width: 10px;
> +    max-width: 300px;

Removing the max-width allows for cases like this to work fine too:

```
some rule {
  --a-very-long-variable-name-because-why-not-right: red;
  background: var(--a-very-long-variable-name-because-why-not-right);
}
```

Otherwise this wraps.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:31
(Diff revision 4)
> +    display: flex;
> +    flex-direction: column;
> +    text-align: center;`;

I don't think these 3 lines of CSS are needed anymore. The text isn't centered.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:45
(Diff revision 4)
> +  // eslint-disable-next-line no-unsanitized/property
> +  div.innerHTML = html;

This is actually a security issue.
It is possible to craft a css variable value in such a way that privileged HTML/JS code can run in the devtools UI:

```
some rule {
  --inject: <button>test</button>;
  color: var(--inject);
}
```

Please use textContent instead.

So, in fact, after applying the simplifications listed above, the function could just be:

```
function setVariableTooltip(tooltip, doc, text) {
  // Create tooltip content
  let div = doc.createElementNS(XHTML_NS, "div");
  div.classList.add("devtools-monospace");
  div.style.padding = PADDING + "px";
  div.textContent = text;

  tooltip.setContent(div);
}
```
Attachment #8912976 - Flags: review?(pbrosset)
(In reply to Rajdeep Nanua from comment #21)
> I agree. My goal was to have swatch styling similar to the ruleview. But,
> the styling for ruleview swatches is in client/themes/rules.css. Variable
> tooltip doesn't pick up this styling because it is server side. I'm not sure
> if I should move the existing styling to a different css file that the
> tooltip picks up. 
> 
> So, for now I hardcoded the style. But, I do intend to change it once I
> figure out how to get VariableTooltipHelper to pick up ruleview-swatch
> styling in rules.css.
I think the other tooltips we have all have their styles in \devtools\client\themes\tooltips.css
So, at the very least, you can move your styles for the swatch here.
But the right long-term fix would be to have our styles for color swatches in just one place. Not sure what would be the best place for this yet.
Comment on attachment 8926712 [details]
Bug 1102464 - Implement CSS variable tooltip.

2 commits need to be squashed
Attachment #8926712 - Flags: review?(gl)
Attachment #8926712 - Attachment is obsolete: true
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review209364

One final CSS simplification, looks great otherwise. Thank you.

::: devtools/client/inspector/shared/tooltips-overlay.js:317
(Diff revision 6)
> +   *        The text to display for the variable tooltip
> +   * @return {Promise} A promise that resolves when the preview tooltip content is ready
> +   */
> +  _setVariablePreviewTooltip: Task.async(function* (text) {
> +    let doc = this.view.inspector.panelDoc;
> +    let fillStyle = getColor("body-color");

I don't think it is necessary to go through JS to get the color. All the theme variables are available from your tooltip, so you can use them there directly.

So, I suggest removing this line.

::: devtools/client/inspector/shared/tooltips-overlay.js:318
(Diff revision 6)
> +   * @return {Promise} A promise that resolves when the preview tooltip content is ready
> +   */
> +  _setVariablePreviewTooltip: Task.async(function* (text) {
> +    let doc = this.view.inspector.panelDoc;
> +    let fillStyle = getColor("body-color");
> +    yield setVariableTooltip(this.getTooltip("previewTooltip"), doc, text, {fillStyle});

And here remove the {fillStyle}

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:11
(Diff revision 6)
> +
> +"use strict";
> +
> +const XHTML_NS = "http://www.w3.org/1999/xhtml";
> +
> +const PADDING = 2;

You can also remove this, we can define this in CSS.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:23
(Diff revision 6)
> + * @param {Object} options
> + *        - {String} fillStyle is the text color
> + */
> +function setVariableTooltip(tooltip, doc, text, options) {

You can remove the options parameter here.

::: devtools/client/shared/widgets/tooltip/VariableTooltipHelper.js:30
(Diff revision 6)
> +  div.classList.add("devtools-monospace");
> +  div.style.padding = PADDING + "px";
> +  div.style.color = fillStyle;

And instead of defining the padding and color from JS here, you can instead do something like this:

div.classList.add("devtools-monospace", "devtools-tooltip-css-variable");

And then, add the following new rule inside devtools/client/themes/tooltips/css:

/* Tooltip: CSS variables tooltip */

.devtools-tooltip-css-variable {
  color: var(--theme-body-color);
  padding: 2px;
}

This way, everything is defined in CSS, where it belongs. No need to define these things in JS.
Attachment #8912976 - Flags: review?(pbrosset)
Comment on attachment 8912976 [details]
Bug 1102464 - Implement CSS variable tooltip.

https://reviewboard.mozilla.org/r/184334/#review209658
Attachment #8912976 - Flags: review?(pbrosset) → review+
Blocks: 1413310
Summary: Show a trace of how a css variable got set in the Inspector → Implement a CSS variable tooltip
This also fails devtools devtools/client/shared/test/browser_outputparser.js on Windows 7 debug without e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=150294779&repo=mozilla-inbound
Backed out changeset b5950ede2b76 (bug 1102464) for failing devtools on Windows 7 debug at devtools/client/shared/test/browser_outputparser.js r=backout on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b5950ede2b7613fb84dcaf298c951e3c09a936fa

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9b8bbfd224edaf9f3ced038feff465bc1d7e65

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=150329945&repo=mozilla-inbound&lineNumber=8401
Flags: needinfo?(rajdeep.nanua)
https://hg.mozilla.org/mozilla-central/rev/887a4e41b70d
https://hg.mozilla.org/mozilla-central/rev/7223d0316537
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Flags: needinfo?(rajdeep.nanua)
Product: Firefox → DevTools
Component: Inspector: Computed → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: