Closed
Bug 1139644
Opened 9 years ago
Closed 9 years ago
Flash only relevant attributes in markup view when changed
Categories
(DevTools :: Inspector, defect)
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: bgrins, Assigned: bgrins)
References
Details
Attachments
(2 files, 2 obsolete files)
9.11 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
14.78 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
Just a WIP showing how this could work.
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
Try push with both: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5374b5142127
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/fb06cbc42442 remote: https://hg.mozilla.org/integration/fx-team/rev/e36d122aeef8
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb06cbc42442 https://hg.mozilla.org/mozilla-central/rev/e36d122aeef8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•