1373339 - Add a button to toggle the CSS shapes highlighter

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Mike Park [:mparkms]]

There should be a button to toggle the CSS shapes highlighter created in bug 1282716 on and off. This button should be in the rules view, next to the clip-path/shape-outside definition, like the grid highlighter toggle, and should be behind a preference and not visible by default.

Comment 1

[Sebastian Zartner [:sebo]]

If I understand correctly, the highlighter created in bug 1282716 doesn't have any user-visible UI yet, right?

Sebastian

Comment 2

[Mike Park [:mparkms]]

Yes, that's right. Currently the only way to activate the highlighter is by selecting a node with clip-path in the inspector, and executing the following code in the browser toolbox's console:

/* -sp-context:browser */
var t = [...gDevTools._toolboxes][0][1];
var i = t.getPanel("inspector");
t.highlighterUtils.getHighlighterByType("ShapesHighlighter").then(h => {
  h.show(i.selection.nodeFront, {mode: "cssClipPath"}).then(() => {
    // highlighter is now shown
  }, e => console.error("could not show", e));
}, e => console.error("could not get highlighter", e));

Comment 3

[Sebastian Zartner [:sebo]]

I see. Thank you for the detailed info how to turn the highlighter on!

Sebastian

Comment 4

[Mike Park [:mparkms]]

Attachment: Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

Requires pref "devtools.inspector.shapesHighlighter.enabled" to be true.

Review commit: https://reviewboard.mozilla.org/r/150978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150978/

Comment 5

[Mike Park [:mparkms]]

Comment on attachment 8879630 [details]
Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150978/diff/1-2/

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8879630 [details]
Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

https://reviewboard.mozilla.org/r/150978/#review164626

::: commit-message-55b91:1
(Diff revision 2)
> +Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter. r=zer0

s/r=zer0/r=gl

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_01.js:25
(Diff revision 2)
> +
> +const HIGHLIGHTER_TYPE = "ShapesHighlighter";
> +const CSS_SHAPES_ENABLED_PREF = "devtools.inspector.shapesHighlighter.enabled";
> +
> +add_task(function* () {
> +  SpecialPowers.setBoolPref(CSS_SHAPES_ENABLED_PREF, true);

Instead of doing this in every shapes test file. We should set this in rules/test/head.js

See http://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/test/head.js#19

::: devtools/client/inspector/rules/views/text-property-editor.js:364
(Diff revision 2)
>        filterSwatchClass: SHARED_SWATCH_CLASS + " " + FILTER_SWATCH_CLASS,
>        gridClass: "ruleview-grid",
>        defaultColorType: !propDirty,
>        urlClass: "theme-link",
> -      baseURI: this.sheetHref
> +      baseURI: this.sheetHref,
> +      shapeClass: "ruleview-shape"

Move this after gridClass

::: devtools/client/inspector/shared/highlighters-overlay.js:42
(Diff revision 2)
>    this.selectorHighlighterShown = null;
> +  // NodeFront of the shape that is highlighted
> +  this.shapesHighlighterShown = null;
>    // Saved state to be restore on page navigation.
>    this.state = {
>      // Only the grid highlighter state is saved at the moment.

We can remove this.

::: devtools/client/inspector/shared/highlighters-overlay.js:323
(Diff revision 2)
>      this.geometryEditorHighlighterShown = null;
>    }),
>  
>    /**
>     * Restore the saved highlighter states.
> -   *
> +   * @param {String} name the name of the highlighter to be restored

Add a new line to separate the jsdoc and @param

::: devtools/client/inspector/shared/highlighters-overlay.js:324
(Diff revision 2)
>    }),
>  
>    /**
>     * Restore the saved highlighter states.
> -   *
> +   * @param {String} name the name of the highlighter to be restored
> +   * @param {String} selector the selector of the node that was previously highlighted

Put the parameter description on a new line amd capitalize

::: devtools/client/inspector/shared/highlighters-overlay.js:459
(Diff revision 2)
>    _isRuleViewDisplayGrid: function (node) {
>      return this.isRuleView && node.classList.contains("ruleview-grid");
>    },
>  
>    /**
> +   * Is the current clicked node have the shapes highlighter toggle in the

s/clicked node have the shapes/clicked node the shapes

::: devtools/client/inspector/shared/highlighters-overlay.js:460
(Diff revision 2)
>      return this.isRuleView && node.classList.contains("ruleview-grid");
>    },
>  
>    /**
> +   * Is the current clicked node have the shapes highlighter toggle in the
> +   * rule-view

s/rule-view/rule-view.

::: devtools/client/inspector/shared/highlighters-overlay.js:556
(Diff revision 2)
>      this._lastHovered = null;
>      this._hideHoveredHighlighter();
>    },
>  
>    /**
>     * Handler function for "markupmutation" events. Hides the grid highlighter if the grid

Update the JSDoc to include the shapes highlighter

::: devtools/client/inspector/shared/highlighters-overlay.js:600
(Diff revision 2)
>     * Restore saved highlighter state after navigate.
>     */
>    onNavigate: Task.async(function* () {
>      try {
> -      yield this.restoreState();
> +      yield this.restoreState("grid", this.state.grid,
> +                              this.showGridHighlighter.bind(this));

Bind these functions on the class initialization

::: devtools/client/inspector/shared/highlighters-overlay.js:614
(Diff revision 2)
>     * Clear saved highlighter shown properties on will-navigate.
>     */
>    onWillNavigate: function () {
>      this.geometryEditorHighlighterShown = null;
>      this.gridHighlighterShown = null;
> +    this.shapesHighlighterShown = null;

Move this after this.selectorHighlighterShown to keep it kinda alphabetically ordered.

::: devtools/client/inspector/shared/highlighters-overlay.js:649
(Diff revision 2)
>      this.supportsHighlighters = null;
>      this.state = null;
>  
>      this.geometryEditorHighlighterShown = null;
>      this.gridHighlighterShown = null;
> +    this.shapesHighlighterShown = null;

Same as above.

::: devtools/client/shared/output-parser.js:16
(Diff revision 2)
>  const {
>    ANGLE_TAKING_FUNCTIONS,
>    BEZIER_KEYWORDS,
>    COLOR_TAKING_FUNCTIONS,
> -  CSS_TYPES
> +  CSS_TYPES,
> +  BASIC_SHAPE_FUNCTIONS

Move this after ANGLE_TAKING_FUNCTIONS

::: devtools/client/shared/output-parser.js:81
(Diff revision 2)
>    parseCssProperty: function (name, value, options = {}) {
>      options = this._mergeOptions(options);
>  
>      options.expectCubicBezier = this.supportsType(name, CSS_TYPES.TIMING_FUNCTION);
>      options.expectDisplay = name === "display";
> +    options.expectShape = name === "clip-path" || name === "shape-outside";

Move this after options.expectFilter

::: devtools/client/shared/output-parser.js:202
(Diff revision 2)
>                this._appendCubicBezier(functionText, options);
>              } else if (colorOK() &&
>                         colorUtils.isValidCSSColor(functionText, this.cssColor4)) {
>                this._appendColor(functionText, options);
> +            } else if (Services.prefs.getBoolPref(CSS_SHAPES_ENABLED_PREF) &&
> +                       options.expectShape &&

I would reorder the options.expectShape to be first to avoid a call to getBoolPref if it didn't expect it.

::: devtools/client/shared/output-parser.js:721
(Diff revision 2)
>     *           - filterSwatch: false    // A special case for parsing a
>     *                                    // "filter" property, causing the
>     *                                    // parser to skip the call to
>     *                                    // _wrapFilter.  Used only for
>     *                                    // previewing with the filter swatch.
>     *           - gridClass: ""          // The class to use for the grid icon.

Add shapeClass jsdoc

::: devtools/client/shared/output-parser.js:743
(Diff revision 2)
>        filterSwatch: false,
>        gridClass: "",
>        supportsColor: false,
>        urlClass: "",
>        baseURI: undefined,
> +      shapeClass: "",

Move this after gridClass

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

We should also add unit test for !important

Comment 8

[Mike Park [:mparkms]]

Comment on attachment 8879630 [details]
Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150978/diff/2-3/

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until the review requirements for MozReview are met.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 10

[Mike Park [:mparkms]]

Comment on attachment 8879630 [details]
Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150978/diff/3-4/

Comment 11

[Mike Park [:mparkms]]

Comment on attachment 8879630 [details]
Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150978/diff/4-5/

Comment 12

[Mike Park [:mparkms]]

Comment on attachment 8879630 [details]
Bug 1373339 - Add a button in the rules view to toggle the CSS shapes highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150978/diff/5-6/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d425977fef75
Add a button in the rules view to toggle the CSS shapes highlighter. r=gl

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Rares Doghi, Desktop QA]

Hi everyone, i have retested this issue on Windows 10 , Windows 7 , Mac OSx and Linux using the latest version of nightly 62.0a1 (2018-05-28) and i can Confirm it as Fixed.
I will mark it accordingly.