Last Comment Bug 696755 - automatically open the inspector sidebar on inspect if previously-active
: automatically open the inspector sidebar on inspect if previously-active
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Dave Camp (:dcamp)
:
Mentors:
: 590536 705243 714400 (view as bug list)
Depends on: 719607
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-24 07:31 PDT by Luke Crouch [:groovecoder]
Modified: 2012-02-27 01:20 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reduced patch demonstrating Style Inspector initialization problem (1.48 KB, patch)
2011-12-06 10:51 PST, Heather Arthur [:harth]
no flags Details | Diff | Review
fix v1 (19.52 KB, patch)
2012-01-28 13:39 PST, Dave Camp (:dcamp)
no flags Details | Diff | Review
fix v2 (15.07 KB, patch)
2012-01-28 14:48 PST, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Review

Description Luke Crouch [:groovecoder] 2011-10-24 07:31:25 PDT
When I inspect an element, I want it to automatically open to the HTML panel.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-24 23:46:39 PDT
The best solution is probably to remember the open / closed state of all registered tools.
Comment 2 Luke Crouch [:groovecoder] 2011-10-25 06:33:23 PDT
Totally agree. I want to hit a key combination (cmd+shift+i is fine) and switch to "development mode" - open the console, inspector, and/or scratchpad depending if I had them open last time.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-10-25 11:07:52 PDT
I'm not sure this will be true for everyone.

The Web Console and Scratchpad are currently independent of the other tools. This will change somewhat in the near future, however and it will make sense to show them all when we have the global toolbar. (bug needed)

For the highlighter specifically, I'm not sure everyone will always want the HTML panel open or the Style Inspector or (insert tool here). They're intended as task-specific tools. With the addition of Breadcrumbs (bug 672006), I don't think the HTML panel will be needed for all tree navigation.

Anyway, it's a good bug to have. We should probably start a discussion on dev.apps.firefox to try to figure this out.
Comment 4 Kevin Dangoor 2011-10-25 11:26:32 PDT
The breadcrumbs will certainly change the dynamic here. The addition of the developer toolbar may change the dynamic again.

Rob is right that the general idea here is to open the tools you need for the task at hand. It *may* be the case that people will want the HTML panel to always be displayed when they open the highlighter, but it would be good to see what people think of the breadcrumbs first.
Comment 5 Paul Rouget [:paul] 2011-10-26 09:15:21 PDT
We should make the state of this panel persistent. We should do that in bug 689946.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-11-02 05:43:49 PDT
I'd rather not conflate that bug with this one. This is about opening the panels, the HTML size bug should be about persisting the height of that and maybe the width of the sidebar when it lands.
Comment 7 Heather Arthur [:harth] 2011-11-28 15:55:16 PST
*** Bug 705243 has been marked as a duplicate of this bug. ***
Comment 8 Heather Arthur [:harth] 2011-12-06 10:51:15 PST
Created attachment 579375 [details] [diff] [review]
Reduced patch demonstrating Style Inspector initialization problem

Here's a WIP. Whenever the highlighter is "restored" (on first open, switching back after being on another tab), the tools are reopened; this just hard codes a set of tools to open on first load. It works for the Rule View and HTML panel, but gets an error for the Style Inspector:

Error: element is null
Source File: resource:///modules/devtools/CssLogic.jsm
Line: 689

Only when using the context menu "Inspect Element" because that doesn't give the Style Inspector enough time to initialize before inspecting a particular element.

Waiting for the "StyleInspector-opened" event fixes this problem, but we need more general initialization handling if this is something that could affect other highlighter-based tools.
Comment 9 Paul Rouget [:paul] 2011-12-20 10:10:46 PST
(In reply to Heather Arthur [:harth] from comment #8)
> Waiting for the "StyleInspector-opened" event fixes this problem, but we
> need more general initialization handling if this is something that could
> affect other highlighter-based tools.

Can you give a bit more details about this problem? Maybe open a new bug.
Comment 10 Heather Arthur [:harth] 2011-12-20 10:27:29 PST
(In reply to Paul Rouget [:paul] from comment #9)
> (In reply to Heather Arthur [:harth] from comment #8)
> > Waiting for the "StyleInspector-opened" event fixes this problem, but we
> > need more general initialization handling if this is something that could
> > affect other highlighter-based tools.
> 
> Can you give a bit more details about this problem? Maybe open a new bug.

Dcamp looked into this and figured out the problem. He's going to rewrite some of this code to make the Properties view initialization better.
Comment 11 Dave Camp (:dcamp) 2012-01-04 14:38:52 PST
(In reply to Heather Arthur [:harth] from comment #10)

> Dcamp looked into this and figured out the problem. He's going to rewrite
> some of this code to make the Properties view initialization better.

To elaborate a bit, registered tools provide an isOpen() method, which for style sidebar tools is implemented as "(My iframe is loaded and initialized) && (I am the currently-active sidebar tool, and the sidebar is showing)"

The style inspector protects itself from being updated while !isOpen by queuing the node to be highlighted until it has been opened.  If that were working, this bug wouldn't happen.  But the highlighter itself doesn't even bother to update the panels if they aren't open.  So the queuing doesn't happen.

isOpen conflating "active panel" and "loaded and initialized" makes this all more confusing than it needs to be.  I started to separate that out a bit.
Comment 12 Paul Rouget [:paul] 2012-01-07 14:02:44 PST
> It works for the Rule View and HTML panel, but gets an error for the Style Inspector.
Can we get a first patch that implements only the HTML panel persistence?
Comment 13 Paul Rouget [:paul] 2012-01-10 08:18:06 PST
*** Bug 705947 has been marked as a duplicate of this bug. ***
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-11 02:57:18 PST
*** Bug 590536 has been marked as a duplicate of this bug. ***
Comment 15 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-11 05:37:34 PST
*** Bug 714400 has been marked as a duplicate of this bug. ***
Comment 16 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-11 05:38:37 PST
Bug triage, filter on PEGASUS.
Comment 17 Anthony Ricaud (:rik) 2012-01-27 07:01:36 PST
Should we open another bug for Style Inspector? This is the panel that I'm most interested to see opened by default.
Comment 18 Paul Rouget [:paul] 2012-01-27 07:12:49 PST
If I understand correctly:
- persistence of the HTML Tree state is easy.
- persistence of the Sidebar state is hard.

The Sidebar problem should not block the HTML Tree persistence.
I am opening a different bug just for the Style sidebar: bug 721745
Comment 19 Paul Rouget [:paul] 2012-01-27 07:14:37 PST
taking this bug
Comment 20 Paul Rouget [:paul] 2012-01-27 08:22:15 PST
Sounds like this is going to be fixed in bug 719607.
Comment 21 Dave Camp (:dcamp) 2012-01-28 13:38:28 PST
It will be fixed for the HTML panel in bug 719607, but not the sidebar.
Comment 22 Dave Camp (:dcamp) 2012-01-28 13:39:53 PST
Created attachment 592435 [details] [diff] [review]
fix v1

Pending a larger reworking of registerTools, this will at least let us save the state of the sidebar for now.  Haven't written a test yet, but in case you're curious about the approach while I write that test...
Comment 23 Dave Camp (:dcamp) 2012-01-28 14:48:18 PST
Created attachment 592443 [details] [diff] [review]
fix v2

Now with a test.
Comment 24 Dave Camp (:dcamp) 2012-01-28 14:50:45 PST
I removed saveToolState and removeToolState in favor of sidebar-specific saving.  This will break registerTools that aren't sidebars.  But with bug 719607 we don't have any tools that aren't sidebars, and we're planning on messing more with  registerTools in the very near future anyway.
Comment 25 Rob Campbell [:rc] (:robcee) 2012-01-29 09:45:25 PST
Comment on attachment 592443 [details] [diff] [review]
fix v2

I have not had a chance to test this, but codewise this looks excellent.
Comment 27 Tim Taubert [:ttaubert] 2012-02-27 01:20:16 PST
https://hg.mozilla.org/mozilla-central/rev/31daf7fba165

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