Closed Bug 1456680 Opened 6 years ago Closed 6 years ago

Toggle the grid 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

(7 files)

      No description provided.
Comment on attachment 8971108 [details] [diff] [review]
Bug 1456680 - Part 4: Rename event and display node to badge in element-editor.js;

https://reviewboard.mozilla.org/r/239906/#review245720


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:282
(Diff revision 1)
> -    // Update the display type node
> -    let showDisplayNode = this.node.displayType in DISPLAY_TYPES;
> -    this.displayNode.textContent = this.node.displayType;
> -    this.displayNode.dataset.display = this.node.displayType;
> -    this.displayNode.style.display = showDisplayNode ? "inline-block" : "none";
> -    this.displayNode.title = showDisplayNode ? DISPLAY_TYPES[this.node.displayType] : "";
> +    // Update the display markup badge
> +    let showDisplayBadge = this.node.displayType in DISPLAY_TYPES;
> +    this.displayBadge.textContent = this.node.displayType;
> +    this.displayBadge.dataset.display = this.node.displayType;
> +    this.displayBadge.style.display = showDisplayBadge ? "inline-block" : "none";
> +    this.displayBadge.title = showDisplayNode ? DISPLAY_TYPES[this.node.displayType] : "";

Error: 'showdisplaynode' is not defined. [eslint: no-undef]
Comment on attachment 8971106 [details] [diff] [review]
Bug 1456680 - Part 2: Format markup.js;

https://reviewboard.mozilla.org/r/239902/#review246324
Attachment #8971106 - Flags: review?(pbrosset) → review+
Comment on attachment 8971108 [details] [diff] [review]
Bug 1456680 - Part 4: Rename event and display node to badge in element-editor.js;

https://reviewboard.mozilla.org/r/239906/#review246326
Attachment #8971108 - Flags: review?(pbrosset) → review+
I've started reviewing and testing this, but here are some comments about the feature itself:

- It's a new UI feature and we're within the code freeze for 61. So let's land this after the merge so it has a chance of sitting in nightly 62 for a full cycle and be tested/fine-tuned if needed.
- Isn't it going to be seen as inconsistent by users if clicking on 'grid' does something, but clicking on 'flex', 'contents', 'flow-root' doesn't? If you make one clickable, that sets the expectation that all are clickable. Pinging Victoria on this particular point: is it a problem? Do we have ideas for making the others clickable in some way?
Flags: needinfo?(victoria)
Comment on attachment 8971917 [details] [diff] [review]
Bug 1456680 - Part 5: Remove unused onShowGridLineNamesHighlight method.

https://reviewboard.mozilla.org/r/240650/#review246328
Attachment #8971917 - Flags: review?(pbrosset) → review+
Comment on attachment 8971919 [details] [diff] [review]
Bug 1456680 - Part 7: Make the grid display markup badge able to toggle on and off the grid highlighter.

https://reviewboard.mozilla.org/r/240654/#review246330

::: devtools/client/inspector/markup/views/element-editor.js:649
(Diff revision 1)
>      // Start listening for mutations until we find an attributes change
>      // that modifies this attribute.
>      this.markup.inspector.once("markupmutation", onMutations);
>    },
>  
> +  onDisplayBadgeClick: function(event) {

Can you please a simple jsdoc here?
Attachment #8971919 - Flags: review?(pbrosset) → review+
Oh, here's another comment I thought about while testing: the tooltip shown on hover of the grid badge now also need to say what happens when users click it. It also still needs to say what grid display type means (as it does today).
Comment on attachment 8971105 [details]
Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted.

https://reviewboard.mozilla.org/r/239900/#review245724

::: devtools/client/inspector/shared/highlighters-overlay.js:147
(Diff revision 3)
>    }
>  
>    /**
> +   * Load the grid highligher display settings into the store from the stored preferences.
> +   */
> +  loadHighlighterSettings() {

The highlighter-overlay deals with more than just the grid highlighter, so now that this method lives here, it needs to have a more specific name than that. Otherwise we don't know if this is for grid, fonts, flex, something else.
Attachment #8971105 - Flags: review?(pbrosset) → review+
Comment on attachment 8971107 [details] [diff] [review]
Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted.

https://reviewboard.mozilla.org/r/239904/#review246336

::: devtools/client/inspector/markup/markup.js:2020
(Diff revision 4)
> +  /**
> +   * Handler for "grid-highlighter-hidden" event emitted from the HighlightersOverlay.
> +   * Updates the markup view to show an inactive grid display badge for the given
> +   * nodeFront.
> +   *
> +   * @param  {NodeFront} nodeFront
> +   *         The NodeFront of the grid container element for which the grid highlighter
> +   *         is hidden for.
> +   */
> +  _onGridHighlighterHidden(nodeFront) {
> +    const container = this.getContainer(nodeFront);
> +
> +    if (container) {
> +      container.update();
> +    }
> +
> +    this.highlightedGridContainer = null;
> +  },
> +
> +  /**
> +   * Handler for "grid-highlighter-shown" event emitted from the HighlightersOverlay.
> +   * Updates the markup view to show an active grid display badge for the given nodeFront.
> +   *
> +   * @param  {NodeFront} nodeFront
> +   *         The NodeFront of the grid container element for which the grid highlighter
> +   *         is shown for.
> +   */
> +  _onGridHighlighterShown(nodeFront) {
> +    const container = this.getContainer(nodeFront);
> +
> +    // Update the last highlighted grid element container to inactivate its badge.
> +    if (this.highlightedGridContainer) {
> +      this.highlightedGridContainer.update();
> +    }
> +
> +    if (container) {
> +      container.update();
> +    }
>    }

Let's move all of this (+ the event listener code) to the `MarkupElementContainer` class instead. No need for this higher level class to deal with this since the only thing it wants to do is locate the containers and update them anyway.

You can listen to the same events in `MarkupElementContainer` by accessing the highlighters from the parent markup-view.
Then, in the `onGridHighlighterHidden` callback, you can just check if `nodeFront === this.editor.node` and early return if not, and then just call `this.update();`.

Also, no need for `highlightedGridContainer` anymore I believe. You can probably easily remember if a container's grid badge was clicked in `onGridHighlighterShown`.

::: devtools/client/inspector/markup/views/element-editor.js:284
(Diff revision 4)
> +    if (this.highlighters.gridHighlighterShown === this.node) {
> +      this.displayNode.classList.add("active");
> +
> +      // Store the MarkupElementContainer of the highlighted grid.
> +      this.markup.highlightedGridContainer = this.container;
> +    } else {
> +      this.displayNode.classList.remove("active");
> +    }

As explained above, I believe there should be a way to get rid of `highlightedGridContainer` here.
This way, you can also simplify the if/else into:

```
this.displayNode.classList.toggle("active",
  this.highlighters.gridHighlighterShown === this.node);
```
Attachment #8971107 - Flags: review?(pbrosset)
Comment on attachment 8971918 [details] [diff] [review]
Bug 1456680 - Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay.

https://reviewboard.mozilla.org/r/240652/#review246344

Nice simplifications overall.
Attachment #8971918 - Flags: review?(pbrosset) → review+
Keywords: leave-open
(In reply to Patrick Brosset <:pbro> from comment #20)
> I've started reviewing and testing this, but here are some comments about
> the feature itself:
> 
> - It's a new UI feature and we're within the code freeze for 61. So let's
> land this after the merge so it has a chance of sitting in nightly 62 for a
> full cycle and be tested/fine-tuned if needed.
> - Isn't it going to be seen as inconsistent by users if clicking on 'grid'
> does something, but clicking on 'flex', 'contents', 'flow-root' doesn't? If
> you make one clickable, that sets the expectation that all are clickable.
> Pinging Victoria on this particular point: is it a problem? Do we have ideas
> for making the others clickable in some way?

My original thinking was that all badges should be clickable, including both grid and flex badges — I didn't know about contents and flow-root though.

I can understand the reason to postpone the clickable badge feature for 62. For 61, I'd suggest we style all non-clickable badges differently from the event badges to show that they aren't active buttons yet. I'll work on the styling for this, and post a new mockup here soon.
Flags: needinfo?(victoria)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80f9a47393ed
Part 1: Store the grid highlighter setting in HighlightersOverlay. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef2d7d27ff1
Part 2: Format markup.js; r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa4b6d2be06
Part 5: Remove unused onShowGridLineNamesHighlight method. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/232bed10f4d8
Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay. r=pbro
Attachment #8971105 - Flags: checkin+
Attachment #8971106 - Flags: checkin+
Attachment #8971917 - Flags: checkin+
Attachment #8971918 - Flags: checkin+
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea070f2a2b31
Part 1: Store the grid highlighter setting in HighlightersOverlay. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a4e70b3229
Part 2: Format markup.js; r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7bc1fadc3f
Part 5: Remove unused onShowGridLineNamesHighlight method. r=pbro
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d92d104b4b5f
Part 6: Refactor getting the grid highlighter settings logic into HighlightersOverlay. r=pbro
I updated this badge mockup with new non-clickable styles. The [...] icons should turn into clickable badges. New changes are highlighted in purple. (Includes a change to the dark clickable hover color, since I had to shift things around a bit.)

https://mozilla.invisionapp.com/share/FAGU0I48VD7#/screens

There are more problems to be solved for toggle-able grid/flex icons that can show up in selected rows - I'll do that separately assuming 62 for that feature. I'm hoping this unclickable change can be added for 61.
Comment on attachment 8971107 [details] [diff] [review]
Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted.

https://reviewboard.mozilla.org/r/239904/#review246336

> Let's move all of this (+ the event listener code) to the `MarkupElementContainer` class instead. No need for this higher level class to deal with this since the only thing it wants to do is locate the containers and update them anyway.
> 
> You can listen to the same events in `MarkupElementContainer` by accessing the highlighters from the parent markup-view.
> Then, in the `onGridHighlighterHidden` callback, you can just check if `nodeFront === this.editor.node` and early return if not, and then just call `this.update();`.
> 
> Also, no need for `highlightedGridContainer` anymore I believe. You can probably easily remember if a container's grid badge was clicked in `onGridHighlighterShown`.

I had to move the toggling of the "active" class to ths handler because we can't simply just check if nodeFront === this.editor.node and do an update. Since we won't clear the "active" class for the grid node that got unhighlighted, and I didn't want to do a full this.update() call since the only the displayNode "active" class is being toggled.
Attachment #8971106 - Attachment is obsolete: true
Attachment #8971107 - Attachment is obsolete: true
Attachment #8971108 - Attachment is obsolete: true
Attachment #8971917 - Attachment is obsolete: true
Attachment #8971918 - Attachment is obsolete: true
Attachment #8971919 - Attachment is obsolete: true
Attachment #8971105 - Flags: review?(pbrosset)
Attachment #8971105 - Flags: review+
Attachment #8971105 - Flags: checkin+
Attachment #8971108 - Attachment is obsolete: false
Attachment #8971108 - Attachment is patch: true
Attachment #8971108 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8971107 - Attachment is obsolete: false
Attachment #8971107 - Attachment is patch: true
Attachment #8971107 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8971919 - Attachment is obsolete: false
Attachment #8971919 - Attachment is patch: true
Attachment #8971919 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8971106 - Attachment is obsolete: false
Attachment #8971106 - Attachment is patch: true
Attachment #8971106 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8971917 - Attachment is obsolete: false
Attachment #8971917 - Attachment is patch: true
Attachment #8971917 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8971918 - Attachment is obsolete: false
Attachment #8971918 - Attachment is patch: true
Attachment #8971918 - Attachment mime type: text/x-review-board-request → text/plain
Blocks: 1317102
Product: Firefox → DevTools
Comment on attachment 8971105 [details]
Bug 1456680 - Part 3: Show an active state for the grid markup badge if its grid container is highlighted.

https://reviewboard.mozilla.org/r/239900/#review258464

Found one bug: 
- open about:newtab
- in the grid sidebar panel, click to enable the ul.section-list grid
- in the same sidebar panel, click on the inspector icon next to ul.section-list
- this will auto-expand the DOM tree and select the element in there
The bug here is that the badge is *not* highlighted in this case.

::: devtools/client/inspector/markup/views/element-container.js:72
(Diff revision 4)
> +  /**
> +   * Handler for "grid-highlighter-hidden" and "grid-highlighter-shown" event emitted from
> +   * the HighlightersOverlay. Toggles the active state of the display badge if it matches
> +   * the highlighted grid node.
> +   */
> +  onGridHighlighterChange: function() {

About the bug I noticed, you probably just need to call this the first time the MarkupElementContainer instance is created.
Attachment #8971105 - Flags: review?(pbrosset) → review+
Revisiting the "toggled-on badge on selected row" blue-on-blue problem:

I saw a screenshot from gl of what is currently there, and it looked pretty good - 1px white border and looks like the text might be bolded?

I worked on a few other variants and I think this version with black-on-light blue (#B7DAFF) background - with Blue 40 border, no bold needed - might be better in that it'll be a bit more visible against the blue row:

https://mozilla.invisionapp.com/share/FAGU0I48VD7#/screens

It's less strong of a toggled look for unselected rows, but seems like the row will almost always be selected when toggling this badge, since clicking to toggle will select the row (I assume?).
Flags: needinfo?(gl)
(In reply to Victoria Wang [:victoria] from comment #37)
> Revisiting the "toggled-on badge on selected row" blue-on-blue problem:
> 
> I saw a screenshot from gl of what is currently there, and it looked pretty
> good - 1px white border and looks like the text might be bolded?
> 
> I worked on a few other variants and I think this version with
> black-on-light blue (#B7DAFF) background - with Blue 40 border, no bold
> needed - might be better in that it'll be a bit more visible against the
> blue row:
> 
> https://mozilla.invisionapp.com/share/FAGU0I48VD7#/screens
> 
> It's less strong of a toggled look for unselected rows, but seems like the
> row will almost always be selected when toggling this badge, since clicking
> to toggle will select the row (I assume?).

Let's move this work onto a new bug.
Flags: needinfo?(gl)
Keywords: leave-open
Blocks: 1471460
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/215382ac0854
Part 3: Show an active state for the grid markup badge if its grid container is highlighted. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc3dfb5191c
Part 4: Rename event and display node to badge in element-editor.js; r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/332ec3041d11
Part 7: Make the grid display markup badge able to toggle on and off the grid highlighter. r=pbro
https://hg.mozilla.org/mozilla-central/rev/215382ac0854
https://hg.mozilla.org/mozilla-central/rev/ccc3dfb5191c
https://hg.mozilla.org/mozilla-central/rev/332ec3041d11
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Blocks: 1471764
Blocks: 1471779
See Also: → 1463710
You need to log in before you can comment on or make changes to this bug.