Status

enhancement
P5
normal
8 years ago
a month ago

People

(Reporter: bj, Unassigned)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Allow an easy way to view the DOM for a node with a DOM button. The new sidebar would display the same information currently shown by: <highlight><control-shift-K>inspect($0)
(Reporter)

Updated

8 years ago
Blocks: 63830
You mean a JavaScript object view? Doesn't have to be a only-DOM view.
And how is that related to bug 63830?
(Reporter)

Comment 2

8 years ago
Yes, I think it would be useful to show any properties of the object the highlighter points to.

(Sorry about the block -- select error trying to copy the bug number of the Highlighter meta bug.)
Blocks: 663830
No longer blocks: 63830
So the plan is to move the JS Object inspector from its own window into the Web Console (bug 704180).
(In reply to Paul Rouget [:paul] from comment #3)
> So the plan is to move the JS Object inspector from its own window into the
> Web Console (bug 704180).

for web console inspected objects. I don't think that applies to this bug.

Maybe this should be next to the HTML view?
(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> (In reply to Paul Rouget [:paul] from comment #3)
> > So the plan is to move the JS Object inspector from its own window into the
> > Web Console (bug 704180).
> 
> for web console inspected objects. I don't think that applies to this bug.
> 
> Maybe this should be next to the HTML view?

So a button next to "style", something like "JS Object", that would open the Web Console and show the javascript-obj-view? Or you think we should open it in the sidebar (which would be weird, because the same tool could be display in the sidebar and in the webconsole)?
Well, actually, opening the js-obj-view in the Console if opened from the Console, and in the sidebar if opened from the highlighter makes sense.
(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> Maybe this should be next to the HTML view?

You mean splitting the HTML view in two?
I think it would be better to have it in the sidebar.
except the sidebar is labeled "Style" currently. I also think, since we have a grouping of style tools in the sidebar, it might make more sense to put a "DOM Object" viewer into the same panel as the HTML tree. Or we could pop it up separately as we do now as long as we don't get into more xul panel bugs.

One of the reasons we didn't include an Object button in the Highlighter was because I was selling "Annotations" as a possible way to get information about the object into the Highlighter view. Some people deemed the Object button as "too fringe", though I kind of disagreed at the time.

There is a workaround available if you open the Web Console and type $0 or inspect($0) to open the property viewer directly.
(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> except the sidebar is labeled "Style" currently. I also think, since we have
> a grouping of style tools in the sidebar, it might make more sense to put a
> "DOM Object" viewer into the same panel as the HTML tree. Or we could pop it
> up separately as we do now as long as we don't get into more xul panel bugs.

This tool doesn't have a name yet, but I don't think it's a DOM-only tool. It's a bit more general (we can inspect the whole JS structure, not just the DOM methods/properties). I would call it JS Object View.

This JS Object View can be open in the Web Console (`inspect(window)`). This will happen in bug 704180.

Sounds like we want to have a button that will inspect the node too (this bug).

We can simply open this tool in the Web Console since we will get inline-inspect working. But:
a) from the inspector, the user probably doesn't want to have the whole Web Console;
b) this view would fit better in a vertical view.

So we need to find a better slot for it.

If I'm not mistaken... if we would have had the modular sidebar (from where we could dock/undock tools, and maybe stack tools, with an accordion), we would just add it there. But we don't have this modular sidebar. For now, we have a style-specific sidebar.

We could move it into the HTML Panel. But:
a) It's a horizontal view;
b) We will have to move it the sidebar once it will get the modular sidebar.

I see 3 options:
a) We open the Web Console when the user click on the button. It's horizontal, but at least, we won't have to "kill" the feature when we will be able to host the JS Object view into the future modular sidebar;
b) We hide the style sidebar, and create a JS sidebar;
c) We start working on the modular sidebar now.

Updated

7 years ago
Duplicate of this bug: 719867
triage, filter on centaur
Priority: -- → P2
We want to move the buttons inside the sidebar. See bug 717929.
Summary: [highlighter] Add DOM button next to HTML and Style button → Add a DOM View to the inspector
(P3, as discussed with Kevin)
Priority: P2 → P3

Updated

7 years ago
Blocks: 782593
The Toolbox + Sidebar.jsm makes things much easier.

Bug triage, filter on PINKISBEAUTIFUL
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 817547
Sorry sorry, wrong bug!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Updated

7 years ago
Duplicate of this bug: 801124

Updated

7 years ago
Duplicate of this bug: 796063

Updated

7 years ago
Duplicate of this bug: 787470
Comment hidden (spam)
Probably the most irritating thing for most Firebug users is that we have no DOM tab. In Firebug I can easily inspect an object and click on the DOM tab (In fact, I can use the inspector whilst the DOM tab is open, which makes it very easy to select nodes.

Firebug also allows me to right-click an HTML node  and "Inspect in DOM panel." In our native tools I have to open a scratchpad or the web console, use a selector to get a node and then click Scratchpad's "Inspect" button. This is hardly discoverable nor as simple to use as Firebug's DOM panel.
The VariablesView is a pretty self-contained widget now, so using it inside the inspector shouldn't be too hard.
I will try to take a look at this one. It would indeed be a pretty good addition to our tools.
I've looked at the code a bit and the only unclear part that I'll look into now is how to get an objectActor grip for a dom node in the inspector.
Other than that, it should be pretty straight forward with the VariablesView.

The other thing is we have 4 tabs in the sidepanel already, this would be a 5th! We might need some re-thinking here.
@darrin: thoughts on this?
Assignee: nobody → pbrosset
Flags: needinfo?(dhenein)
The obvious thing would be to show the DOM panel where the Fonts tab is and to have an ellipse button to show less useful tools (e.g. Fonts). We need something like that anyway as the side panel pushes tabs off to the right when it is too narrow.

Darrin may have something better though.
When it comes to actually retrieving properties from the currently selected node, I'm wondering what's the best course of action.

On one side we have the DOM view that will live in the inspector, where we have a reference to the currently selected NodeFront.
And on the other side, we have the VariableView widget that works well with ObjectActor's grips.

We could:

(a) implement a new WalkerActor method that, based on the passed node, would send back the grip. However, this requires to have a ThreadActor (see http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2832) which the Walker does not have.
(b) alternatively, the new WalkerActor method could simply send a new response packet containing the list of key/value pairs for all of the node's properties. This way, we wouldn't use the VariableView's ability to work with grips at all.
(c) or find a way to call the webconsole's server which does the job already (i.e. when you click on the output link for a DOM node).

I'm leaning towards (a) but haven't yet been able to figure out how to make it happen.
Flags: needinfo?(past)
Flags: needinfo?(mihai.sucan)
(c) isn't very practical as it requires a "evaluateJS" packet type which takes a string input whereas we have a DOM node front here.
Mihai can elaborate more on this, but you don't need much machinery to use the debugger APIs with object actors. Unfortunately this is not well documented, but see how WebConsoleActor creates ObjectActors internally (pretending to be a ThreadActor by mocking _gripDepth and createValueGrip mostly). If your actor implements the right methods, the code that will give you the grip should roughly be:

let grip = actor.createValueGrip(actor.makeDebuggeeValue(domObject));
Flags: needinfo?(past)
Thanks Panos for the help.
I've managed to implement the premise of solution (a). I now have problems related to how the VariablesView widget fetches properties, but this seems related to the actorpool which I need to maintain on my side.

No need to needinfo Mihai anymore for now. I'll probably come back with more questions later though :)
Flags: needinfo?(mihai.sucan)
Patrick, I believe solution (a) is the one you should continue work on, as you are already doing.

As Panos pointed out, you only need _gripDepth and createValueGrip(). For createValueGrip() I maintain my own actor pool, because the lifetime of object actors is different in the webconsole, when compared to debugging. During debugging you have thread-lifetime and pause-lifetime actors.

It would be nice if you could reuse the console actor, to avoid code duplication.
Dave, do you have suggestions as to how I could reuse the WebConsoleActor's ability to retrieve object actor grips from the inspector actor?

It seems to me that the inspector, being a protocol.js type actor, does not have access to the globally loaded actors.
Flags: needinfo?(dcamp)
Status update: I've managed to get the DOM view sidebar working but it's a bit of a hack right now.

It works by acting as a ThreadActor, just like the WebConsoleActor does, and in doing so, can easily create ObjectActorGrips that have the thread lifetime instead.

The reason it's hacky right now is that from the WalkerActor I need access to 2 classes defined in script.js: ThreadActor and ObjectActor. Although the WebConsoleActor has access to these classes from the global scope, the WalkerActor does not.
I think you are right, protocol.js-based actors are loaded in separate globals, so perhaps it's time we split the common bits of script.js into separate objects that can be used by both old-style actors (script.js, webconsole.js) and modern ones, like inspector.js. Moving the ObjectActor to an object.js module sounds straightforward and a good place to start.
Darrin, here's a work in progress to add an ellipsis to the sidebar tabbox in case there's not enough room: http://www.youtube.com/watch?v=K2EHMZK9ybU
As you suggested, it would be nice to have an icon for this.
Flags: needinfo?(dhenein)
Flags: needinfo?(dhenein)
Attaching my current patches for this bug.
They're not ready for review just yet, but I'm getting there.

Here's a status of the progress:

part 1:

- New "DOM" sidebar panel in the inspector.
- Reacts as the other panels do: refresh on new selection, empties if no node selected.
- Also reloads on node mutation.
- Uses telemetry.
- Is localized.
- Uses the VariablesView widget to display the properties.
- Goes to the server to translate the NodeFront into an ObjectActorGrip for the VariablesView to work (it does this similarly to how the WebConsole does it: that is by playing the role of the ThreadActor, by overriding some of its methods and managing its own actor pool).
This part isn't very very nice, but it turns out protocol.js actors aren't loaded in the server's global scope, so they don't have access to all actors by default. So what I did was split script.js in several files as I could. For instance, ObjectActor is now in a separate module that can be required by other actors. But I wasn't able to split the ThreadActor in a separate module as it requires simply too many dependencies.

part 2:

- Adds a new option to the ToolSidebar widget that shows an allTabs menu to the right when there's not enough space to let all tabs be visible. This menu contains a drop-down of all tabs.
- Makes the netmonitor also use ToolSidebar so it has the same feature. For this, I had to improve again ToolSidebar in a way that it now also works with existing <tab> elements.

part 3:

- Adds a simple test. I need to keep working on it and add new tests for the inspector and netmonitor sidebars.
Comment on attachment 8363914 [details] [diff] [review]
bug704094-inspector-domview-part1-Adds-the-sidebar.patch

Mihai, getting a review on the part that returns actor grips would be awesome since you did this for the webconsole.
Attachment #8363914 - Flags: review?(mihai.sucan)
Comment on attachment 8363914 [details] [diff] [review]
bug704094-inspector-domview-part1-Adds-the-sidebar.patch

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

Patch looks good and I'm really glad to see we will have a DOM panel in the inspector - finally. ;)

Do you plan to add editing of properties in this patch? If not, please file a follow-up bug - we should allow editing of properties, like the debugger and console do.

::: browser/devtools/dominspector/dominspector-panel.xul
@@ +9,5 @@
> +<?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/devtools/widgets.css" type="text/css"?>
> +<?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
> +
> +<xul:window xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

Why not make XUL the default namespace?

@@ +18,5 @@
> +  <script type="application/javascript;version=1.8"
> +          src="chrome://browser/content/devtools/theme-switching.js"/>
> +  <script type="application/javascript;version=1.8"
> +          src="chrome://browser/content/devtools/dominspector.js"/>
> +  <xul:script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>

Why do you mix <script> tags with and without the xul prefix?

@@ +24,5 @@
> +  <xul:box id="variable-view"
> +       class="devtools-responsive-container theme-body"
> +       flex="1"/>
> +
> +  <script type="application/javascript;version=1.8">

We have bug 960728 about removing all of the inlined scripts and styles. Please avoid adding this inlined script here.

::: browser/devtools/dominspector/dominspector.js
@@ +11,5 @@
> +Cu.import("resource:///modules/devtools/VariablesViewController.jsm");
> +Cu.import("resource://gre/modules/devtools/dbg-client.jsm");
> +Cu.import("resource://gre/modules/devtools/Loader.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +let promise = devtools.require("sdk/core/promise");

nit: you might want to lazy import/require modules you dont need when someone does require("dominspector").

Also, you might want to use Promise.jsm - the promise implementation that doesn't swallow exceptions.

@@ +13,5 @@
> +Cu.import("resource://gre/modules/devtools/Loader.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +let promise = devtools.require("sdk/core/promise");
> +
> +function DomInspector(inspector, rootEl) {

Please add jsdoc comments to functions/public methods.

::: toolkit/components/telemetry/Histograms.json
@@ +5071,5 @@
>    },
> +  "DEVTOOLS_DOMINSPECTOR_OPENED_BOOLEAN": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "How many times has the devtool's DOM Inspector been opened?"

This might be confusing. Which DOM inspector? The markup view is a type of a DOM inspector.

I would suggest: How many times has the DOM panel been opened in the Inspector devtool?

... better suggestions are welcome.

I see below you use this in more places. Why not call it the DOM panel? Even in the devtools folder "inspector" versus "dominspector" could be confusing. You could put the DOM panel in devtools/inspector/ - a dom-panel.js and .xul.

I dont know if we have any "policy", but I was under the impression that the top level devtools/ folder should be used for tools.

(these concerns do not block the review, just mentioning my thoughts - no strong opinions on this. If you feel inclined to do something, maybe you should ping paul/joe to ask.)

::: toolkit/devtools/server/actors/inspector.js
@@ +866,5 @@
>     */
>    initialize: function(conn, tabActor, options) {
>      protocol.Actor.prototype.initialize.call(this, conn);
> +
> +    this.conn.addActorPool(this);

Why do you do this here? All protocol.js Actors do this, if I correctly understand protocol.js.

@@ +926,5 @@
> +
> +    this.dbg.enabled = false;
> +    this.dbg = null;
> +
> +    this.conn.removeActorPool(this);

Same question as above.

@@ +1946,5 @@
> +   * widget
> +   */
> +  getActorGripForNode: method(function(node) {
> +    let dbgGlobal = this.dbg.makeGlobalObjectReference(this.rootWin);
> +    let value = dbgGlobal.makeDebuggeeValue(node.rawNode);

What happens if rawNode comes from an iframe? For the webconsole we needed to get the global from the rawNode, not assume rootWin. See makeDebuggeeValue() in the console actor.

@@ +1960,5 @@
> +
> +  // Making the walker act as a ThreadActor so it can create node value grips.
> +  // This is somewhat similar to what the webconsole actor does, except that
> +  // the ThreadActor class isn't globally available here.
> +  // FIXME: the ThreadActor should be moved to its own separate module so it

Is this something you want to do in this bug or do you want to file a follow-up? If the latter, please add the bug number in the comment.

::: toolkit/devtools/server/actors/object-stringifier.js
@@ +16,5 @@
> + *        The object to stringify.
> + * @return String
> + *         The stringification for the object.
> + */
> +exports.stringify = function(obj) {

Did you make any changes in the code you moved from script.js into this file? If yes, do you have a diff?

::: toolkit/devtools/server/actors/object.js
@@ +8,5 @@
> +
> +const {Cu} = require("chrome");
> +const {DebuggerServer} = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {});
> +const {DevToolsUtils} = Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm", {});
> +const {stringify} = require("devtools/server/actors/object-stringifier");

Why do you move out the stringifiers from script.js into their own file? They seem to be only used by the ObjectActor (object.js). Also, they are deprecated until bug 952197 is fixed - we now have the object previewers (which you can find in script.js).

@@ +18,5 @@
> + *        The debuggee object.
> + * @param aThreadActor ThreadActor
> + *        The parent thread actor for this object.
> + */
> +function ObjectActor(aObj, aThreadActor) {

Did you make any changes in the code you moved from script.js into this file? If yes, do you have a diff?

::: toolkit/devtools/server/actors/webconsole.js
@@ -260,5 @@
>  
>    _createValueGrip: ThreadActor.prototype.createValueGrip,
>    _stringIsLong: ThreadActor.prototype._stringIsLong,
> -  _findProtoChain: ThreadActor.prototype._findProtoChain,
> -  _removeFromProtoChain: ThreadActor.prototype._removeFromProtoChain,

Please also remove the references to _protoChains.
Attachment #8363914 - Flags: review?(mihai.sucan)
(In reply to Patrick Brosset [:pbrosset] from comment #37)
> Comment on attachment 8363914 [details] [diff] [review]
> bug704094-inspector-domview-part1-Adds-the-sidebar.patch
> 
> Mihai, getting a review on the part that returns actor grips would be
> awesome since you did this for the webconsole.

That part seems fine. Is the threadActor always available on the tabActor? I understand these are wip patches, so dont worry about all the comments, I noted them so we dont forget things with the next update.

Updated

5 years ago
Flags: needinfo?(dcamp)
See Also: → 992679

Updated

5 years ago
Duplicate of this bug: 1066026
Flags: needinfo?(dhenein)
NIing Mike on this old bug which I never finished ...
Getting this landed would be great! For info, I also started bug 1101569 to make the sidebar easier to use when there are many tabs. So attachment 8363915 [details] [diff] [review] should be ignored here.
Flags: needinfo?(mratcliffe)
I don't have time to work on this at the moment. I would prefer a sidebar than our current context menu implementation because I think it would be more discoverable and useable.
Flags: needinfo?(mratcliffe)

Updated

4 years ago
Attachment #8363915 - Attachment is obsolete: true
Depends on: 1201475

Updated

3 years ago
Assignee: pbrosset → nobody
Status: REOPENED → NEW
Duplicate of this bug: 1306160

Updated

3 years ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Duplicate of this bug: 1320351
Resetting assignee due to lack of activity.
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW
Comment hidden (advocacy)

Comment 47

11 months ago
I have a colleague that still uses an ancient build of Firefox so they can us Firebug and the dom properties panel.  In Fx, currently, the right-click, "show dom properties" gets kind of close. In Firebug, that panel was on the right in a tabbed interface, and in Fx, it's in a panel below (using the console), but that's a minor issue.  The real issue is that I cannot seem to edit the dom properties in the "inspect($0)" anymore.  I think I used to be able to in Fx, but I know I was able to in Firebug DOM Properties.

Fx dev tools is better than Firebug in most ways, now, but this seems a major oversight to me.  It was one of the best features of Firebug.

Updated

10 months ago
Product: Firefox → DevTools
Duplicate of this bug: 1491877

Comment 49

2 months ago

What is the status of this bug? I see that at comment 38 a patch was reviewed (it was 5 years ago), than after, some patch was marked obsolete and there is no more acrivities.

This bug is paused because the feature isn't deemed high priority enough by the team at Mozilla working on devtools, and its community of volunteers.
The patch that was done a number of years ago will likely not apply anymore and we would have to start from scratch again.
The right-click/show dom properties is the best alternative to this feature if you do need it.
At this stage, I don't expect activity on this bug unless someone had time to commit to it. And even then, the extra sidebar would still cause usability problems as the inspector's tabbar isn't easily navigable.

Martin: would you say we're actively trying to avoid adding more sidebar panels, or would we land the patch if someone did spend the time to create it?
Just trying to figure out if we should WONTFIX this or not.

Flags: needinfo?(mbalfanz)

As discussed with Martin, this isn't a priority right now for the mozilla team working on devtools. However we'll happily accept patches if people want to fix this.
Note however that I've made this depend on bug 1534982 because we ideally want to fix the tabbar situation first.

Depends on: 1534982
Flags: needinfo?(mbalfanz)
Priority: P3 → P5
You need to log in before you can comment on or make changes to this bug.