Closed Bug 840247 Opened 11 years ago Closed 11 years ago

Entering the Inspector from the Web Console should show the <html> and <body> tag expanded

Categories

(DevTools :: Inspector, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: jaws, Assigned: paul)

Details

(Keywords: polish, Whiteboard: [good first bug][mentor=jaws][lang=js])

Attachments

(1 file, 8 obsolete files)

patch v8
7.86 KB, patch
Details | Diff | Splinter Review 
I usually enter the Inspector by hitting the keyboard shortcut for the Web Console then hitting the Inspector button. This shows the <html> element collapsed, which is pretty useless.

I then have to expand the <html> element to get to what I was looking for.

I'd posit that > 90% of the time, developers are looking to work on something within the body tag. We should just expand the <html> and <body> tags by default in this scenario.
In this file:

https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/InspectorPanel.jsm

...there are 3 references to `.documentElement`. This is where we initialize the inspector.
Before using `.documentElement` (which is <html> in the case of a HTML document), we can try to access `.body` (it's not always present).

(can be a good-first-bug)
Whiteboard: [good first bug][mentor=jaws][lang=js]
Blocks: DevToolsPaperCuts
Hi,

I'll take the bug if that's OK.
Assignee: nobody → jlmendezbonini
Status: NEW → ASSIGNED
Assignee: jlmendezbonini → nobody
Status: ASSIGNED → NEW
Hi

I would like to work on this bug.

Thanks
Sinduja
Assignee: nobody → sinduja_psg
Status: NEW → ASSIGNED
I am not sure how to add test case for this fix. Any info will be helpful.
Attachment #719140 - Flags: review?(paul)
Comment on attachment 719140 [details] [diff] [review]
Fix for this issue. When inspector is opened, body node will be selected if available

It looks like a good start. Thank you :)

Pass the document instead of the root to your new function, and check if `document.body` is present (no need to use getElementsByTagName).

For the test, look at the /tests/ directory. Find a file you think could be a good place to add a test, or add your own
file (change the makefile as well).

To run a test:

$ TEST_PATH=browser/devtools/inspector/tests/mytest.js make mochitest-browser-chrome

For the next round, please ask ":jwalker" to review your patch.

Come to IRC (#devtools) if you have any question.
Attachment #719140 - Flags: review?(paul)
I have done the code changes. But some test cases are failing and I am correcting them. It is taking me some time as I am also busy with my office work. Will send the patch soon.
(In reply to sinduja ramaraj from comment #6)
> I have done the code changes. But some test cases are failing and I am
> correcting them. It is taking me some time as I am also busy with my office
> work. Will send the patch soon.

No hurry. Thank you for taking care of that :)
Attached patch Fix with test cases corrected (obsolete) — — Splinter Review 
Review comments implemented and corrected test cases affected by this change.
Attachment #719140 - Attachment is obsolete: true
Attachment #725016 - Flags: review?(paul)
Attachment #725016 - Flags: review?(jwalker)
Comment on attachment 725016 [details] [diff] [review]
Fix with test cases corrected

Looks good to me. I'll land that soon.
Attachment #725016 - Flags: review?(paul)
Attachment #725016 - Flags: review?(jwalker)
Attachment #725016 - Flags: review+
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=jaws][lang=js][land-in-fx-team]
If I apply this patch then body does not auto expand for me on Linux.
Let's figure out what's going on.
Whiteboard: [good first bug][mentor=jaws][lang=js][land-in-fx-team] → [good first bug][mentor=jaws][lang=js]
this will need a rebase
(In reply to Paul Rouget [:paul] from comment #12)
> this will need a rebase

I've added it to my list of bugs to consider
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> If I apply this patch then body does not auto expand for me on Linux.

oh wait, auto-expand? This patch just auto-selects. Not auto-expand.

We can land this patch and file a bug for auto-expand, or add the auto-expand in this bug.

Sinduja, what do you think?
I think we should make auto-expand part of this bug. I don't think auto-select by itself is enough of an improvement.
I am with Jared on this one, auto expand would be best.
Comment on attachment 725016 [details] [diff] [review]
Fix with test cases corrected

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

::: browser/devtools/inspector/test/browser_inspector_bug_566084_location_changed.js
@@ +107,1 @@
>      is(inspector.selection.node, root, "Selection is the root of the new page.");

nit, the variable name and testcase description here should be updated.
Okie. Will correct it.
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #17)
> Comment on attachment 725016 [details] [diff] [review]
> Fix with test cases corrected
> 
> Review of attachment 725016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/devtools/inspector/test/
> browser_inspector_bug_566084_location_changed.js
> @@ +107,1 @@
> >      is(inspector.selection.node, root, "Selection is the root of the new page.");
> 
> nit, the variable name and testcase description here should be updated.
Can you please explain auto-expand?
How is it different from auto-select?

(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #15)
> I think we should make auto-expand part of this bug. I don't think
> auto-select by itself is enough of an improvement.
auto-select means to select the node automatically, which the attached patch does.

auto-expand includes auto-select, but also expands the node in the Inspector's DOM tree view to show the first-level children of the node.
Thanks! Let me work on it and send a new patch.
Attached patch Expand body tag default (obsolete) — — Splinter Review 
Patch for expanding body node by default
Attachment #725016 - Attachment is obsolete: true
Attachment #738434 - Flags: review?(paul)
Attachment #738434 - Flags: review?(jwalker)
Comment on attachment 738434 [details] [diff] [review]
Expand body tag default

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

Thanks for the updated patch.
We need to fix a bunch of style nits, please:
- please don't add trailing spaces
- our indents are 2 spaces
- max line length = 80 chars

I'll review the updated patch, and push it to try if you'd like. Paul is away for several more weeks and won't be able to review.
Attachment #738434 - Flags: review?(paul)
Attachment #738434 - Flags: review?(jwalker)
Attached patch Patch with code style corrected (obsolete) — — Splinter Review 
Implemented style changes
Attachment #738434 - Attachment is obsolete: true
Attachment #739538 - Flags: review?(paul)
Attachment #739538 - Flags: review?(jwalker)
Comment on attachment 739538 [details] [diff] [review]
Patch with code style corrected

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

Thanks for the update.

So the trailing spaces thing probably sounds really picky, but it's not really - If you don't have a policy of 'no trailing spaces' then you end up with files with all sorts of trailing spaces, which makes it tricky to make changes without making unintentional whitespace changes, and you then discover that your patches are harder to read than they need to be.

In fact our review tool points the trailing spaces out for this reason, and there are still some there. See https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=840247&attachment=739538 and click on 'InspectorPanel.jsm'

::: browser/devtools/inspector/InspectorPanel.jsm
@@ +281,5 @@
>        self._initMarkup();
> +      
> +      self.once("markuploaded", function() {
> +        this.markup.expandNode(this.selection.node);
> +      }.bind(self));

It's a bit funny to be using both bind and self. Generally we prefer bind in Mozilla code, but since this function is already using self, lets stick with that.
Attachment #739538 - Flags: review?(paul)
Attachment #739538 - Flags: review?(jwalker)
(In reply to Joe Walker [:jwalker] from comment #25)
> 
> It's a bit funny to be using both bind and self. Generally we prefer bind in
> Mozilla code, but since this function is already using self, lets stick with
> that.

Why not switch to arrow functions? :P
Attached patch Corrected Space (obsolete) — — Splinter Review 

        Attachment #740016 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Corrected Spaces (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry for missing the spaces.

        Attachment #739538 -
        Attachment is obsolete: true

        Attachment #740016 -
        Attachment is obsolete: true

        Attachment #740016 -
        Flags: review?(jwalker)

        Attachment #740017 -
        Flags: review?(paul)

        Attachment #740017 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v6 (obsolete)
        — 
          — Splinter Review
      

    


  
I did the tweak to remove 'self' that I mentioned, and made the change to use fat arrows that Victor mentioned.
I also rebased the patch against the new locations for the inspector files.

One thing that is still remaining is fixing the unit tests broken by this change, which you can see from this try run:

https://tbpl.mozilla.org/?tree=Try&rev=b06887c65cfa

Do you know how to run our test suite?

https://developer.mozilla.org/en-US/docs/Running_automated_tests

        Attachment #740017 -
        Attachment is obsolete: true

        Attachment #740017 -
        Flags: review?(paul)

        Attachment #740017 -
        Flags: review?(jwalker)

    
    

    
  
Repushing to try as the previous result are not available anymore:
https://tbpl.mozilla.org/?tree=Try&rev=3ab3ca2ca960

    
    

    
  
I'll take it from here. I'll fix the test and land it.
Assignee: sinduja_psg → paul

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  
2 files have changed:

browser/devtools/markupview/test/browser_inspector_markup_edit.js
browser/devtools/markupview/test/browser_inspector_markup_navigation.js

        Attachment #746642 -
        Attachment is obsolete: true

        Attachment #760426 -
        Flags: review?(jwalker)

    
    

    
  
Comment on attachment 760426 [details] [diff] [review]
Patch v7

still one orange

        Attachment #760426 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v8
        — 
          — Splinter Review
      

    


  
3 files have changed:

browser/devtools/markupview/test/browser_inspector_markup_edit.js
browser/devtools/markupview/test/browser_inspector_markup_navigation.js
browser/devtools/styleinspector/test/browser_ruleview_focus.js

        Attachment #760426 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 760938 [details] [diff] [review]
patch v8

Green.

    
  
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=jaws][lang=js][land-in-fx-team]

    
    

    
  
https://hg.mozilla.org/integration/fx-team/rev/5ff142a19263
Whiteboard: [good first bug][mentor=jaws][lang=js][land-in-fx-team] → [good first bug][mentor=jaws][lang=js][fixed-in-fx-team]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5ff142a19263
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jaws][lang=js][fixed-in-fx-team] → [good first bug][mentor=jaws][lang=js]
Target Milestone: --- → Firefox 24
No longer blocks: DevToolsPaperCuts

    
  
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: