911982 - Highlight mutated elements in the inspector for some time

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

The idea is to attract attention by temporarily flashing DOM nodes that have been mutated (addition/removal/change of an attribute, or removeChild/appendChild/...).

When bug 855523 is fixed, it will become easy to highlight the whole row.

I have started a patch for this and will try to attach it in the coming days.

Comment 1

[Patrick Brosset <:pbro>]

Flashing node containers in the inspector when mutations occur is the easy part. The markup-view already listens for mutations.

The hard part is to not flash when the mutation comes from the inspector itself. This would happen when the user makes attributes or tag changes in the inspector.

One way to proceed is to temporarily stop the flashing when a node is change in the inspector and resume it after the mutation for that change is detected and applied.
The complex part here is that mutation events come as unsolicited packets and there's therefore no way to differentiate them from actual DOM mutations caused by the content page's javascript code.

Comment 2

[Patrick Brosset <:pbro>]

Pushed a new patch to TRY: https://tbpl.mozilla.org/?tree=Try&rev=6203d41941e8

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug911982-mutation-flashing.patch

And here is the patch, for review.

Joe, not sure exactly who would be best to review this, so feel free to re-assign if needed.

As for the patch itself, here is a description of the changes made:

- After the mutation observer callback has been executed (in MarkupView), we execute a new function that'll take care of flashing nodes that have changed.
- The flashing logic is:
  - if a node was added, it flashes
  - if a node was removed, its parent flashes
  - if a node was re-appended, it flashes
  - if a node get any of its attributes added, changed or removed, it flashes
- The flashing is handled at MarkupContainer level, by a specific function that simply adds a CSS class to the line element and removes it after a while (using a CSS transition to make it go away smoothly)
- The CSS class used is "theme-bg-contrast", I created it because there didn't seem to be any theme class that corresponded to the use case.
- Flashing only occurs when the container is not the one currently selected in the markup view.
  - This is the only way I found so far that avoids containers from flashing when changes are made using the devtools (i.e. changing an attribute using devtools should probably not flash the node, since you already know you're making that change).

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 806673 [details] [diff] [review]
bug911982-mutation-flashing.patch

Review of attachment 806673 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me overall.  Just a few minor notes, and one about possibly using a settings parameter for _updateChildren.

::: browser/devtools/markupview/markup-view.css
@@ -32,5 @@
>    line-height: 1.4em;
>    position: relative;
>  }
>  
> -/* Children are indented thanks to their parent's left padding, that means they

Did you mean to remove this comment?

::: browser/devtools/markupview/markup-view.js
@@ +420,5 @@
>        if (type === "attributes" || type === "characterData") {
>          container.update(false);
>        } else if (type === "childList") {
>          container.childrenDirty = true;
> +        // Update the children to take care of changes in the DOM

I wonder if we should pass in an options object instead of the final two parameters to _updateChildren.  It is probably still at the number where it isn't completely necessary, but the fact that we need a comment to explain it could be a sign that it would be better as:

this._updateChildren(container, {
  expand: false
  highlight: true
});

I notice places elsewhere in the file using the aExpand parameter could probably be a little more clear this way as well.

@@ +421,5 @@
>          container.update(false);
>        } else if (type === "childList") {
>          container.childrenDirty = true;
> +        // Update the children to take care of changes in the DOM
> +        // Passing true as the last parameter asks for mutation flashing of the 

trailing whitespace

@@ +433,5 @@
>      });
>    },
>  
>    /**
> +   * Given a list of mutations returned by the mutation observer, flash the 

trailing whitespace

::: browser/devtools/markupview/test/browser_inspector_markup_mutation_flashing.js
@@ +31,5 @@
> +  }
> +
> +  function startTests() {
> +    markup = inspector.markup;
> +    

trailing whitespace

::: browser/themes/shared/devtools/dark-theme.css
@@ +51,5 @@
>    background-color: rgba(0,0,0,0.5);
>  }
>  
> +.theme-bg-contrast {
> +  background: #a18650;

Minor detail: It's hard to read the text while it is flashing with this background color.  A suggestion for both themes: a yellow highlight color like #ffffbe?

Comment 5

[Patrick Brosset <:pbro>]

TRY build is green.
Thanks Brian for the valuable feedback. I'll provide another patch soon.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 806673 [details] [diff] [review]
bug911982-mutation-flashing.patch

Review of attachment 806673 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/markupview/markup-view.js
@@ +1065,5 @@
> +      if (this._flashMutationTimer) {
> +        contentWin.clearTimeout(this._flashMutationTimer);
> +        this._flashMutationTimer = null;
> +      }
> +      this._flashMutationTimer = this.markup._frame.contentWindow.setTimeout(() => {

s/this.markup._frame.contentWindow/contentWin ?

::: browser/themes/shared/devtools/dark-theme.css
@@ +51,5 @@
>    background-color: rgba(0,0,0,0.5);
>  }
>  
> +.theme-bg-contrast {
> +  background: #a18650;

I think we should ask Shorlander what he thinks. There could be all sorts of colour theory that goes into colour selection. I think it's best to CC him on the bug and send a summary email explaining the problem.

Comment 7

[Patrick Brosset <:pbro>]

CCing shorlander.

@Shorlander: This bug makes it possible to highlight/flash the nodes in the markup-view for about a second when they change in the DOM. This is something that Firebug has been doing for a long time and that you might have seen. This is also what I demoed quickly during the work week in Paris.

The question here is that I need some theme support for the flashing background color.
And in fact, it also makes sense to change the color of the text while the background is flashing to read easier.
What I've done so far is reuse the "yellow" color from the dark and light themes:
> .theme-fg-color5 { /* Yellow */
>  color: #a18650;
> }
And created a new theme class with it:
> .theme-bg-contrast {
>   background: #a18650;
> }
Since I also want the text color to change, I should probably go ahead and also create the following:
> .theme-fg-contrast {
>   color: black;
> }

Do you think this makes sense in light of the way themes have been defined so far?
Thanks for your help.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug911982-mutation-flashing.patch

Thanks Joe for the review.

Here is a new patch with the following changes as compared to the first one:
- fixed whitespaces
- fixed contentWin
- changed the _updateChildren method signature to use an option object instead
- added a css font color class so the text is easier to read when nodes are flashing.

What's missing after this:
- confirmation from Stephen about the new CSS classes
- also I don't think the way it does not flash if you do changes in the devtools is the best, but I think it would be good to go ahead with this patch anyway and iterate from there.

Comment 9

[Patrick Brosset <:pbro>]

I TRYed the last patch, all green: https://tbpl.mozilla.org/?tree=Try&rev=3e330b2c0923
Ready for check-in.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

https://hg.mozilla.org/integration/fx-team/rev/1770589caa8b

Comment 11

[Girish Sharma [:Optimizer]]

This is not yet resolved fixed.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1770589caa8b