984442 - Can't remove XUL attribute in browser toolbox's inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[:Gijs (he/him)]

STR:

1. Open Firefox
2. Open Browser Toolbox
3. Switch to inspector
4. Select an attribute on, say, the #navigator-toolbox

Then any of:
5a. Right click, click "Delete" in the context menu
5b. Hit "delete" on the keyboard
5c. Hit "backspace" on the keyboard

Expected:
delete the attribute

Actual:
deletes the entire node

(be careful what you delete - you may be able to restore the right bits by using the 'undo' shortcut)

Furthermore, if you doubleclick to edit an attribute and then delete everything, including the attribute name, and hit enter, the attribute value becomes empty but the attribute remains set, instead of being removed altogether.

Comment 1

[Brian Grinstead [:bgrins]]

> Furthermore, if you doubleclick to edit an attribute and then delete everything, including the attribute name, and hit enter, the attribute value becomes empty but the attribute remains set, instead of being removed altogether.

Same thing happens on about:addons with any XUL elements. Interestingly, in this case modifying the attribute of the <xhtml:link> elements does work though.

Comment 2

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> > Furthermore, if you doubleclick to edit an attribute and then delete everything, including the attribute name, and hit enter, the attribute value becomes empty but the attribute remains set, instead of being removed altogether.
> 
> Same thing happens on about:addons with any XUL elements. Interestingly, in
> this case modifying the attribute of the <xhtml:link> elements does work
> though.

I wonder if the xul-namespaced DOM attribute nodes don't get recognized as attributes the way HTML ones are, by whatever code is meant to handle this. I ran into something similar recently when I was mucking about in the browser console, used createElementNS with the wrong namespace (trailing slash...) and only found out after a long time why various bits and pieces (like the style attribute) weren't doing what I thought they should be doing.

Comment 3

[Brian Grinstead [:bgrins]]

I can have a closer look at this, but the attribute parsing is done by the DOM parser: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#2031.  It may be as simple as using a doc.createElementNS("div", HTML_NS) on line 2038 instead of just createElement

Comment 4

[Brian Grinstead [:bgrins]]

I don't think this is actually specific to XUL documents.  When I single click on an attribute in the markup view so that it's outlined and press delete the entire node is deleted.  Patrick, do you know if it's always been this way or if this is a regression?

> Furthermore, if you doubleclick to edit an attribute and then delete
> everything, including the attribute name, and hit enter, the attribute value
> becomes empty but the attribute remains set, instead of being removed
> altogether.

This problem is XUL-specific.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> I don't think this is actually specific to XUL documents.  When I single
> click on an attribute in the markup view so that it's outlined and press
> delete the entire node is deleted.  Patrick, do you know if it's always been
> this way or if this is a regression?
It has already been like this (at least, since I started working on the inspector more than a year ago).
Also, the item in the ctx menu says "delete node", so this one is ok I think. We could do with adding new items to this menu: "delete attribute", "add attribute", "edit attribute". In fact that's bug 994555.

Pressing delete has also always deleted the whole node as far as I remember, even if an attribute is selected.
In fact, we don't really have the concept of a "selected attribute", it's just that attributes are keyboard-navigable and their outline is visible when focused, but the markup-view doesn't know which attribute is selected as of now.
If we want the delete key to only remove the selected attribute (if any), then we need to file a new bug for this.

> > Furthermore, if you doubleclick to edit an attribute and then delete
> > everything, including the attribute name, and hit enter, the attribute value
> > becomes empty but the attribute remains set, instead of being removed
> > altogether.
> 
> This problem is XUL-specific.

Yeah, that's really a problem, and is, as you said, XUL specific. I was able to reproduce it on about:newtab.
The same thing happens if you select a XUL node in the inspector, then run |$0.removeAttribute("id")| in the console. The markup-view will react to the mutation and will show <node id=""> instead of removing the whole attribute.
But if you run |$0.attributes|, the id attribute isn't part of the map anymore.

I haven't looked at this for very long, but removing the attribute causes the following mutation info to be sent on the protocol:

  "mutations": [
    {
      "type": "attributes",
      "target": "conn1.domnode47",
      "numChildren": 6,
      "attributeName": "id",
      "newValue": ""
    }
  ],

Doing the same on an HTML document:

  "mutations": [
    {
      "type": "attributes",
      "target": "conn0.child1/domnode30",
      "numChildren": 7,
      "attributeName": "id",
      "newValue": null
    }
  ],

note the difference between null and "" in the newValue property.

Comment 6

[Patrick Brosset <:pbro>]

Ahah! https://developer.mozilla.org/en-US/docs/Web/API/Element.getAttribute#Notes

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Ahah!
> https://developer.mozilla.org/en-US/docs/Web/API/Element.getAttribute#Notes

Good find - that looks like exactly the problem.  Copying relevant part from page:

"The implementation of getAttribute in XUL (Gecko) actually follows the DOM 3 Core specification and returns an empty string. Consequently, you should use element.hasAttribute() to check for an attribute's existence prior to calling getAttribute() if it is possible that the requested attribute does not exist on the specified element."

Comment 9

[Patrick Brosset <:pbro>]

So the fix seems simple enough and should be done here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2450

Instead of only returning the value of getAttribute, we should first execute hasAttribute, and if that returns false, just assign null to mutation.newValue.
Something like this:

mutation.newValue = targetNode.hasAttribute(mutation.attributeName)
                    ? targetNode.getAttribute(mutation.attributeName)
                    : null;

Of course, a new markup-view test should be added too.

Comment 10

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Remove XUL attributes when their attributeName is deleted; r=pbrosset

Hi Patrick,

I made a test for the fix, tell me if you think something's missing.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8569827 [details] [diff] [review]
Remove XUL attributes when their attributeName is deleted; r=pbrosset

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

Looks good to me.
If I'm not mistaken, you don't have commit access level 1 yet, which means you can't push that patch to the try server yourself, to check if the tests all pass.
Can you request it? https://www.mozilla.org/en-US/about/governance/policies/commit/

::: browser/devtools/markupview/test/browser_markupview_remove_xul_attributes.js
@@ +22,5 @@
> +
> +  info("Waiting for markupmutation");
> +  yield inspector.once("markupmutation");
> +
> +  is(panelFront.hasAttribute("id"), false, "panelFront doesn\'t have id attribute anymore");

nit: no need for the \ before the single quote.

Comment 12

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Remove XUL attributes when their attributeName is deleted; r=pbrosset

Comment 13

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

You're right, I don't have access yet. I read everything mentioned and requested it. 

Here is my request: https://bugzilla.mozilla.org/show_bug.cgi?id=1137209

Everything was clear except one thing, I'm not sure about the TryChooser Unit Test Suites, I see mochitest-{1,...,5}, etc. Which ones should I choose?

Thanks Patrick!

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #13)
> You're right, I don't have access yet. I read everything mentioned and
> requested it. 
> 
> Here is my request: https://bugzilla.mozilla.org/show_bug.cgi?id=1137209
Vouched for you.
> Everything was clear except one thing, I'm not sure about the TryChooser
> Unit Test Suites, I see mochitest-{1,...,5}, etc. Which ones should I choose?
To push to try, the changeset(s) you push must have a specially formatted commit message. The message serves as an instruction for the server to know which build/test jobs to run.
For most devtools changes, I usually use this one:
try: -b do -p linux,linux64,macosx64,win32 -u xpcshell,mochitest-dt,mochitest-e10s-devtools-chrome,mochitest-o -t none

This builds on all 3 platforms, and runs the mochitest-devtools tests (which is most of the tests we have) on both non-e10s and e10s builds, as well as xpcshell tests (just in case, since we do have some xpcshell devtools tests) and mochitest-o tests too, as we have some of these too.

Comment 15

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

I was assuming the same options (using http://trychooser.pub.build.mozilla.org/).

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7814d5a7f693

Thanks Patrick!

Comment 16

[Patrick Brosset <:pbro>]

Alright, try is green, this should be good to go!
One last thing, the commit message is incorrectly formatted, it should start with the bug number, so:
"Bug 984442 - Remove XUL attributes when their attributeName is deleted; r=pbrosset"
instead of
"Remove XUL attributes when their attributeName is deleted; r=pbrosset"

Can you change this, then mark that new patch as R+ (since I already R+'d the previous one), and add checkin-needed in the "keywords" field at the top of this bug. This will make the bug appear on the sheriffs' radar and they will check it in fx-team when they get a chance.

Comment 17

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 984442 - Remove XUL attributes when their attributeName is deleted; r=pbrosset

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/598b6af6b91f

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Thank you Mahdi for fixing this bug! +1 to you! :)

Comment 20

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Thanks Jared, I enjoy fixing bugs! :)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/598b6af6b91f