1333714 - Grid highlighter doesn't hide when the DOM it is highlighting is removed/reloaded in jsfiddle

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Patrick Brosset <:pbro>]

STR are a bit complicated, maybe there is a way to make this easier somehow.

- Open jsfiddle: https://jsfiddle.net/
- In the HTML pane, enter:
<div id="grid">
  <div id="i1">a</div>
  <div id="i2">b</div>
</div>
- in the CSS pane, enter:
#grid {display: grid;}
#i1 {grid-row: 1;grid-column: 1;}
#i2 {grid-row: 2;grid-column: 2;}
- press the "run" button at the top
- now, with the inspector opened, select the #grid div from the output pane
- in the rule-view, click on the grid icon to highlight the grid
- now, in the HTML pane, add one more item:
  <div id="i3">c</div>
- and in the CSS pane, one rule that goes with it:
#i3 {grid-row: 3;grid-column: 2;}
- press the "run" button again --> This will remove the output iframe entirely and construct a new document.

==> The grid highlighter stays visible on the page, even if the grid element it was highlighting was removed and a new one was added.

==> If you select the #grid element again and click on the grid icon in the rule-view again, then everything goes back to normal.

I think the grid should be hidden when the document gets removed.
The other solution could be to update the grid when the new node gets created, but it means we'd have to listen for mutations, and find the right node, which would be complex (also, what if you changed the id of the grid container in the HTML pane in the meantime?).

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: test_case_1333714.html

Comment 2

[Julian Descottes [:jdescottes]]

Alternate STRs:
- open https://bug1333714.bmoattachments.org/attachment.cgi?id=8839976
- open devtools layout panel
- display the grid for div#grid
- click on the "delete grid container" button

ER: 
- the grid highlighter should be deleted
- the grid listing should be updated

Comment 3

[Bogdan Maris, Desktop Test Engineering]

Just a note here, using str from comment 0 and latest Nightly 54.0a1, I can see that the grid highlighter is actually dismissed when I hit run after adding the third row. This does not happen in Fx 52 beta 8.

Using STR from comment 2 I see the grid highlighter is not dismissed in both Fx 52 beta 8 and Latest Nightly 54.0a1.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

When a highlighter was created for a node that has been removed, it can no longer be
hidden due to a check performed in the hide method of the basic auto-refresh class.

While it makes sense not to display a highlighter for an invalid node, hiding a
highlighter should always remain possible.

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

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1333714 - update grid highlighter and layout panel on markupmutation;

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

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

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

Clearing the review flag in favor of a needinfo for now.

The root cause of my problem here is that I need to hide a highlighter for a node that has been removed from the document. Being out of the document makes the node fail the isNodeValid check that currently bails out of the hide method defined in the base class auto-refresh.js. 

I think we should be allowed to hide highlighters that were set on invalid nodes (I would even say, we should always hide highlighters set on invalid nodes). Simply removing this check makes the geometry-editor highlighter fail because the _hide function relies on this.currentNode.ownerDocument.documentElement when calling setIgnoreLayoutChanges(false, ...) (the document element being used to trigger a reflow).

All the other highlighters are using the document coming from the highlighter environment. Is there a reason that forces us to use the document linked to the current node here?

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Julian Descottes [:jdescottes] from comment #8)

> I think we should be allowed to hide highlighters that were set on invalid
> nodes (I would even say, we should always hide highlighters set on invalid
> nodes).

It sounds reasonable.

> Simply removing this check makes the geometry-editor highlighter
> fail because the _hide function relies on
> this.currentNode.ownerDocument.documentElement when calling
> setIgnoreLayoutChanges(false, ...) (the document element being used to
> trigger a reflow).
> 
> All the other highlighters are using the document coming from the
> highlighter environment. Is there a reason that forces us to use the
> document linked to the current node here?

I honestly don't recall why is using `currentNode`. The only differences I can think about, is that the document coming from the highlighter environment is the document of the boundary window – the top window, basically, where the highlighter is living – where the ownerDocument from the `currentNode` can be any window (e.g. frames).

So, in the first case it will force a sync reflow in the boundary window, where in the other case it will force a sync reflow in node's window (that can be the top one or not).
In both cases all the layout changes are ignored since that point.

What I can't recall, if there was any specific reason for the geometry-editor to specifically force the sync reflow in the node's window instead the top's one.

Since I didn't add any specific comment about it, and I can't recall anything specific, I believe the boundary window will works as well in geometry-editor's scenario. Just check if we have any issues or regression when we're zooming.

I'm saying that since I could just pass `node` if the node's window was really important, so I honestly believe that that part is a leftover from some tests.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

https://reviewboard.mozilla.org/r/117924/#review119756

::: devtools/server/actors/highlighters/geometry-editor.js:515
(Diff revision 2)
>  
>      // Avoid zooming the arrows when content is zoomed.
>      let node = this.currentNode;
>      this.markup.scaleRootElement(node, this.ID_CLASS_PREFIX + "root");
>  
> -    setIgnoreLayoutChanges(false, node.ownerDocument.documentElement);
> +    setIgnoreLayoutChanges(false, this.highlighterEnv.window.document.documentElement);

Nit: you can use directly `this.highlighterEnv.document`.

::: devtools/server/actors/highlighters/geometry-editor.js:595
(Diff revision 2)
>      this.getElement("offset-parent").setAttribute("hidden", "true");
>      this.hideArrows();
>  
>      this.definedProperties.clear();
>  
> -    setIgnoreLayoutChanges(false,
> +    setIgnoreLayoutChanges(false, this.highlighterEnv.window.document.documentElement);

Ditto.

Comment 11

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

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

https://reviewboard.mozilla.org/r/117926/#review119948

::: devtools/client/inspector/shared/highlighters-overlay.js:322
(Diff revision 2)
>      this._lastHovered = null;
>      this._hideHoveredHighlighter();
>    },
>  
> +  onMarkupMutation: function (evt, mutations) {
> +    let hasInterestingMutation = mutations.some(({ type }) => type === "childList");

s/({ type })/{ type }

Avoid () for a single argument.

::: devtools/client/inspector/shared/highlighters-overlay.js:331
(Diff revision 2)
> +      return;
> +    }
> +
> +    let nodeFront = this.gridHighlighterShown;
> +
> +    this.inspector.walker.isInDOMTree(nodeFront).then(isInTree => {

Can you convert this to use Task.async? We did some conversion to avoid using Promise.then() in this file to make everything look cleaner.

Comment 12

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

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

https://reviewboard.mozilla.org/r/117926/#review119952

::: devtools/client/inspector/shared/highlighters-overlay.js:321
(Diff revision 2)
>      // Otherwise, hide the highlighter.
>      this._lastHovered = null;
>      this._hideHoveredHighlighter();
>    },
>  
> +  onMarkupMutation: function (evt, mutations) {

Add a JSDoc that basically describes:

Handler function for a "markupmutation" event. Hides the grid highlighter if the grid container is no longer in the DOM tree.

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

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

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

https://reviewboard.mozilla.org/r/117926/#review119948

Thanks for the review. Still have some try failures related to part1. Will land after that

> s/({ type })/{ type }
> 
> Avoid () for a single argument.

This is not a valid syntax when using destructuring. I switched to some(mut => mut.type === "childList") instead. no strong preference.

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

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

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

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

Comment 17

[Julian Descottes [:jdescottes]]

Matteo: can you take another look at the first patch ? 

I had a few try failures which led me to fix a test, and guard the hide() a bit more (based on this.highlighterEnv.window instead of the currentNode).

Comment 18

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=28924759fcb5b16f4a1a5f16ce54b9baef829d34

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

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

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

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

Comment 21

[Julian Descottes [:jdescottes]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f17a2cb9e7ca277781daa00ab0619b6d2060862

Comment 22

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

https://reviewboard.mozilla.org/r/117924/#review121158

Looks good to me!

::: devtools/client/commandline/test/browser_cmd_highlight_01.js:88
(Diff revision 4)
>          output: "1 node highlighted"
>        }
>      }
>    ]);
>  
> +  yield helpers.audit(options, [{

Could you please add a comment why this was needed, for future editor.

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8844525 [details]
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;

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

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8844526 [details]
Bug 1333714 - update grid highlighter and layout panel on markupmutation;

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3a0d990896d
Allow highlighter to be hidden even if current node is not valid;r=zer0
https://hg.mozilla.org/integration/autoland/rev/4a9ad284a596
update grid highlighter and layout panel on markupmutation;r=gl

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3a0d990896d
https://hg.mozilla.org/mozilla-central/rev/4a9ad284a596