Closed
Bug 840247
Opened 12 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)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: jaws, Assigned: paul)
Details
(Keywords: polish, Whiteboard: [good first bug][mentor=jaws][lang=js])
Attachments
(1 file, 8 obsolete files)
(deleted),
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.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Whiteboard: [good first bug][mentor=jaws][lang=js]
Assignee | ||
Updated•12 years ago
|
Blocks: DevToolsPaperCuts
Comment 2•12 years ago
|
||
Hi, I'll take the bug if that's OK.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → jlmendezbonini
Status: NEW → ASSIGNED
Updated•12 years ago
|
Assignee: jlmendezbonini → nobody
Reporter | ||
Updated•12 years ago
|
Status: ASSIGNED → NEW
Comment 3•12 years ago
|
||
Hi I would like to work on this bug. Thanks Sinduja
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → sinduja_psg
Status: NEW → ASSIGNED
Comment 4•12 years ago
|
||
I am not sure how to add test case for this fix. Any info will be helpful.
Attachment #719140 -
Flags: review?(paul)
Assignee | ||
Comment 5•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
(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 :)
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=jaws][lang=js][land-in-fx-team]
Comment 10•12 years ago
|
||
If I apply this patch then body does not auto expand for me on Linux.
Assignee | ||
Comment 11•12 years ago
|
||
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]
Assignee | ||
Comment 12•12 years ago
|
||
this will need a rebase
Comment 13•12 years ago
|
||
(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
Assignee | ||
Comment 14•12 years ago
|
||
(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?
Reporter | ||
Comment 15•12 years ago
|
||
I think we should make auto-expand part of this bug. I don't think auto-select by itself is enough of an improvement.
Comment 16•12 years ago
|
||
I am with Jared on this one, auto expand would be best.
Reporter | ||
Comment 17•12 years ago
|
||
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.
Comment 18•12 years ago
|
||
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.
Comment 19•12 years ago
|
||
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.
Reporter | ||
Comment 20•12 years ago
|
||
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.
Comment 21•12 years ago
|
||
Thanks! Let me work on it and send a new patch.
Comment 22•12 years ago
|
||
Patch for expanding body node by default
Attachment #725016 -
Attachment is obsolete: true
Attachment #738434 -
Flags: review?(paul)
Attachment #738434 -
Flags: review?(jwalker)
Comment 23•12 years ago
|
||
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)
Comment 24•12 years ago
|
||
Implemented style changes
Attachment #738434 -
Attachment is obsolete: true
Attachment #739538 -
Flags: review?(paul)
Attachment #739538 -
Flags: review?(jwalker)
Comment 25•12 years ago
|
||
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)
Comment 26•12 years ago
|
||
(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
Comment 27•12 years ago
|
||
Attachment #740016 -
Flags: review?(jwalker)
Comment 28•12 years ago
|
||
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)
Comment 29•11 years ago
|
||
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)
Assignee | ||
Comment 30•11 years ago
|
||
Repushing to try as the previous result are not available anymore: https://tbpl.mozilla.org/?tree=Try&rev=3ab3ca2ca960
Assignee | ||
Comment 31•11 years ago
|
||
I'll take it from here. I'll fix the test and land it.
Assignee: sinduja_psg → paul
Assignee | ||
Comment 32•11 years ago
|
||
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)
Assignee | ||
Comment 33•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1a8a8ed530bb
Assignee | ||
Comment 34•11 years ago
|
||
Comment on attachment 760426 [details] [diff] [review] Patch v7 still one orange
Attachment #760426 -
Flags: review?(jwalker)
Assignee | ||
Comment 35•11 years ago
|
||
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
Assignee | ||
Comment 36•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0ff12dcba4fc
Assignee | ||
Comment 37•11 years ago
|
||
Comment on attachment 760938 [details] [diff] [review] patch v8 Green.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=jaws][lang=js][land-in-fx-team]
Comment 38•11 years ago
|
||
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]
Comment 39•11 years ago
|
||
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
Updated•11 years ago
|
No longer blocks: DevToolsPaperCuts
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•