Closed Bug 1333714 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: pbro, Assigned: jdescottes)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

test_case_1333714.html
882 bytes, text/html
Details
Bug 1333714 - Allow highlighter to be hidden even if current node is not valid;
59 bytes, text/x-review-board-request
zer0
: review+
Details
Bug 1333714 - update grid highlighter and layout panel on markupmutation;
59 bytes, text/x-review-board-request
gl
: review+
Details 
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?).
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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
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 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?
Attachment #8844525 - Flags: review?(zer0) → feedback?(zer0)
(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 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.
Attachment #8844525 - Flags: review+
Attachment #8844525 - Flags: feedback?(zer0) → feedback+
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.
Attachment #8844526 - Flags: review?(gl) → review+
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 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.
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).
Flags: needinfo?(zer0)
Flags: needinfo?(zer0)
Attachment #8844525 - Flags: review+ → review?(zer0)
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.
Attachment #8844525 - Flags: review?(zer0) → review+
Pushed by jdescottes@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e3a0d990896d
https://hg.mozilla.org/mozilla-central/rev/4a9ad284a596
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: