Closed Bug 1139644 Opened 5 years ago Closed 5 years ago

Flash only relevant attributes in markup view when changed

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files, 2 obsolete files)

Instead of highlighting the whole line, we should only highlight attributes that changed.  A couple of test cases: 

* The svg animation here: http://maxwellito.github.io/vivus/#polaroid
* The changing attribute element here: https://bgrins.github.io/devtools-demos/inspector/mutations.html
Attached patch flash-attribute-WIP.patch (obsolete) — Splinter Review
Just a WIP showing how this could work.
Attached patch flash-attribute.patch (obsolete) — Splinter Review
Refactored, with tests.  The behavior is:

* Removed attribute: highlight the entire tag (as before)
* Edited attribute: highlight the existing attribute's value
* New attribute: highlight new attribute's value (we possibly could adjust this in the future to highlight both name / value)
Assignee: nobody → bgrinstead
Attachment #8574774 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8582079 - Flags: review?(pbrosset)
Comment on attachment 8582079 [details] [diff] [review]
flash-attribute.patch

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

A few nits only. Overall looks good. And, very happy to see this feature being implemented.

::: browser/devtools/markupview/markup-view.js
@@ +320,5 @@
>    _brieflyShowBoxModel: function(nodeFront) {
>      let win = this._frame.contentWindow;
>  
>      if (this._briefBoxModelTimer) {
> +      clearTimeout(this._briefBoxModelTimer);

Not a big deal, but it would help keep change history nice if you split out all setTimeout/clearTimeout changes in a separate patch.

@@ +788,5 @@
> +        if (type === "characterData") {
> +          addedOrEditedContainers.add(container);
> +        } else if (type === "attributes" && newValue === null) {
> +          // Removed attributes should flash this node.  New or changed
> +          // attributes will flash the attribute itself.

This means that modified attributes aren't handled in this function at all, right?
The call to this.flashAttribute(attr.name) in ElementEditor.update takes care of this.
That's fine, but it makes it harder to track where the feature is implemented in this file. Before, everything was handled here only.
So I think it's at least worth a better comment that explains where updated attributes are being flashed in the code.

@@ +1900,5 @@
>    },
>  
>    set flashed(aValue) {
>      if (aValue) {
> +      flashElementOn(this.tagState, this.editor.elt);

Now that the flashed setter is so simple, and because it's only used in flashMutation, I think you should remove it, and in flashMutation, just call flashElementOn/Off instead.
This would also make it more similar to the code in ElementEditor that flashes attributes.

@@ +2771,5 @@
>    // Attributes return from DOMParser in reverse order from how they are entered.
>    return attributes.reverse();
>  }
>  
> +function flashElementOn(backgroundElt, foregroundElt) {

Could you add a jsdoc comment for this function?

@@ +2789,5 @@
> +    span => span.classList.add("theme-fg-contrast")
> +  );
> +}
> +
> +function flashElementOff(backgroundElt, foregroundElt) {

And for this one too?

::: browser/devtools/markupview/test/browser_markupview_mutation_02.js
@@ +9,5 @@
>  const TEST_URL = TEST_URL_ROOT + "doc_markup_flashing.html";
>  
>  // The test data contains a list of mutations to test.
>  // Each item is an object:
>  // - desc: a description of the test step, for better logging

Can you also update this comment with something like:
- attribute: if set, the test will expect the corresponding attribute to flash instead of the whole node

@@ +34,5 @@
>      rootNode.appendChild(rootNode.firstElementChild);
>    },
>    flashedNode: ".list .item:last-child"
>  }, {
>    desc: "Adding an attribute should flash the node",

s/should flash the node/should flash the attribute

@@ +42,3 @@
>    }
>  }, {
>    desc: "Editing an attribute should flash the node",

s/the node/the attribute

@@ +46,5 @@
>    mutate: (doc, rootNode) => {
>      rootNode.setAttribute("class", "list value-" + Date.now());
>    }
>  }, {
> +  desc: "Multiple changes to an attribute should flash the node",

s/the node/the attribute

@@ +119,5 @@
> +
> +function* assertAttributeFlashing(nodeFront, attribute, inspector) {
> +  let container = getContainerForNodeFront(nodeFront, inspector);
> +  ok(container, "Markup container for node found");
> +  ok(container.editor, "Markup editor for node found");

Not sure if we need an assertion for this property in this test.

@@ +123,5 @@
> +  ok(container.editor, "Markup editor for node found");
> +  ok(container.editor.attrs[attribute], "Attribute exists on editor");
> +
> +  let attributeElement = container.editor.getAttributeElement(attribute);
> +  ok(attributeElement, "Attribute element exists on editor");

Also not sure if this assertion is really needed here. We really only care if the correct node is flashing.
Attachment #8582079 - Flags: review?(pbrosset) → review+
Split the timeout change into another patch.  Also converted the calls to setInterval and clearInterval while I was at it.  Try push with just this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29352e50558
Attachment #8582513 - Flags: review+
Addresses notes from comment 3.

> > +        if (type === "characterData") {
> > +          addedOrEditedContainers.add(container);
> > +        } else if (type === "attributes" && newValue === null) {
> > +          // Removed attributes should flash this node.  New or changed
> > +          // attributes will flash the attribute itself.
> 
> This means that modified attributes aren't handled in this function at all,
> right?
> The call to this.flashAttribute(attr.name) in ElementEditor.update takes
> care of this.
> That's fine, but it makes it harder to track where the feature is
> implemented in this file. Before, everything was handled here only.
> So I think it's at least worth a better comment that explains where updated
> attributes are being flashed in the code.

Yes, it's all handed in the ElementEditor.  Unfortunately, moving it here would make the change more complicated, since it doesn't know if the attribute has actually changed or not.  The frontend should be able to be refactored a bit to allow this but  for now I've added a comment to explain.
Attachment #8582079 - Attachment is obsolete: true
Attachment #8582635 - Flags: review+
Keywords: checkin-needed
No longer depends on: 1139569
Blocks: 1147128
https://hg.mozilla.org/mozilla-central/rev/fb06cbc42442
https://hg.mozilla.org/mozilla-central/rev/e36d122aeef8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Depends on: 1147325
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.