Closed Bug 895561 Opened 11 years ago Closed 11 years ago

"Edit As HTML" option in the markup view

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 27

People

(Reporter: harth, Assigned: bgrins)

References

Details

Attachments

(3 files, 8 obsolete files)

edit-html-toolkit-2.patch
9.53 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
interdiff.diff
15.02 KB, patch
Details | Diff | Splinter Review
edit-html-browser-4.patch
49.89 KB, patch
bgrins
: review+
Details | Diff | Splinter Review 
Right clicking a node and doing "Edit as HTML" should let you update the innerHTML of an element (and thus, a way to add nodes).
erg, outerHTML, not inner
Assignee: nobody → bgrinstead
Agreed - I really missed this feature when I first switched from Firebug to Firefox, so much so that I wrote an extension with this feature: https://addons.mozilla.org/en-US/firefox/addon/devtools-tweaks/

I'm changing innerHTML, and it works well, I didn't try outerHTML, I wonder if there would be any conflicts if used that way? The only bad thing I've found about the edit-HTML feature (Firebug or devtools) is it seems it can remove event-listeners that were referencing an element, which is removed and replaced when setting innerHTML. I would imagine this is the reason the Firefox edit-HTML option has not been included from the beginning?
(In reply to Luke from comment #4)
> Agreed - I really missed this feature when I first switched from Firebug to
> Firefox, so much so that I wrote an extension with this feature:
> https://addons.mozilla.org/en-US/firefox/addon/devtools-tweaks/

Nice, I installed the extension - looks good.  I like the smaller tabs btw - take a look at Bug 916766 if you want to track some of the UI changes coming in soon.  Regarding the "Adds New Rule and Open URL to the CSS inspector.", I'm sure it wasn't there when you made the extension, but is this not already inside of devtools?  If you wouldn't mind posting to the mailing list (https://lists.mozilla.org/listinfo/dev-developer-tools) with a list of the features that are not yet in devtools that you have implemented in the addon, we may be able to point you towards existing bugs or file new ones for them.

> I'm changing innerHTML, and it works well, I didn't try outerHTML, I wonder
> if there would be any conflicts if used that way? The only bad thing I've
> found about the edit-HTML feature (Firebug or devtools) is it seems it can
> remove event-listeners that were referencing an element, which is removed
> and replaced when setting innerHTML. I would imagine this is the reason the
> Firefox edit-HTML option has not been included from the beginning?

Yes, you are right about the problem with removing event listeners.  We have some ideas about how to handle this, but are going to move forward with this more simple method of setting outerHTML to get the UI in place and allow a quicker turnaround for this highly requested features.  After this, in another bug, we will work on preserving the DOM tree as much as possible while editing.
Yeah, my "Add New Rule" can actually be accomplished by live-editing any css in the styles tab, and now that urls are links in css inspector in newer Firefox, the main advantage to my version is that it isn't affected by bug 921686.
Depends on: 919709
Attached patch edit-html-WIP-1.patch (obsolete) — — Splinter Review 
Work in progress.  Right click on an element -> Edit HTML.  This opens up a  CodeMirror instance in splace of the tag (with dark theme and light theme matching the markup view).  You can cmd/ctrl+enter to automatically save and close the editor, or press esc to close without saving.
One weird thing that I've noticed is that when you set `document.body.outerHTML = document.body.outerHTML` it adds a new <head> tag.  Likewise, if you set `document.head.outerHTML = document.head.outerHTML` it adds an additional <body> tag.
Might be strange side-effects of the HTML tree construction algorithm[1].  Not sure if bits of that are re-run in certain ways when using outerHTML.

[1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction.html#the-before-head-insertion-mode
Attached patch edit-html-WIP-2.patch (obsolete) — — Splinter Review 
This patch is ready for review **except** for the following files (basically anything touching codemirror or themes).
 - browser/devtools/sourceeditor/codemirror/README
 - browser/devtools/sourceeditor/codemirror/htmlmixed.js
 - browser/devtools/sourceeditor/editor.js
 - browser/themes/shared/devtools/dark-theme.css
 - browser/themes/shared/devtools/light-theme.css

One specific thing I was thinking about and hoping to get some feedback on is:
 - Once an edit is finished, we want to reselect the node that just got changed.  Right now I keep track of which index the node is inside of the parent element, and then after mutations I reselect the node of the same index inside of the parent.  Can you think of a better way to track / reselect the node after the outerHTML is set?  We can't just use the original node since its reference is invalidated by the outerHTML call, and we don't get a reference to the new one in the 'added' array of the mutation by design in the WalkerActor: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#1717.  Even if we did get a reference to the new added node, I don't believe that we can guarantee that it is the new one created by the edit, and not just some random change that happened at the same time.
Attachment #818023 - Attachment is obsolete: true
Attachment #820399 - Flags: review?(jwalker)
When I test with the simulator (1.3), "Edit as HTML" will open the textarea with the correct content, but saving will fail with "unrecognizedPacketType".  Presumably this is because the server introduces a new method for setOuterHTML (but .outerHTML was already there which explains why it opens up with the correct content).  Is there a way to get this to work or should we conditionally hide this option from the context menu if the method isn't supported by the remote device?  If we should hide it, what is the best way to feature test for this new method on the server?
Flags: needinfo?(jryans)
Can you split your patch in 2 patches? /browser/ and /toolkit/.
We can try to get the /toolkit/ patch into FxOS 1.2.

Being able to test if setOuterHTML is available sounds important.

Panos, what is the best way to test if a feature is present on the server side?
Flags: needinfo?(jryans) → needinfo?(past)
Comment on attachment 820399 [details] [diff] [review]
edit-html-WIP-2.patch

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

I've not looked at html-editor.js and markup-view.js yet.

::: browser/devtools/inspector/inspector-panel.js
@@ +717,5 @@
> +      return;
> +    }
> +    if (this.markup) {
> +      this.markup.beginEditingOuterHTML(this.selection.nodeFront);
> +    }

I wonder if we need to do something better than ignore the users request if they're not on a node or there is no markup. Might we be able to grey out the menu? (Fine for this to be done in a follow-up)

::: browser/devtools/markupview/markup-view.css
@@ +17,5 @@
> +   position:relative;
> +}
> +
> +.html-editor {
> +  display : flex;

Some formatting nits in this file...

@@ +23,5 @@
> +  display:none;
> +  position:absolute;
> +  z-index: 2;
> +  margin-left: -1000em;
> +  padding-left: 1000em;

eh?
What's the logic behind this?

::: browser/devtools/markupview/test/head.js
@@ +8,5 @@
>  let TargetFactory = devtools.TargetFactory;
>  
> +let tempScope = {};;
> +Components.utils.import("resource://gre/modules/devtools/Console.jsm", tempScope);
> +let console = tempScope.console;

This incantation is the same as:

    let {console} = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;

(I think the second parameter and return value to Cu.import came after Console.jsm)

::: browser/devtools/sourceeditor/codemirror/htmlmixed.js
@@ +1,1 @@
> +// TODO actually recognize syntax of TypeScript constructs

Have you changed this file since it being in codemirror?

::: browser/devtools/sourceeditor/editor.js
@@ +49,1 @@
>  [ "    <link rel='stylesheet' href='" + style + "'>" for (style of CM_STYLES) ].join("\n") +

I know this isn't your code, but could you unroll this loop?

2 reasons:
* string concatenation in HTML is a good place for us to trip up and cause a security problem
* The unrolled version is simpler and fewer lines of code

::: browser/themes/shared/devtools/dark-theme.css
@@ +50,5 @@
> +.theme-bg-none {
> +  background: transparent !important;
> +}
> +
> +.theme-bg-darker, .cm-s-default .CodeMirror-gutters {

Formatting again (NL after ',')

::: browser/themes/shared/devtools/light-theme.css
@@ +47,5 @@
>    background-color: #CCC;
>  }
>  
> +.theme-bg-none {
> +  background: transparent !important;

!important is a bit of a nuclear option. Is there an gentler way to make this rule win?

@@ +109,5 @@
>    color: black;
>  }
>  
> +
> +.CodeMirror pre {color: black;}

Please could you format this like the other lines
Attached patch edit-html-browser-1.patch (obsolete) — — Splinter Review 
Split the patch into two pieces - one for toolkit and one for browser.  Addressed initial comments.
Attachment #820399 - Attachment is obsolete: true
Attachment #820399 - Flags: review?(jwalker)
Attachment #820508 - Flags: review?(jwalker)
Attached patch edit-html-toolkit-1.patch (obsolete) — — Splinter Review 

        Attachment #820509 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        edit-html-browser-1.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Minor follow up on browser patch that fixes formatting and layout issue

        Attachment #820508 -
        Attachment is obsolete: true

        Attachment #820508 -
        Flags: review?(jwalker)

        Attachment #820522 -
        Flags: review?(jwalker)

    
    

    
  
(In reply to Joe Walker [:jwalker] from comment #14)
> Comment on attachment 820399 [details] [diff] [review]
> edit-html-WIP-2.patch
> 
> Review of attachment 820399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've not looked at html-editor.js and markup-view.js yet.
> 
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +717,5 @@
> > +      return;
> > +    }
> > +    if (this.markup) {
> > +      this.markup.beginEditingOuterHTML(this.selection.nodeFront);
> > +    }
> 
> I wonder if we need to do something better than ignore the users request if
> they're not on a node or there is no markup. Might we be able to grey out
> the menu? (Fine for this to be done in a follow-up)

These checks may be a bit over defensive, was mostly copying the style of similar things (like delete node and copy inner HTML).  It does disable on non elements in inspector-panel.js:  editHTML.setAttribute("disabled", "true");.  I'm not really sure if there is a case where this action will be requested in either of these cases, so we could possibly remove the checks.

> 
> ::: browser/devtools/markupview/markup-view.css
> @@ +17,5 @@
> > +   position:relative;
> > +}
> > +
> > +.html-editor {
> > +  display : flex;
> 
> Some formatting nits in this file...

I believe all the formatting nits are fixed.

> 
> @@ +23,5 @@
> > +  display:none;
> > +  position:absolute;
> > +  z-index: 2;
> > +  margin-left: -1000em;
> > +  padding-left: 1000em;
> 
> eh?
> What's the logic behind this?

Added a comment explaining why this is there (helps cover hover effects and the collapse arrows behind it).

> 
> ::: browser/devtools/markupview/test/head.js
> @@ +8,5 @@
> >  let TargetFactory = devtools.TargetFactory;
> >  
> > +let tempScope = {};;
> > +Components.utils.import("resource://gre/modules/devtools/Console.jsm", tempScope);
> > +let console = tempScope.console;
> 
> This incantation is the same as:
> 
>     let {console} = Cu.import("resource://gre/modules/devtools/Console.jsm",
> {}).console;
> 
> (I think the second parameter and return value to Cu.import came after
> Console.jsm)
> 

Updated

> ::: browser/devtools/sourceeditor/codemirror/htmlmixed.js
> @@ +1,1 @@
> > +// TODO actually recognize syntax of TypeScript constructs
> 
> Have you changed this file since it being in codemirror?
> 
> ::: browser/devtools/sourceeditor/editor.js
> @@ +49,1 @@
> >  [ "    <link rel='stylesheet' href='" + style + "'>" for (style of CM_STYLES) ].join("\n") +
> 
> I know this isn't your code, but could you unroll this loop?
> 
> 2 reasons:
> * string concatenation in HTML is a good place for us to trip up and cause a
> security problem
> * The unrolled version is simpler and fewer lines of code
> 

Waiting until Bug 919709 lands to make any more changes to this file.

> ::: browser/themes/shared/devtools/light-theme.css
> @@ +47,5 @@
> >    background-color: #CCC;
> >  }
> >  
> > +.theme-bg-none {
> > +  background: transparent !important;
> 
> !important is a bit of a nuclear option. Is there an gentler way to make
> this rule win?
> 

This isn't used anymore - I've removed it in both themes.

    
    

    
  
(In reply to Paul Rouget [:paul] from comment #13)
> Being able to test if setOuterHTML is available sounds important.
> 
> Panos, what is the best way to test if a feature is present on the server
> side?

The debugger client has an elaborate mechanism in place for that purpose:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#l757

If you are using protocol.js, that won't be useful to you, but the gist of it is:

- see if you can deduce the API level of the server from already received packets
- send a request for a new API and look for an unrecognizedPacketType response (or for a modified API and look for particular error responses, like missingParameter or badParameterType)
- check the root actor's hello packet for any traits that indicate a particular API level
Flags: needinfo?(past)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        edit-html-browser-2.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Rebased after Bug 919709 has landed.  This should get rid of a lot of the noise in the previous patch, since the new CM mode is already included in fx-team.

One question:

> I know this isn't your code, but could you unroll this loop?
> 
> 2 reasons:
> * string concatenation in HTML is a good place for us to trip up and cause a
> security problem
> * The unrolled version is simpler and fewer lines of code
> 

Can you give an example of what you mean here?  Do you mean just inline the 3 <link> tags rather than store them in an array and iterate?

        Attachment #820522 -
        Attachment is obsolete: true

        Attachment #820522 -
        Flags: review?(jwalker)

        Attachment #820737 -
        Flags: review?(jwalker)

        Attachment #820509 -
        Flags: review?(jwalker) → review+

    
    

    
  
Comment on attachment 820737 [details] [diff] [review]
edit-html-browser-2.patch

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

::: browser/devtools/markupview/html-editor.js
@@ +77,5 @@
> +   *
> +   * @param string value
> +   *        The text to set the editor to.
> +   */
> +  setText: function(value)

Can I suggest inlining this function, and calling this._value this._originalValue?

It's only called from one place, and there's a danger that other users could thing that it's safe to call setText to update the current state (which would break the 'is it changed' detection on hide.

And on a similar (athough not dangerous) note, do we need getText()?

@@ +132,5 @@
> +  _detach: function()
> +  {
> +    if (this._attachedElement) {
> +      this._attachedElement.classList.remove("html-editor-container");
> +      delete this._attachedElement;

Nit:

    this._attachedElement = undefined;

Will stop us going into dictionary mode.

::: browser/devtools/markupview/markup-view.js
@@ +433,5 @@
> +          reselectParent = target;
> +          reselectChildIndex = this._outerHTMLChildIndex;
> +
> +          delete this._outerHTMLNode;
> +          delete this._outerHTMLChildIndex;

Nit: dictionary mode.

@@ +618,5 @@
> +    // return -1 indicating that it isn't inside of its parent children list.
> +    if (!parentNode) {
> +      def.resolve(-1);
> +    } else {
> +      this.walker.children(parentNode).then((o) => {

Double nit here and in other places. Feel free to ignore both:
'o' isn't a very descriptive variable name. My gut reaction is that 'i' is fine as everyone recognises it as a loop variable, but 'o' doesn't have the same instant recognition to me.
Also, you don't need the brackets round it '(o)'.

::: browser/devtools/markupview/test/head.js
@@ +5,5 @@
>  const Cu = Components.utils;
>  
>  let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
>  let TargetFactory = devtools.TargetFactory;
> +let {console} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;

(Wrong URL, missing .console)

::: browser/devtools/sourceeditor/editor.js
@@ +49,5 @@
>    "  <head>" +
>    "    <style>" +
>    "      html, body { height: 100%; }" +
>    "      body { margin: 0; overflow: hidden; }" +
> +  "      .CodeMirror { width: 100%; height: 100% !important; background: transparent !important; }" +

Are these !importants still needed?

@@ +177,5 @@
>    appendTo: function (el) {
>      let def = promise.defer();
>      let cm  = editors.get(this);
>      let doc = el.ownerDocument;
> +    let env = doc.createElement("iframe");

XUL iframes and HTML iframes have some subtle differences (I can't remember what right now other than it has tripped me up in the past) It seems more correct to have an HTML iframe here, but I thought it was worth keeping in mind.

::: browser/themes/shared/devtools/dark-theme.css
@@ +137,5 @@
> +.cm-s-default .cm-property { color: #eed1b3; }
> +.cm-s-default .cm-operator {color: #fa8d6a;}
> +.cm-s-default .cm-number { color: #78CF8A; }
> +.cm-s-default .cm-def { color: #aac6e3; }
> +.cm-s-default .cm-comment { color: #555; font-style:italic; }

Shouldn't there be more symmetry between this and light-theme.css?

        Attachment #820737 -
        Flags: review?(jwalker) → review+

    
    

    
  
(In reply to Joe Walker [:jwalker] from comment #21)
> Comment on attachment 820737 [details] [diff] [review]
> edit-html-browser-2.patch
> 
> Review of attachment 820737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Great feedback, thanks.

> ::: browser/devtools/markupview/html-editor.js
> @@ +77,5 @@
> > +   *
> > +   * @param string value
> > +   *        The text to set the editor to.
> > +   */
> > +  setText: function(value)
> 
> Can I suggest inlining this function, and calling this._value
> this._originalValue?
> 
> It's only called from one place, and there's a danger that other users could
> thing that it's safe to call setText to update the current state (which
> would break the 'is it changed' detection on hide.
> 
> And on a similar (athough not dangerous) note, do we need getText()?
> 

Removed both functions, makes the interface simpler and safer.

> @@ +132,5 @@
> > +  _detach: function()
> > +  {
> > +    if (this._attachedElement) {
> > +      this._attachedElement.classList.remove("html-editor-container");
> > +      delete this._attachedElement;
> 
> Nit:
> 
>     this._attachedElement = undefined;
> 
> Will stop us going into dictionary mode.
> 
> ::: browser/devtools/markupview/markup-view.js
> @@ +433,5 @@
> > +          reselectParent = target;
> > +          reselectChildIndex = this._outerHTMLChildIndex;
> > +
> > +          delete this._outerHTMLNode;
> > +          delete this._outerHTMLChildIndex;
> 
> Nit: dictionary mode.
> 

Removed all `deletes` in the file.

> @@ +618,5 @@
> > +    // return -1 indicating that it isn't inside of its parent children list.
> > +    if (!parentNode) {
> > +      def.resolve(-1);
> > +    } else {
> > +      this.walker.children(parentNode).then((o) => {
> 
> Double nit here and in other places. Feel free to ignore both:
> 'o' isn't a very descriptive variable name. My gut reaction is that 'i' is
> fine as everyone recognises it as a loop variable, but 'o' doesn't have the
> same instant recognition to me.
> Also, you don't need the brackets round it '(o)'.
> 

Updated with better variable name and removed parens.

> ::: browser/devtools/markupview/test/head.js
> @@ +5,5 @@
> >  const Cu = Components.utils;
> >  
> >  let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> >  let TargetFactory = devtools.TargetFactory;
> > +let {console} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> 
> let console = (Cu.import("resource://gre/modules/devtools/Console.jsm",
> {})).console;
> 
> (Wrong URL, missing .console)
> 

Of course, good catch.

> ::: browser/devtools/sourceeditor/editor.js
> @@ +49,5 @@
> >    "  <head>" +
> >    "    <style>" +
> >    "      html, body { height: 100%; }" +
> >    "      body { margin: 0; overflow: hidden; }" +
> > +  "      .CodeMirror { width: 100%; height: 100% !important; background: transparent !important; }" +
> 
> Are these !importants still needed?

Unfortunately, yes.  Both the background and height are set on the themes.  The background allows us to apply the background color on the body (theme-body) and the height fixes a variety of weird problems.

> @@ +177,5 @@
> >    appendTo: function (el) {
> >      let def = promise.defer();
> >      let cm  = editors.get(this);
> >      let doc = el.ownerDocument;
> > +    let env = doc.createElement("iframe");
> 
> XUL iframes and HTML iframes have some subtle differences (I can't remember
> what right now other than it has tripped me up in the past) It seems more
> correct to have an HTML iframe here, but I thought it was worth keeping in
> mind.
> 

There was some talk in #devtools about this.  It seemed like using createElement would work in our case here, but I will double check scratchpad and debugger to make sure they are still working, and also attempt to create it in the HTML NS and see what happens.

> ::: browser/themes/shared/devtools/dark-theme.css
> @@ +137,5 @@
> > +.cm-s-default .cm-property { color: #eed1b3; }
> > +.cm-s-default .cm-operator {color: #fa8d6a;}
> > +.cm-s-default .cm-number { color: #78CF8A; }
> > +.cm-s-default .cm-def { color: #aac6e3; }
> > +.cm-s-default .cm-comment { color: #555; font-style:italic; }
> 
> Shouldn't there be more symmetry between this and light-theme.css?

Yes, I'm working on the theming right now and will reupload the patch once this is better.

    
    

    
  


  
Nick, 
I made a small change to debugger client to expose the traits object.  This makes checking a simple boolean much easier, since I don't have to make the stub FeatureCompatibilityShim and I can get the trait synchronously from inspector-panel.  Can you give a quick review to dbg-client.js and root.js and let me know if that seems reasonable?

        Attachment #820509 -
        Attachment is obsolete: true

        Attachment #821106 -
        Flags: review?(nfitzgerald)

        Attachment #821106 -
        Flags: review?(nfitzgerald) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        edit-html-browser-3.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Joe,
The entire patch (including theme and editor changes) is ready for review now.  I've also added the ability to pref off this feature.  You will need to apply the new toolkit patch first to for this to run.

        Attachment #820737 -
        Attachment is obsolete: true

        Attachment #821111 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        edit-html-browser-3.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry for all the uploads - in this version I updated some of the theming, and matched the line height inside of the markup view so it should look more consistent now (the editor's lines were a little too close together before).  Regarding feedback from earlier, I was able to get rid of the !important on background in the editor by setting transparent in the theme.

Also pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=75706b9bb085

        Attachment #821111 -
        Attachment is obsolete: true

        Attachment #821111 -
        Flags: review?(jwalker)

        Attachment #821267 -
        Flags: review?(jwalker)

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #25)
> Created attachment 821267 [details] [diff] [review]
> edit-html-browser-3.patch
> 
> Sorry for all the uploads - in this version I updated some of the theming,
> and matched the line height inside of the markup view so it should look more
> consistent now (the editor's lines were a little too close together before).
> Regarding feedback from earlier, I was able to get rid of the !important on
> background in the editor by setting transparent in the theme.
> 
> Also pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=75706b9bb085

Please could you upload an interdiff? i.e. a diff of the diffs. That will help me carry on where you left off.

BTW - bugzilla has an interdiff feature, but it's broken.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        interdiff.diff
        — 
          — Splinter Review
      

    


  
> Please could you upload an interdiff? i.e. a diff of the diffs. That will
> help me carry on where you left off.

Sure, here are the changes between the first patch you reviewed and the current patch.

    
    

    
  
Comment on attachment 821267 [details] [diff] [review]
edit-html-browser-3.patch

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

::: browser/devtools/markupview/markup-view.js
@@ +620,5 @@
> +      def.resolve(-1);
> +    } else {
> +      this.walker.children(parentNode).then(children => {
> +        let index = -1;
> +        children.nodes.forEach((node, i) => {

let index = [].indexOf.call(children.nodes, aNode);

Might be clearer? I'm not fussed, so don't hold this up for that change.

        Attachment #821267 -
        Flags: review?(jwalker) → review+

    
    

    
  
(In reply to Joe Walker [:jwalker] from comment #28)
> let index = [].indexOf.call(children.nodes, aNode);
> 
> Might be clearer?

Array.indexOf(children.nodes, aNode);

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #29)
> (In reply to Joe Walker [:jwalker] from comment #28)
> > let index = [].indexOf.call(children.nodes, aNode);
> > 
> > Might be clearer?
> 
> Array.indexOf(children.nodes, aNode);

Though if children.nodes is an array (as the use of children.nodes.forEach seems to indicate), you can of course just call children.nodes.indexOf.

    
    

    
  
How does this behave will inspector XUL nodes (about:addons) or SVG nodes?

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > (In reply to Joe Walker [:jwalker] from comment #28)
> > > let index = [].indexOf.call(children.nodes, aNode);
> > > 
> > > Might be clearer?
> > 
> > Array.indexOf(children.nodes, aNode);

We generally try to write standards based JS, hence the original suggestion.

> Though if children.nodes is an array (as the use of children.nodes.forEach
> seems to indicate), you can of course just call children.nodes.indexOf.

Ah - I'd assumed nodes was a NodeList, but you're right, as an array you should be able to just use indexOf.
Still don't hold things up for this though.

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > (In reply to Joe Walker [:jwalker] from comment #28)
> > > let index = [].indexOf.call(children.nodes, aNode);
> > > 
> > > Might be clearer?
> > 
> > Array.indexOf(children.nodes, aNode);
> 
> Though if children.nodes is an array (as the use of children.nodes.forEach
> seems to indicate), you can of course just call children.nodes.indexOf.

Good catch - when I first wrote it I was doing some other processing with the array. I will update to indexOf.

    
    

    
  
(In reply to Paul Rouget [:paul] from comment #31)
> How does this behave will inspector XUL nodes (about:addons) or SVG nodes?

With XUL nodes fine, it works with the weird thing that adds xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" attribute to the element (the same thing is copied to your clipboard if you right click and copy a nodes outer HTML.

<richlistitem xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="addon addon-view" name="Firefox OS 1.2 Simulator" type="extension" remote="false" status="installed" value="fxos_1_2_simulator@mozilla.org" contextmenu="addonitem-popup" active="true" selected="true" current="true"/>

For SVG, I haven't done extensive testing, but you can make updates to https://upload.wikimedia.org/wikipedia/commons/f/fd/Ghostscript_Tiger.svg, for instance.

    
    

    
  


  
Rebased patch. Using indexOf as discussed.

        Attachment #821267 -
        Attachment is obsolete: true

        Attachment #821676 -
        Flags: review+

    
    

    
  
When checking this in, please apply edit-html-toolkit-2.patch first, followed by edit-html-browser-4.patch.
Keywords: checkin-needed

    
  
Blocks: 930588
Status: NEW → ASSIGNED

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9c44473817b5
https://hg.mozilla.org/mozilla-central/rev/8693c78697ae
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: