Closed Bug 1456681 Opened 6 years ago Closed 6 years ago

Toggle the flexbox highlighter from the markup badges

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Product: Firefox → DevTools
Blocks: 1470379
Comment on attachment 8989024 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.

https://reviewboard.mozilla.org/r/254112/#review260834


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/markup/views/element-editor.js:662
(Diff revision 1)
>      }
>  
> -    this.highlighters.toggleGridHighlighter(this.inspector.selection.nodeFront, "markup");
> +    if (target.dataset.display === "grid" || target.dataset.display === "inline-grid") {
> +      this.highlighters.toggleGridHighlighter(this.inspector.selection.nodeFront,
> +        "markup");
> +      return;

Error: Unnecessary return statement. [eslint: no-useless-return]
Attachment #8989024 - Attachment is obsolete: true
Attachment #8989024 - Flags: review?(pbrosset)
Comment on attachment 8989033 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.

https://reviewboard.mozilla.org/r/254118/#review260912

Looks good in general, but this should be hidden behind the flexbox highlighter pref. If you have it off, then the badge should not be clickable, for consistency.
Attachment #8989033 - Flags: review?(pbrosset)
Comment on attachment 8989033 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.

https://reviewboard.mozilla.org/r/254118/#review261226

::: devtools/client/themes/markup.css:421
(Diff revision 2)
> +.markup-badge[data-display="flex"],
>  .markup-badge[data-display="grid"],
> +.markup-badge[data-display="inline-flex"],
> +.markup-badge[data-display="inline-grid"],
>  .markup-badge[data-event] {
>    cursor: pointer;
>  }
>  
> +.markup-badge[data-display="flex"]:focus,
> +.markup-badge[data-display="flex"]:hover,
>  .markup-badge[data-display="grid"]:focus,
>  .markup-badge[data-display="grid"]:hover,
> +.markup-badge[data-display="inline-flex"]:focus,
> +.markup-badge[data-display="inline-flex"]:hover,
> +.markup-badge[data-display="inline-grid"]:focus,
> +.markup-badge[data-display="inline-grid"]:hover,
>  .markup-badge[data-event]:focus,
>  .markup-badge[data-event]:hover {
>    background-color: var(--markup-badge-hover-background-color);
>  }

I'm just a bit concerned that this will make the badge appear clickable even when clicking it doesn't do anything (most users have and will continue to have the pref off).
The simplest solution I see here to land this patch quickly would be to add a class to the badge. Either something that expresses that it can't be clicked (e.g. `inactive`) so you can then add `:not(.inactive)` to those selectors.
Or the opposite, add a class to all badges that *are* clickable (e.g. `interactive`), so you can then add this class to all badges that can be clicked (like the event bubble too).
The advantage of adding a class is you can do that from JS, based on the value of the pref.
Attachment #8989033 - Flags: review?(pbrosset)
Comment on attachment 8989033 [details]
Bug 1456681 - Toggle the flexbox highlighter from the markup display badges.

https://reviewboard.mozilla.org/r/254118/#review261662
Attachment #8989033 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13209d305efe
Toggle the flexbox highlighter from the markup display badges. r=pbro
https://hg.mozilla.org/mozilla-central/rev/13209d305efe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: