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

RESOLVED FIXED in Firefox 39

Status

P1
normal
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 39
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1139569
(Assignee)

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
This depends on 1139644 since it's ready to land and will require rebasing on top of it
Depends on: 1139644
(Assignee)

Comment 3

4 years ago
Created attachment 8582827 [details] [diff] [review]
dupe-attribute.patch

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+
(Assignee)

Comment 7

4 years ago
Created attachment 8584051 [details] [diff] [review]
dupe-attribute.patch

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+
(Assignee)

Comment 8

4 years ago
There are unrelated bc1 test failures, but try is looking good otherwise
Keywords: checkin-needed
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/9630284e15e6
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]

Comment 10

4 years ago
https://hg.mozilla.org/mozilla-central/rev/9630284e15e6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.