Closed Bug 1238133 Opened 4 years ago Closed 4 years ago

Make inspector side panels all live inside the inspector panel frame

Categories

(DevTools :: Inspector, defect)

45 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 46

People

(Reporter: bgrins, Assigned: gl)

References

(Depends on 1 open bug)

Details

Attachments

(8 files, 16 obsolete files)

12.90 KB, patch
bgrins
: review+
u560451
: checkin+
Details | Diff | Splinter Review
6.54 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
24.45 KB, patch
Details | Diff | Splinter Review
25.91 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
20.33 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
65.58 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
5.79 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
2.09 KB, patch
pbro
: review+
Details | Diff | Splinter Review
Right now the tabs are organized like so:

Rules (which loads client/inspector/rules/rules.xhtml)
Computed (which loads client/inspector/rules/computed.xhtml)
Fonts (which loads client/inspector/rules/fonts.xhtml)
Layout (which loads client/inspector/rules/layout.xhtml)

There's a problem right now where we initialize all of the frames on tool startup.  However, even if we fix that it seems unnecessary that these all live in their own frames.  All of the side panel's markup could live in inspector.xul and the tools could be built in that single window.  And the Sidebar module doesn't require iframes so that part should be fine.
Assignee: nobody → gl
Part 1 deals with merging the 2 fonts.css and rules.css that exist in themes/ and within their respective directories. Since we will be loading all the panels' stylesheet into inspector.xul, this is a good time to ensure there is only 1 instance of each stylesheet for each view.
Assignee: gl → gluong
Status: NEW → ASSIGNED
Attachment #8707230 - Flags: review?(bgrinstead)
Attachment #8707230 - Flags: review?(bgrinstead) → review+
Keywords: leave-open
Attachment #8707230 - Flags: checkin+
Attachment #8707237 - Flags: review?(bgrinstead) → review+
Assignee: u560451 → gl
Comment on attachment 8708139 [details] [diff] [review]
Part 3: Make the rule view live inside the inspector panel frame

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

Changes look good to me.  Requesting review from Patrick as another sign off.

I didn't think about one issue with this when requesting the change, which is that we are moving more UI into a XUL doc.  Of course we are using HTML namespace but this could introduce funkiness when pulling in things like React.  Although we are running into the same sorts of issues with the webconsole so we'll be coming up with some solution anyway (bug 1238881).  I still believe this is the right thing to do, and if we want to keep it free of xul then we could work towards a layout like this:

-inspector.xul - contains context menus, panels, etc and nothing else except for an <iframe> to inspector.xhtml
-- inspector.xhtml - contains all other markup in inspector panel plus all the sidebar panels as they get pulled out of frames

I guess the question is if we want to hold up this work on that.  I'd vote no but worth discussing.

::: devtools/client/inspector/rules/test/browser_rules_add-property-cancel_01.js
@@ +41,5 @@
>    is(elementRuleEditor.rule.textProps.length, 0,
>      "Should have cancelled creating a new text property.");
>    ok(!elementRuleEditor.propertyList.hasChildNodes(),
>      "Should not have any properties.");
> +  is(view.styleDocument.documentElement, view.styleDocument.activeElement,

Nit here and elsewhere - the existing assertion order seems backwards.  While we are at changing this we may as well swap them:

is(view.styleDocument.activeElement, view.styleDocument.documentElement

::: devtools/client/inspector/rules/test/browser_rules_edit-selector_05.js
@@ +53,5 @@
> +  // Escape the new property editor after editing the selector
> +  let onBlur = once(view.styleDocument.activeElement, "blur");
> +  EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow);
> +  yield onBlur;
> +  yield ruleEditor.rule._applyingModifications;

I don't think we need to wait on _applyingModifications

Also, I believe we can move the blurring after the assertions since they don't seem relevant to them - it's just cleaning up after itself.
Attachment #8708139 - Flags: review?(pbrosset)
Attachment #8708139 - Flags: review?(bgrinstead)
Attachment #8708139 - Flags: review+
Attachment #8708622 - Attachment is obsolete: true
Attachment #8708622 - Flags: review?(bgrinstead)
Attachment #8708718 - Flags: review?(bgrinstead)
Attachment #8707237 - Flags: checkin+
Attachment #8708718 - Attachment is obsolete: true
Attachment #8708718 - Flags: review?(bgrinstead)
Attachment #8708754 - Flags: review?(bgrinstead)
Attachment #8708706 - Attachment is obsolete: true
Attachment #8708706 - Flags: review?(bgrinstead)
Attachment #8708755 - Flags: review?(bgrinstead)
Comment on attachment 8708139 [details] [diff] [review]
Part 3: Make the rule view live inside the inspector panel frame

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

As discussed with Gabriel last week, I also believe this is the right thing to do, even if that means temporarily bringing more markup into a XUL document.
Moving the inspector to React seems so much further down the road that I don't think we should hold off because of this.
I like Brian's idea of moving the context menu and tooltips into inspector.xul and having a (single) iframe for the actual content of the inspector panel.
Fwiw, they could even be in toolbox.xul and therefore have the inspector just be an xhtml document.

I found a couple of bugs:
- right-click/select all -> The text of the breadcrumbs widget also gets selected
- the height of the inspector and the rule-view is bigger than the available height, I'll attach a gif to illustrate

::: devtools/client/inspector/rules/test/browser_rules_edit-selector_05.js
@@ +53,5 @@
> +  // Escape the new property editor after editing the selector
> +  let onBlur = once(view.styleDocument.activeElement, "blur");
> +  EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow);
> +  yield onBlur;
> +  yield ruleEditor.rule._applyingModifications;

Agreed with Brian.
Also, we shouldn't use _applyingModifications at all, as a general rule.
Instead, there are events (like "ruleview-changed") that can be used.

::: devtools/client/inspector/rules/test/browser_rules_search-filter_context-menu.js
@@ +35,5 @@
>    is(cmdUndo.getAttribute("disabled"), "true", "cmdUndo is disabled");
>    is(cmdDelete.getAttribute("disabled"), "true", "cmdDelete is disabled");
>    is(cmdSelectAll.getAttribute("disabled"), "", "cmdSelectAll is enabled");
> +  is(cmdCut.getAttribute("disabled"), "true", "cmdCut is disabled");
> +  is(cmdCopy.getAttribute("disabled"), "true", "cmdCopy is disabled");

I must have missed something, how come these are disabled now?

::: devtools/client/inspector/rules/test/head.js
@@ +247,5 @@
>   *
>   * @return a promise that resolves when the inspector is ready and the sidebar
>   * view is visible and ready
>   */
>  var openInspectorSideBar = Task.async(function*(id) {

We need to be aware that this may conflict with the changes I'm making in bug 1181837. Not a big deal, but there will be merges to be done.
Attachment #8708139 - Flags: review?(pbrosset)
Attached image viewport-height.gif (obsolete) —
I forgot to attach the gif I mentioned in my last comment. Here it is.
Attachment #8709143 - Attachment is obsolete: true
Attachment #8708139 - Attachment is obsolete: true
Attachment #8709151 - Flags: review+
Attachment #8709151 - Attachment is obsolete: true
Attachment #8709172 - Flags: review+
Attachment #8708754 - Flags: review?(bgrinstead)
Attachment #8708755 - Flags: review?(bgrinstead)
Attachment #8709172 - Attachment is obsolete: true
Attachment #8710107 - Flags: review+
Attachment #8708754 - Attachment is obsolete: true
Attachment #8710108 - Flags: review?(bgrinstead)
Attachment #8708755 - Attachment is obsolete: true
Attachment #8710109 - Flags: review?(bgrinstead)
Attachment #8710109 - Attachment is obsolete: true
Attachment #8710109 - Flags: review?(bgrinstead)
Attachment #8710804 - Flags: review?(bgrinstead)
Sorry, didn't get to the reviews this week.  I'll take a look early next week
Attachment #8710108 - Attachment is obsolete: true
Attachment #8710108 - Flags: review?(bgrinstead)
Attachment #8711305 - Flags: review?(bgrinstead)
Attachment #8710804 - Attachment is obsolete: true
Attachment #8710804 - Flags: review?(bgrinstead)
Attachment #8711306 - Flags: review?(bgrinstead)
Attachment #8710805 - Attachment is obsolete: true
Attachment #8710805 - Flags: review?(bgrinstead)
Attachment #8711307 - Flags: review?(bgrinstead)
Attachment #8710806 - Attachment is obsolete: true
Attachment #8710806 - Flags: review?(bgrinstead)
Attachment #8711308 - Flags: review?(bgrinstead)
Comment on attachment 8711305 [details] [diff] [review]
Part 4: Make the computed view live inside the inspector panel frame [4.0]

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

::: devtools/client/inspector/computed/test/browser_computed_select-and-copy-styles.js
@@ +52,5 @@
>    ok(props, "captain, we have the property-view nodes");
>  
>    let range = contentDocument.createRange();
>    range.setStart(props[1], 0);
> +  range.setEnd(props[3], 2);

Why is this needed?

::: devtools/client/inspector/inspector-panel.js
@@ +356,5 @@
>  
>      this.sidebar.on("select", this._setDefaultSidebar);
>  
>      this.ruleview = new RuleViewTool(this, this.panelWin);
> +    this.computedview = new ComputedViewTool(this, this.panelWin);

As we were discussing earlier, I'd prefer that we modify the Sidebar API such that it doesn't use addExistingTabs anymore and instead we can call addTab with an existing DOM element instead of just a URL.  Seems like follow-up work though, especially since we'll have to migrate the netmonitor for this.
Attachment #8711305 - Flags: review?(bgrinstead) → review+
Attachment #8711309 - Attachment is obsolete: true
Attachment #8711309 - Flags: review?(bgrinstead)
Attachment #8711942 - Flags: review?(bgrinstead)
Comment on attachment 8711306 [details] [diff] [review]
Part 5: Make the font inspector live inside the inspector panel frame [4.0]

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

::: devtools/client/inspector/inspector.xul
@@ +184,5 @@
>               crop="end"/>
> +        <tab id="sidebar-tab-fontinspector"
> +             label="&fontInspectorTitle;"
> +             crop="end"
> +             hidden="true"/>

Why is this hidden="true"?
Attachment #8711306 - Flags: review?(bgrinstead) → review+
Attachment #8711307 - Flags: review?(bgrinstead) → review+
Attachment #8711308 - Flags: review?(bgrinstead) → review+
Comment on attachment 8711942 [details] [diff] [review]
Part 8: Select the default inspector sidebar tab on open and record the tool opened [2.0]

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

I don't love this but I like it more than putting the same logic in inspector-panel.  Redirecting review to Patrick - also see Comment 34 with a suggestion for changing up the Sidebar object in a way that might avoid this
Attachment #8711942 - Flags: review?(bgrinstead) → review?(pbrosset)
Comment on attachment 8711942 [details] [diff] [review]
Part 8: Select the default inspector sidebar tab on open and record the tool opened [2.0]

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

::: devtools/client/framework/sidebar.js
@@ +468,5 @@
>  
> +    // If an id is given, select the corresponding sidebar tab and record the
> +    // tool opened.
> +    if (id) {
> +      this._panelDoc.defaultView.setTimeout(() => {

The comment above the other setTimeout in addTab says "For some reason I don't understand ...". I'm guessing we still don't know why that is needed and haven't found another (nicer) work around?
If that is the case, then I'm fine adding one more setTimeout in here. The 10ms duration does seem random however. Shouldn't it be exactly the same to use 0ms?
Also, please investigate a way to move the 2 setTimeout instances in one function only. Something like:

/**
 * Hack required to select a tab right after it was created.
 */
_selectTabSoon: function(id) {
  this._panelDoc.defaultView.setTimeout(() => {
    this.select(id);
  }, 0);
}

I'm assuming here that only `this.select(id)` requires the setTimeout hack and that you could safely set _currentTool and call _telemetry.toolOpened before.
Attachment #8711942 - Flags: review?(pbrosset) → review+
Depends on: 1243598
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1247534
Depends on: 1260235
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Depends on: 1274821
Depends on: 1201750
Depends on: 1279651
Depends on: 1286538
Depends on: 1327981
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.