Closed Bug 1147128 Opened 5 years ago Closed 5 years ago

Attribute not showing up in markup view after removing and setting to the previous value

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

Open any page and inspector panel
Open split console

Run these commands one by one:

document.body.setAttribute("foo", "bar");   (Attribute added, element flashes)
document.body.removeAttribute("foo");       (Attribute removed, element flashes)
document.body.setAttribute("foo", "bar");   (Attribute NOT added, element flashes)

The `foo` attribute should definitely show up.  This is most likely a regression of Bug 1139569.  This is not an issue on 38.
Blocks: 1139569
This is only an issue when the value is set to what it used to be (which tricks the valueChanged check).
Summary: Attribute not showing up in markup view after removing and setting → Attribute not showing up in markup view after removing and setting to the previous value
This depends on 1139644 since it's ready to land and will require rebasing on top of it
Depends on: 1139644
Attached patch dupe-attribute.patch (obsolete) — Splinter Review
The problem is that we weren't keeping the data model in sync with the DOM.  So this.attrs["foo"] would still exist and point to a DOM node that actually had the same value, but just wasn't attached to the document.

I've added a removeAttribute method that cleans up both the data model and the DOM, and added a loop that keeps this.attrs in sync with node.attributes.  I switched this.attrs to use a Map so we can iterate easily and quickly compare with the current values of this.node.attributes.  Changing to map makes the iterating code nice but also means I had to make some trivial changes to handful of tests that were directly hitting editor.attrs[foo].

There is also a regression test added for this.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0a1447f8cb3
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8582827 - Flags: review?(mratcliffe)
Comment on attachment 8582827 [details] [diff] [review]
dupe-attribute.patch

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

Drive by review. I'll let Mike finish his review, but I just have a small comment about variable names.

::: browser/devtools/markupview/markup-view.js
@@ +2407,5 @@
>    update: function() {
>      let attrs = this.node.attributes || [];
> +
> +    // Keep the data model in sync with attributes on the node.
> +    let currentAttributes = new Set(attrs.map(a=>a.name));

Reading through this code is a little hard with attrs, this.attrs and currentAttributes all being so confusingly similar.
Maybe rename attrs actualDomNodeAttributes :) Or something much better than this.
Comment on attachment 8582827 [details] [diff] [review]
dupe-attribute.patch

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

Overall this looks good but just one nit:

::: browser/devtools/markupview/markup-view.js
@@ +2404,5 @@
>    /**
>     * Update the state of the editor from the node.
>     */
>    update: function() {
>      let attrs = this.node.attributes || [];

We have this.attrs and a local attrs. In the constructor we also have this.attrList which make the code a little confusing. It would be great if you could rename these to something that is more specific about their use.
Attachment #8582827 - Flags: review?(mratcliffe) → review+
Renamed `this.attrs` to `this.attrElements` and `let attrs` to `let nodeAttributes` as discussed.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60dca5ca514e
Attachment #8582827 - Attachment is obsolete: true
Attachment #8584051 - Flags: review+
There are unrelated bc1 test failures, but try is looking good otherwise
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9630284e15e6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.