345982 - Object - DOM Node does not update when changing values of a node's attributes

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Shawn Wilsher :sdwilsh]

In the Object - DOM Node view, you can select an attribute and edit it.  However, upon saving the change the view is not updated and the old value is still present.

This also happens when inserting a new attribute.  The new attribute is not shown unless you deselect the node, and then select it again.

Comment 1

[Sylvain Pasche]

Attachment: Testcase (needs chrome privs)

Comment 2

[Sylvain Pasche]

Attachment: Patch, v1

For the first tree in the testcase (the one broken in the bugreport, which is used in the domNode view of DOM Inspector), the call "NodeToRow(content, &contentRow)" is always failing as the content node is not in any rows.

That's why I have put the following code in the else block.

Comment 3

[[e-mail]]

This is partly a regression from the new edit dialog - since it actually removes and reinserts the attribute instead of replacing it, it actually does more damage such as forgetting the selection, changing the order of the map, etc.

Comment 4

[[e-mail]]

Comment on attachment 236633 [details] [diff] [review]
Patch, v1

I think the mRootNode == content test should be done much earlier. Also I suspect that having a -w version of your next patch might ease review.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> This is partly a regression from the new edit dialog - since it actually
> removes and reinserts the attribute instead of replacing it, it actually does
> more damage such as forgetting the selection, changing the order of the map,
> etc.

I think you are reffering to Bug 205872, but this isn't correct.  It only removes the attribute if there is a namespace change, otherwise it should just set it.

Comment 6

[Sylvain Pasche]

Attachment: Testcase, v2 (needs chrome privs)

DOMNodeViewer.reset() function was missing

Comment 7

[Sylvain Pasche]

Attachment: patch, v2

(In reply to comment #4)
> (From update of attachment 236633 [details] [diff] [review] [edit])
> I think the mRootNode == content test should be done much earlier. Also I
> suspect that having a -w version of your next patch might ease review.

That what I wanted to do first, but the condition (mRootNode == content) is also true if the root element is visible, like the second tree of the testcase. So I added a condition if we are not showing the elements.

I did not supply a -w for this second patch, as I think it is clearer now what part has been moved. I also had to remove the !contentNode in the second condition, otherwise the second tree of the testcase is not update because the index of the root == 0.

Comment 8

[[e-mail]]

Comment on attachment 236659 [details] [diff] [review]
patch, v2

Yes, I like this, thanks.

Comment 9

[Sylvain Pasche]

timeless: Thanks for the check-in. BTW, the patch was by me which is not what the check-in comment says, but I'm won't be offended for this ;-)

Would this make sense on the branch?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> timeless: Thanks for the check-in. BTW, the patch was by me which is not what
> the check-in comment says, but I'm won't be offended for this ;-)

Sorry - I guess that's what happens when I bug him to get it checked in :/
I'll be sure to specify next time.

> Would this make sense on the branch?

Well, it only doesn't work on 1.5 (I know you mean 2.0, but I can't test it.  I'm going to presume that the behavior is probably the same) when you insert a new attribute.  Changing them works just fine, and in fact it may have been a result of an error on my part in a previous bug that made it not work right for changing anyway.

What I'm trying to say is that it would be nice on branch, but it's been like this for a while so you don't have to go out of your way for it :)

Comment 11

[Shawn Wilsher :sdwilsh]

Is this fixed (as in, can we resolve this)?

Comment 12

[Sylvain Pasche]

(In reply to comment #11)
> Is this fixed (as in, can we resolve this)?

The testcase runs as expected on minefield -> resolving