NodeActor should have a property for properly cased nodeName

RESOLVED FIXED in Firefox 49

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

47 Branch
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

There are are several bugs that shows issues with node name case both in the client (https://bugzilla.mozilla.org/show_bug.cgi?id=1248381, https://bugzilla.mozilla.org/show_bug.cgi?id=1269887) and on the server (https://bugzilla.mozilla.org/show_bug.cgi?id=1270205).

We used to lowercase element.nodeName, which is wrong for non-HTML tags (e.g. `cliPath`).
As discussed with jdescottes on IRC, I think we should either :
- properly case `nodeName`property on the actor (if it don't break too much tests)
- add a new property (displayName ?), which would be the properly cased nodeName

If none of this is wanted, we might want to create an helper function which can be called from the client and the server.
Priority: -- → P3
See Also: → 1248381, 1269887, 1270205
Assignee: nobody → chevobbe.nicolas
Blocks: 1269887
Blocks: 1270205
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #0)
> We used to lowercase element.nodeName, which is wrong for non-HTML tags
> (e.g. `cliPath`).
Where is this lowercasing done?
> As discussed with jdescottes on IRC, I think we should either :
> - properly case `nodeName`property on the actor (if it don't break too much
> tests)
> - add a new property (displayName ?), which would be the properly cased
> nodeName
First solution seems right to me. I don't see why we should introduce a new property in this case. Also, I'm surely missing something because I don't see anywhere on the server where we lowercase the nodeName. So it feels to me like we're sending the right information from the server already.
(In reply to Patrick Brosset <:pbro> from comment #1)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #0)
> > We used to lowercase element.nodeName, which is wrong for non-HTML tags
> > (e.g. `cliPath`).
> Where is this lowercasing done?

Three locations:
- markup view: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#2731
- breadcrumbs: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/breadcrumbs.js#147
- highlighter: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.css#161

> Also, I'm surely missing something because I don't
> see anywhere on the server where we lowercase the nodeName. So it feels to
> me like we're sending the right information from the server already.

Yes today it is not done, which is the issue. Indeed the server returns the node name as it is stored, which means:
- as authored for XML (svg, xhtml ...) documents -> this is OK
- uppercase for HTML documents -> here we want to display in lowercase

 The idea here is to add a .toLowerCase() on the server, for all nodes in HTML documents (either modifying directly nodeName, or via a new property).
(In reply to Julian Descottes [:jdescottes] from comment #2)
> Yes today it is not done, which is the issue. Indeed the server returns the
> node name as it is stored, which means:
> - as authored for XML (svg, xhtml ...) documents -> this is OK
> - uppercase for HTML documents -> here we want to display in lowercase
> 
>  The idea here is to add a .toLowerCase() on the server, for all nodes in
> HTML documents (either modifying directly nodeName, or via a new property).
Thanks. I read this backward then.
I think it makes sense to lowercase nodeName for nodes from HTML documents then, rather than introduce a new property to the form.
I made the change ( https://hg.mozilla.org/try/rev/30d448a4dc453d1f74d7bbbf408cca7955146f6a ), and pushed to TRY ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e8678c8daf5 ).
There doesn't seem to be any test failing because of this change, so I think it's safe.

Now I wonder if this patch should be just about this change, or if I should handle related bugs (breadcrumb, highlighter, and removing getTagName function in the markup view) here too ?
Attachment #8749590 - Flags: feedback?(jdescottes)
Comment on attachment 8749590 [details] [diff] [review]
lowercase nodeActor.nodeName for HTML elements

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

The code change looks good, but we need another try run fixing the issue I mentioned before moving forward.

Let's update markup-view, breadcrumbs and highlighter in this bug as well.

::: devtools/server/actors/inspector.js
@@ +280,5 @@
>        baseURI: this.rawNode.baseURI,
>        parent: parentNode ? parentNode.actorID : undefined,
>        nodeType: this.rawNode.nodeType,
>        namespaceURI: this.rawNode.namespaceURI,
>        nodeName: this.rawNode.nodeName,

We need to use the new nodeName you computed here :)
Attachment #8749590 - Flags: feedback?(jdescottes) → feedback+
Duplicate of this bug: 1269887
Duplicate of this bug: 1270205
(In reply to Julian Descottes [:jdescottes] from comment #5)
> The code change looks good, but we need another try run fixing the issue I
> mentioned before moving forward.

There's a handful of tests that are failing when I actually switch to the new lowercased nodeName (usually "DIV" !== "div"). I'm gonna fix this and ask for review when done.
While fixing this, I came across another bug : the suggestion results were also lowercase, both server-side (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#2261) and client-side (https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-search.js#455). 

I now have a patch that does not trigger any errors (https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d15e28999a). 

I have a question though. In my patch I changed how NodeFront.tagName was computed : previously, we would get it from NodeFront.nodeName , but has they were a lot of places where we rely on tagName to be uppercase, I chose to get tagName from NodeActor.rawNode.nodeName .
So in my patch, <div>.nodeName === "div" , and <div>.tagName === "DIV" . Does that sounds good for you ? I'm unsure about this because we usually expect tagName to be equal to nodeName .
If we keep tagName equal to nodeName, I could just fix the several assertions where we expect it to be uppercase, but I'm afraid of missing some.
Flags: needinfo?(jdescottes)
While fixing this, I came across another bug : the suggestion results were also lowercase, both server-side (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#2261) and client-side (https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-search.js#455). 

I now have a patch that does not trigger any errors (https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d15e28999a). 

I have a question though. In my patch I changed how NodeFront.tagName was computed : previously, we would get it from NodeFront.nodeName , but has they were a lot of places where we rely on tagName to be uppercase, I chose to get tagName from NodeActor.rawNode.nodeName .
So in my patch, <div>.nodeName === "div" , and <div>.tagName === "DIV" . Does that sounds good for you ? I'm unsure about this because we usually expect tagName to be equal to nodeName .
If we keep tagName equal to nodeName, I could just fix the several assertions where we expect it to be uppercase, but I'm afraid of missing some.
The more I think about this, the more I'm thinking that replacing nodeName is not the good solution. Here's a few things that bother me :

- if we replace nodeName, it won't be what people expect it to be in regards to the specs
- if we replace nodeName, we wont have nodeName == tagName without replacing tagName too
- replacing nodeName (and tagName) requires to replace lots of existing assertion in the code, with the risk of missing some.
- there's some place where we don't have a NodeActor but a simple node, and still want to have proper case (highlighter https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/box-model.js#657 , and search suggestions https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#2261)

With all this in mind, I think it would be better to have a shared helper function (in css-logic.js ?) , and call it when we want to display a node name in the UI.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> The more I think about this, the more I'm thinking that replacing nodeName
> is not the good solution. Here's a few things that bother me :
> 
> - if we replace nodeName, it won't be what people expect it to be in regards
> to the specs
> - if we replace nodeName, we wont have nodeName == tagName without replacing
> tagName too

Both very good points. "The HTML DOM returns the tagName of an HTML element in the canonical uppercase form". IMO, this settles it, nodeName and tagName should stick to the specs. Also agree about the fact that tagName and nodeName should have the same case.

Another issue I overlooked with the current implementation: updating the form's nodeName and removing the lowerCase() in the client means that when connecting the client to an older server, we will display uppercase nodes in the markup view.
Safer approach: perform the lowerCase() in "get nodeName()" of the NodeFront ?

> - there's some place where we don't have a NodeActor but a simple node, and
> still want to have proper case (highlighter
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> highlighters/box-model.js#657 , and search suggestions
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> inspector.js#2261)
> 
> With all this in mind, I think it would be better to have a shared helper
> function (in css-logic.js ?) , and call it when we want to display a node
> name in the UI.

I think you're right we should go with a shared helper. We are going to be using this in both client and server, so it should go to devtools/shared. Most of the callers will be linked to the inspector, but a few won't (actors/object.js for instance). 

I guess it's still OK to have this helper in shared/inspector. Regarding reusing css-logic.js, I am not sure. It looks like the file contains some generic utils despite its name, but maybe we should create a new utils file here (devtools/shared/inspector/utils.js ?)

Forwarding the ni? to pbro, as I also feel I might be missing something.

pbro: Feel free to push back if you think we should still go with a modification of the NodeActor/NodeFront here. Otherwise, what would you do for this new helper method.
Flags: needinfo?(jdescottes) → needinfo?(pbrosset)
Ok, reading the last few comments, keeping the actor form's nodeName and tagName unchanged make sense.
And I like the idea of using the front to expose a helper method that returns the properly cased node name, instead of adding another util file somewhere.
The front may define methods that the actor doesn't have, so maybe we could add a getter like:

get properlyCasedNodeName() {
  return isInHTMLDocument ? this.nodeName.toLowerCase() : this.nodeName;
}

with a better name, obviously.

Then, all places on the client-side that need to display the a user-friendly node name would use this getter.
In fact, we should really just use devtools/client/inspector/shared/dom-node-preview.js everywhere we can on the client.

On the server, you said there's a couple of places where we need to display nodenames too, but these places don't use NodeActor objects, just DOM Nodes, so these places should just do their own thing to properly case the node name, right?
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #14)
> Ok, reading the last few comments, keeping the actor form's nodeName and
> tagName unchanged make sense.
> And I like the idea of using the front to expose a helper method that
> returns the properly cased node name, instead of adding another util file
> somewhere.
> The front may define methods that the actor doesn't have, so maybe we could
> add a getter like:
> 
> get properlyCasedNodeName() {
>   return isInHTMLDocument ? this.nodeName.toLowerCase() : this.nodeName;
> }
> 
> with a better name, obviously.

IIRC, doing it on the NodeFront will require to add another property on the actor to know if the node is a SVG element (a SVG element might be in an html document, but we don't want to lowercase its tagName).

> Then, all places on the client-side that need to display the a user-friendly
> node name would use this getter.
> In fact, we should really just use
> devtools/client/inspector/shared/dom-node-preview.js everywhere we can on
> the client.

Okay, I'll try do do this

> On the server, you said there's a couple of places where we need to display
> nodenames too, but these places don't use NodeActor objects, just DOM Nodes,
> so these places should just do their own thing to properly case the node
> name, right?

Mhh, the original idea was to not have duplicate logic for this (lower case tagName if node is in html document and isn't a svg element), but maybe that's not really a concern ? 

Would it be wrong if we declare a standalone function in inspector.js (e.g. `getDisplayableNodeName(node)`), and then call this in the NodeFront getter (and in the suggestion function on the inspector), and require it in the highlighter ? I have the feeling that it's not ideal, but neither is creating a new util file.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> (In reply to Patrick Brosset <:pbro> from comment #14)
> > Ok, reading the last few comments, keeping the actor form's nodeName and
> > tagName unchanged make sense.
> > And I like the idea of using the front to expose a helper method that
> > returns the properly cased node name, instead of adding another util file
> > somewhere.
> > The front may define methods that the actor doesn't have, so maybe we could
> > add a getter like:
> > 
> > get properlyCasedNodeName() {
> >   return isInHTMLDocument ? this.nodeName.toLowerCase() : this.nodeName;
> > }
> > 
> > with a better name, obviously.
> 
> IIRC, doing it on the NodeFront will require to add another property on the
> actor to know if the node is a SVG element (a SVG element might be in an
> html document, but we don't want to lowercase its tagName).
But you can know if an element is SVG by using its namespaceURI:
On the client, if you have a nodeFront:
nodeFront.namespaceURI === "http://www.w3.org/2000/svg"
On the server, if you have a DOM node:
node.namespaceURI === "http://www.w3.org/2000/svg"

Why would you need another property on the actor's form?

> > Then, all places on the client-side that need to display the a user-friendly
> > node name would use this getter.
> > In fact, we should really just use
> > devtools/client/inspector/shared/dom-node-preview.js everywhere we can on
> > the client.
> 
> Okay, I'll try do do this
This could be done in another bug though.

> > On the server, you said there's a couple of places where we need to display
> > nodenames too, but these places don't use NodeActor objects, just DOM Nodes,
> > so these places should just do their own thing to properly case the node
> > name, right?
> 
> Mhh, the original idea was to not have duplicate logic for this (lower case
> tagName if node is in html document and isn't a svg element), but maybe
> that's not really a concern ? 
I don't know, I felt like this wasn't so much code that we needed to worry too much about sharing it across client and server. But I guess you're right, we could share it in devtools/shared. I'm just not a fan of creating new helper modules, we already have a lot.
> Would it be wrong if we declare a standalone function in inspector.js (e.g.
> `getDisplayableNodeName(node)`), and then call this in the NodeFront getter
> (and in the suggestion function on the inspector), and require it in the
> highlighter ? I have the feeling that it's not ideal, but neither is
> creating a new util file.
Yes, that would work. This could be required anywhere we need it.
(In reply to Patrick Brosset <:pbro> from comment #16)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> > (In reply to Patrick Brosset <:pbro> from comment #14)
> > > Ok, reading the last few comments, keeping the actor form's nodeName and
> > > tagName unchanged make sense.
> > > And I like the idea of using the front to expose a helper method that
> > > returns the properly cased node name, instead of adding another util file
> > > somewhere.
> > > The front may define methods that the actor doesn't have, so maybe we could
> > > add a getter like:
> > > 
> > > get properlyCasedNodeName() {
> > >   return isInHTMLDocument ? this.nodeName.toLowerCase() : this.nodeName;
> > > }
> > > 
> > > with a better name, obviously.
> > 
> > IIRC, doing it on the NodeFront will require to add another property on the
> > actor to know if the node is a SVG element (a SVG element might be in an
> > html document, but we don't want to lowercase its tagName).
> But you can know if an element is SVG by using its namespaceURI:
> On the client, if you have a nodeFront:
> nodeFront.namespaceURI === "http://www.w3.org/2000/svg"
> On the server, if you have a DOM node:
> node.namespaceURI === "http://www.w3.org/2000/svg"
> 
> Why would you need another property on the actor's form?

Oh yes, I missed that we have namespaceURI

> 
> > > Then, all places on the client-side that need to display the a user-friendly
> > > node name would use this getter.
> > > In fact, we should really just use
> > > devtools/client/inspector/shared/dom-node-preview.js everywhere we can on
> > > the client.
> > 
> > Okay, I'll try do do this
> This could be done in another bug though.

Okay, I'll file one later

> > Would it be wrong if we declare a standalone function in inspector.js (e.g.
> > `getDisplayableNodeName(node)`), and then call this in the NodeFront getter
> > (and in the suggestion function on the inspector), and require it in the
> > highlighter ? I have the feeling that it's not ideal, but neither is
> > creating a new util file.
> Yes, that would work. This could be required anywhere we need it.

Deal, let's try that
(In reply to Patrick Brosset <:pbro> from comment #14)
> Ok, reading the last few comments, keeping the actor form's nodeName and
> tagName unchanged make sense.
> And I like the idea of using the front to expose a helper method that
> returns the properly cased node name, instead of adding another util file
> somewhere.
> The front may define methods that the actor doesn't have, so maybe we could
> add a getter like:
> 
> get properlyCasedNodeName() {
>   return isInHTMLDocument ? this.nodeName.toLowerCase() : this.nodeName;
> }

Ehm, why not use localName? AFAIK that always returns the correctly cased tag name.

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #18)
> Ehm, why not use localName? AFAIK that always returns the correctly cased
> tag name.
Oh nice, I actually never looked at the property in details. It does seem to provide the right thing for us.
MDN [1] says that it has been removed from chrome though, does that mean it is now obsolete or not standard? I wouldn't want to introduce a new NodeActor form property for it if that's the case.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Node/localName#Notes
Did not know about this property as well! Reading the spec I am wondering if we should be concerned about the following:

"For nodes of any type other than ELEMENT_NODE and ATTRIBUTE_NODE and nodes created with a DOM Level 1 method, such as createElement from the Document interface, this is always null." [1]

Which seems to be confirmed by the document.createElement() spec [2].

Maybe I don't understand this correctly but it's unclear to me if this property will still be available in the long term on elements created with "createElement". Note that the same goes for "namespaceURI".

Today, all the browsers seem to set namespaceURI, prefix and localName on elements created with "createElement", so maybe I'm just not reading this correctly.

[1] https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-NodeNSLocalN
[2] https://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-2141741547
(In reply to Patrick Brosset <:pbro> from comment #19)
> MDN says that it has been removed from chrome though, does that mean it
> is now obsolete or not standard? I wouldn't want to introduce a new
> NodeActor form property for it if that's the case.

It was removed from the Node interface but it is instead available on the Element and Attr interfaces[1][2][3]. Gecko 48.0 did the same[4], though MDN doesn't reflect that yet.
The point is that localName is useless for nodes other than elements and attributes.

(In reply to Julian Descottes [:jdescottes] from comment #20)
> "For nodes of any type other than ELEMENT_NODE and ATTRIBUTE_NODE and nodes
> created with a DOM Level 1 method, such as createElement from the Document
> interface, this is always null." [1]
> 
> Which seems to be confirmed by the document.createElement() spec [2].
> 
> Maybe I don't understand this correctly but it's unclear to me if this
> property will still be available in the long term on elements created with
> "createElement". Note that the same goes for "namespaceURI".
> 
> Today, all the browsers seem to set namespaceURI, prefix and localName on
> elements created with "createElement", so maybe I'm just not reading this
> correctly.
> 
> [1] https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-NodeNSLocalN
> [2] https://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-2141741547

You are referring to an old spec. DOM4 defines it differently[5]:

"Return a new element that implements interface, with no attributes, namespace set to the HTML namespace [see bug 19431], local name set to localName, and node document set to the context object."
So, obviously localName won't go away completely.

Sebastian

[1] https://dom.spec.whatwg.org/#interface-element
[2] https://dom.spec.whatwg.org/#interface-attr
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=504574
[4] bug 1055776
[5] https://www.w3.org/TR/dom/#dom-document-createelement
Add Element.localName to NodeFront and uses it wherever we display a node name
Use Element.localName directly on the server when we want to display/return
a node name.
Edit some tests to ensure we correctly display node names.

Review commit: https://reviewboard.mozilla.org/r/52407/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52407/
Attachment #8752062 - Flags: review?(jdescottes)
I've pushed my patch to TRY yesterday (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9472f2e244f7&selectedJob=20766964). There were only one error that I fixed before asking for review, so we should be good.
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

https://reviewboard.mozilla.org/r/52407/#review49489

Thanks Nicolas, the code change looks good!
We have a small regression when viewing namespaced documents which we need to address before moving on though.

1\. localName is missing the namespace for prefixed elements. We need to use the prefix here [1]. For instance, <svg:clipPath> has nodeName = svg:clipPath, but localName = clipPath. In order to rebuild the display name, you have to use something like node.prefix ? node.prefix + ":" node.localName : node.localName. This means however exposing one more property to the NodeFront and pushing this logic to all clients. I think I would prefer having a single new property such as displayName that would be created using prefix and localName

2\. In some client files were added fallbacks to tagName.toLowerCase(). Is this for backward compatibility? If yes, maybe we could handle it on the NodeFront directly? This way client callers can simply use the front property and don't have to duplicate this logic.

3\. In markup.js::getTagName we are still checking if the document is not SVG and then doing a lowercase on the nodeName. I guess this should be migrated to the new property as well?

[1] https://developer.mozilla.org/en-US/docs/Web/API/Element/prefix

::: devtools/client/inspector/rules/models/rule.js:86
(Diff revision 1)
>      if (this._inheritedSource) {
>        return this._inheritedSource;
>      }
>      this._inheritedSource = "";
>      if (this.inherited) {
> -      let eltText = this.inherited.tagName.toLowerCase();
> +      let eltText = this.inherited.tagName.localName ||

This should be this.inherited.localName.

Any reason to keep the fallback to tagName.toLowerCase() ?

::: devtools/server/actors/highlighters/box-model.js:657
(Diff revision 1)
>  
>      let {bindingElement: node, pseudo} =
>          getBindingElementAndPseudo(this.currentNode);
>  
>      // Update the tag, id, classes, pseudo-classes and dimensions
> -    let tagName = node.tagName;
> +    let tagName = node.localName;

Should rename the variable to be consistent

::: devtools/server/actors/object.js:1580
(Diff revision 1)
>  
>      let preview = grip.preview = {
>        kind: "DOMNode",
>        nodeType: rawObj.nodeType,
>        nodeName: rawObj.nodeName,
> +      localName: rawObj.localName,

Correct me if I'm wrong but I don't think this new property is used? 

The nodename is still lowercased a few lines below for dom elements in HTML documents. Maybe we should leave the console migration for a follow up bug.
Attachment #8752062 - Flags: review?(jdescottes)
Attachment #8749590 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/52407/#review49489

Thanks for reviewing this Julian, sorry it had many mistakes :/ 
Does the tests looks good to you ? 

1. oh okay, I didn't thought about that syntax. Now I'm reluctant to use localName, it seems to require extra work in comparison of what we already have with nodeName and namespaceURI. I'll add a new displayName property and create a standalone getDisplayName(nodeName) function in inspector.js that can be required in the highlighter , like I was going to do before we talked about localName.

2. Yes, it was indeed to make sure a newer client can cope with an older server, where NodeActor.localName wouldn't be available. I might getting this wrong, but on an older server, the NodeFront won't have the localName/displayName property too right ? 

3. Yes, I missed this one, I'll delete it

> This should be this.inherited.localName.
> 
> Any reason to keep the fallback to tagName.toLowerCase() ?

erf, sorry about that.

> Correct me if I'm wrong but I don't think this new property is used? 
> 
> The nodename is still lowercased a few lines below for dom elements in HTML documents. Maybe we should leave the console migration for a follow up bug.

You're right, maybe I wanted to handle it on the client, but it does work without this (because we call toLowerCase only for HTMLElement), I'll remove this
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #25)
> https://reviewboard.mozilla.org/r/52407/#review49489
> 
> Thanks for reviewing this Julian, sorry it had many mistakes :/ 

Your patch is fine, really! That's a tricky bug, touching a lot of different layers. Thanks for handling this again! Also we had no tests checking the issue I mentioned, don't feel bad about this :)

> Does the tests looks good to you ? 

Yes, looks good to me. I was just wondering if we should add more tests to cover the issues with the namespaced elements. I think I will file a follow up for this.

> 
> 1. oh okay, I didn't thought about that syntax. Now I'm reluctant to use
> localName, it seems to require extra work in comparison of what we already
> have with nodeName and namespaceURI. I'll add a new displayName property and
> create a standalone getDisplayName(nodeName) function in inspector.js that
> can be required in the highlighter , like I was going to do before we talked
> about localName.
> 

I think the new property is a great approach. For the implementation of the getter we can either
- use namespaceURI to check for SVG (keeping in mind we don't cover XHTML for now)
- switch to prefix:localName in the property getter

I am leaning towards prefix+localName because it feels simpler (no need to check the document namespace, or the node namespace). But no strong preference, so up to you.

> 2. Yes, it was indeed to make sure a newer client can cope with an older
> server, where NodeActor.localName wouldn't be available. I might getting
> this wrong, but on an older server, the NodeFront won't have the
> localName/displayName property too right ? 
> 

That's the tricky part with our actor files. They are requested on the client and on the server. The server uses the Actor part only, and the client uses the Front only. We are in the middle of decoupling the two which will make this easier two understand.

Long story short: the NodeFront is on the client, and it is a great entry point to handle backward compatibility issues. You can test the properties available in the form, and do your best depending on what is available.

Which means we could have on the front (assuming we use localName for the sake of the example):
get displayName() {
  let {localName, prefix, nodeName} = this._form;
  if (!localName && !prefix) {
    return nodeName.toLowerCase(); // Not sure how far to go with the fallback, feel like this is enough
  }
  return prefix ? prefix + ":" + localName : localName;
} 

> 3. Yes, I missed this one, I'll delete it
> 
> > This should be this.inherited.localName.
> > 
> > Any reason to keep the fallback to tagName.toLowerCase() ?
> 
> erf, sorry about that.
> 
> > Correct me if I'm wrong but I don't think this new property is used? 
> > 
> > The nodename is still lowercased a few lines below for dom elements in HTML documents. Maybe we should leave the console migration for a follow up bug.
> 
> You're right, maybe I wanted to handle it on the client, but it does work
> without this (because we call toLowerCase only for HTMLElement), I'll remove
> this
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52407/diff/1-2/
Attachment #8752062 - Flags: feedback+ → review?(jdescottes)
Julian, you might not want to look at the interdiff as I rebased and there were the commit for the separation of the actor and the front since my first patch.

I did not added tests for namespaced element since you said that you'd file a bug for this.
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

https://reviewboard.mozilla.org/r/52407/#review49826

Thanks for submitting a new patch Nicolas, looks really good to me! 
Just a few ":" missing in the actors. We can fix them one by one, or slightly change the implementation (see my comment below).
Up to you! Either way I don't think another review is mandatory.

I opened Bug 1273323 as a follow up to add more tests around the namespaced elements.

::: devtools/server/actors/highlighters/box-model.js:657
(Diff revision 2)
>  
>      let {bindingElement: node, pseudo} =
>          getBindingElementAndPseudo(this.currentNode);
>  
>      // Update the tag, id, classes, pseudo-classes and dimensions
> -    let tagName = node.tagName;
> +    let displayName = (node.prefix ? node.prefix : "") + node.localName;

node.prefix + ":"

::: devtools/server/actors/inspector.js:248
(Diff revision 2)
>        parent: parentNode ? parentNode.actorID : undefined,
>        nodeType: this.rawNode.nodeType,
>        namespaceURI: this.rawNode.namespaceURI,
>        nodeName: this.rawNode.nodeName,
> +      localName: this.rawNode.localName,
> +      prefix: this.rawNode.prefix,

I'm wondering if instead of duplicating the logic between the client and the server we should not compute the displayName on the server and return it in the form (and no longer send prefix / localName of course).

The NodeFront could still test if form.displayName is available and if not fallback on nodeName.toLowerCase()
The good thing is that we could easily put this logic in a helper method of the inspector actor. Could import it and use it also in the styles and box-model actors.

What do you think? (sorry for changing my mind about this Nicolas)

::: devtools/server/actors/inspector.js:1644
(Diff revision 2)
>            nodes = this._multiFrameQuerySelectorAll("*");
>          } else {
>            nodes = this._multiFrameQuerySelectorAll(query);
>          }
>          for (let node of nodes) {
> -          let tag = node.tagName.toLowerCase();
> +          let tag = (node.prefix ? node.prefix : "") + node.localName;

Should be `node.prefix + ":"` here.

::: devtools/server/actors/inspector.js:1671
(Diff revision 2)
>  
>        case "null":
>          nodes = this._multiFrameQuerySelectorAll(query);
>          for (let node of nodes) {
>            sugs.ids.set(node.id, (sugs.ids.get(node.id)|0) + 1);
> -          let tag = node.tagName.toLowerCase();
> +          let tag = (node.prefix ? node.prefix : "") + node.localName;

node.prefix + ":"

::: devtools/server/actors/styles.js:895
(Diff revision 2)
>      if (rawNode.id) {
>        selector = "#" + CSS.escape(rawNode.id);
>      } else if (rawNode.className) {
>        selector = "." + [...rawNode.classList].map(c => CSS.escape(c)).join(".");
>      } else {
> -      selector = rawNode.tagName.toLowerCase();
> +      selector = (rawNode.prefix ? rawNode.prefix : "") + rawNode.localName;

rawNode.prefix + ":"
Attachment #8752062 - Flags: review?(jdescottes) → review+
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52407/diff/2-3/
I added a displayName property on the NodeActor like you suggested and handled the fallback in the NodeFront (the separation of the actor and the front makes it more clear indeed !).

In the search suggestion (actor/inspector.js) and the style.js , we just use Element.localName because we want to output a CSS selector, and thus, we don't want something like `svg:svg`, because it is invalid. 

Thanks for filing to follow-up bug for namespaced node.
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

Asking for review again because the patch has substantial changes
Attachment #8752062 - Flags: review+ → review?(jdescottes)
Attachment #8752062 - Flags: review?(jdescottes) → review+
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

https://reviewboard.mozilla.org/r/52407/#review50232

Let's fix the "add new rule" issue + some nits.

Can you check if we have a test for this feature on nodes without id/classname? 
If not, let's add a comment to the test bug we filed?

::: devtools/client/fronts/inspector.js:212
(Diff revisions 2 - 3)
>    get nodeName() {
>      return this._form.nodeName;
>    },
>    get displayName() {
> -    let {localName, prefix, nodeName} = this._form;
> -    if (!localName && !prefix) {
> +    let {displayName, nodeName} = this._form;
> +    return displayName || nodeName.toLowerCase();

nit: add a comment to say that nodeName.toLowerCase() is kept for backward compatibility.

::: devtools/server/actors/highlighters/box-model.js:15
(Diff revisions 2 - 3)
>    CanvasFrameAnonymousContentHelper,
>    getBindingElementAndPseudo, hasPseudoClassLock, getComputedStyle,
>    createSVGNode, createNode, isNodeValid } = require("./utils/markup");
>  const { getCurrentZoom,
>    setIgnoreLayoutChanges } = require("devtools/shared/layout/utils");
> +const inspector = require("devtools/server/actors/inspector");

nit: maybe just import const {getNodeDisplayName} ?

::: devtools/server/actors/inspector.js:185
(Diff revisions 2 - 3)
>  // when it receives the node to be inspected over the message manager.
>  exports.setInspectingNode = function (val) {
>    gInspectingNode = val;
>  };
>  
> +const getNodeDisplayName = function (rawNode) {

nit: JS Doc here? to explain the goal is to have a properly cased version of the tagName?

::: devtools/server/actors/styles.js:895
(Diff revisions 2 - 3)
>      if (rawNode.id) {
>        selector = "#" + CSS.escape(rawNode.id);
>      } else if (rawNode.className) {
>        selector = "." + [...rawNode.classList].map(c => CSS.escape(c)).join(".");
>      } else {
> -      selector = (rawNode.prefix ? rawNode.prefix : "") + rawNode.localName;
> +      selector = node.localName;

This should be rawNode.localName otherwise addNewRule breaks when the node has no id or className.

Do you know if we have tests for this?
https://reviewboard.mozilla.org/r/52407/#review50232

If I understand correctly, we'll fix Bug 1272208 within this patch ?

> nit: maybe just import const {getNodeDisplayName} ?

That's what I wanted to do, but it does not seems to work. I was going to ask you if you knew why :)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #34)
> https://reviewboard.mozilla.org/r/52407/#review50232
> 
> If I understand correctly, we'll fix Bug 1272208 within this patch ?
> 

Not really, the current patch creates a regression: you can no longer add a new rule on an element which has no classname or no id. That's because localName is not available on NodeFront, only on the rawNode property. 
We just need to patch the styles actor I think?

> > nit: maybe just import const {getNodeDisplayName} ?
> 
> That's what I wanted to do, but it does not seems to work. I was going to
> ask you if you knew why :)

Indeed! The require returns an empty object which is only filled with the exported properties later on. Let's keep it like that for now.
(In reply to Julian Descottes [:jdescottes] from comment #35)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #34)
> > https://reviewboard.mozilla.org/r/52407/#review50232
> > 
> > If I understand correctly, we'll fix Bug 1272208 within this patch ?
> > 
> 
> Not really, the current patch creates a regression: you can no longer add a
> new rule on an element which has no classname or no id. That's because
> localName is not available on NodeFront, only on the rawNode property. 
> We just need to patch the styles actor I think?

/me punch himself in the head

> > > nit: maybe just import const {getNodeDisplayName} ?
> > 
> > That's what I wanted to do, but it does not seems to work. I was going to
> > ask you if you knew why :)
> 
> Indeed! The require returns an empty object which is only filled with the
> exported properties later on. Let's keep it like that for now.

okay !
> This should be rawNode.localName otherwise addNewRule breaks when the node
> has no id or className.
> 
> Do you know if we have tests for this?

We do have one : https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_add-rule_01.js , and it did break with my change. Fixing it now
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52407/diff/3-4/
TRY run is over (https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46787b83155&selectedJob=21057759), and even if there's lots of failures (mainly, timeouts), I checked and none of them concerned one of the test I touched, nor seem to be caused by my patch.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Needs rebasing against fx-team tip.
Keywords: checkin-needed
Comment on attachment 8752062 [details]
MozReview Request: Bug 1270215 - Ensure we display properly cased node names all across the devtools. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52407/diff/4-5/
(In reply to Ryan VanderMeulen [:RyanVM] from comment #40)
> Needs rebasing against fx-team tip.

Rebased https://bugzilla.mozilla.org/attachment.cgi?id=8752062
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50a5ded6bc42
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.