Last Comment Bug 777085 - A new markup panel
: A new markup panel
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Dave Camp (:dcamp)
:
Mentors:
Depends on: 785186 785335 785380 785421 787481 797052 801424
Blocks: 785150 789902
  Show dependency treegraph
 
Reported: 2012-07-24 14:40 PDT by Dave Camp (:dcamp)
Modified: 2013-01-04 02:02 PST (History)
13 users (show)
mgoodwin: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (66.03 KB, patch)
2012-07-24 14:40 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review
v1 (71.97 KB, patch)
2012-08-10 21:06 PDT, Dave Camp (:dcamp)
dao+bmo: review-
Details | Diff | Review
v1 tests (80.52 KB, patch)
2012-08-10 21:07 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review
v2 (77.86 KB, patch)
2012-08-16 14:04 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review
v3 (77.82 KB, patch)
2012-08-16 14:08 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review
v3 tests (81.09 KB, patch)
2012-08-16 14:08 PDT, Dave Camp (:dcamp)
jwalker: review+
Details | Diff | Review
v3.1 (77.95 KB, patch)
2012-08-17 11:27 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review
v3.2 (77.92 KB, patch)
2012-08-17 11:35 PDT, Dave Camp (:dcamp)
jwalker: review+
Details | Diff | Review
Patch to land. (158.25 KB, patch)
2012-08-23 11:02 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review

Description Dave Camp (:dcamp) 2012-07-24 14:40:30 PDT
Created attachment 645512 [details] [diff] [review]
WIP 1

Here's a WIP
Comment 1 Paul Rouget [:paul] 2012-08-07 02:08:19 PDT
Does this need a feedback?
Comment 2 Dave Camp (:dcamp) 2012-08-10 21:06:25 PDT
Created attachment 651080 [details] [diff] [review]
v1
Comment 3 Dave Camp (:dcamp) 2012-08-10 21:07:07 PDT
Created attachment 651081 [details] [diff] [review]
v1 tests
Comment 4 Dave Camp (:dcamp) 2012-08-10 21:08:19 PDT
I pinged Joe and Heather for review here, I'd be happy for review from either or both of you.
Comment 5 Dave Camp (:dcamp) 2012-08-10 21:10:15 PDT
I'm mostly happy with the tests, but didn't test iframes.  Should probably figure that out before landing, but this should be far enough along to start review on while I write an iframe test.
Comment 6 Dave Camp (:dcamp) 2012-08-10 21:12:40 PDT
Rob, as module owner can you make a ruling on the preferred naming of jsms in the devtools?  Right now we have StudlyCaps.jsm and mushedlowercase.jsm.  To make things interesting I used dashed-lowercase.jsm.
Comment 7 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-08-10 22:38:47 PDT
(In reply to Dave Camp (:dcamp) from comment #6)
> StudlyCaps.jsm

This is what most of the rest of the tree uses.
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-11 00:40:51 PDT
There is a pattern that is applied 99% of the time (afaics) which has some logic and was actually discussed already:
- StudlyCapsForNormal.jsm
- importedorgenerated.jsm

The thinking was that lower case would alert you to the fact that something was different here. It's not clear to me that this actually works, and while we're moving GCLI files around we could easily 'fix' the commandline directory if we wanted (bug 776875).
Comment 9 Dão Gottwald [:dao] 2012-08-11 02:43:39 PDT
(In reply to Dave Camp (:dcamp) from comment #6)
> Rob, as module owner can you make a ruling on the preferred naming of jsms
> in the devtools?  Right now we have StudlyCaps.jsm and mushedlowercase.jsm. 
> To make things interesting I used dashed-lowercase.jsm.

I would also appreciate it if you could package the new jsm in /modules/devtools/ rather than /modules/.
Comment 10 Dão Gottwald [:dao] 2012-08-11 02:58:08 PDT
Comment on attachment 651080 [details] [diff] [review]
v1

>--- /dev/null
>+++ b/browser/devtools/highlighter/markup-panel.css

"highlighter/markup-panel.css" doesn't seem to make sense. Should the folder be renamed (in a separate bug) or should this new code just live somewhere else?

>+.newattr {
>+	display: inline-block;
>+	width: 1em;
>+	height: 1ex;
>+}

TABs crept in

>\ No newline at end of file

add newline at end of file

>--- /dev/null
>+++ b/browser/devtools/highlighter/markup-panel.html
>@@ -0,0 +1,36 @@
>+<!-- This Source Code Form is subject to the terms of the Mozilla Public
>+   - License, v. 2.0. If a copy of the MPL was not distributed with this
>+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>+
>+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
>+  "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">

<!DOCTYPE html>

>+<html xmlns="http://www.w3.org/1999/xhtml" xmlns:xul=xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xml:lang="en">

Please decide whether you want HTML or XHTML. The former doesn't have namespaces, the latter needs an XML prolog. xml:lang="en" is probably pointless anyway.

>--- /dev/null
>+++ b/browser/themes/gnomestripe/devtools/markup-panel.css

>+body {
>+  font-family: Menlo, Andale Mono, monospace;

font-family: monospace;

>+  font-size: 11px;

What exactly is this trying to do? Approximate the average system font size?

>+.styleinspector-propertyeditor {
>+  border: 1px solid #CCC;
>+s}

typo

>--- a/browser/themes/winstripe/jar.mn
>+++ b/browser/themes/winstripe/jar.mn
>@@ -128,16 +128,17 @@ browser.jar:
>         skin/classic/browser/devtools/common.css                    (devtools/common.css)
>         skin/classic/browser/devtools/arrows.png                    (devtools/arrows.png)
>         skin/classic/browser/devtools/commandline.png               (devtools/commandline.png)
>         skin/classic/browser/devtools/alerticon-warning.png         (devtools/alerticon-warning.png)
>         skin/classic/browser/devtools/goto-mdn.png                  (devtools/goto-mdn.png)
>         skin/classic/browser/devtools/csshtmltree.css               (devtools/csshtmltree.css)
>         skin/classic/browser/devtools/gcli.css                      (devtools/gcli.css)
>         skin/classic/browser/devtools/htmlpanel.css                 (devtools/htmlpanel.css)
>+        skin/classic/browser/devtools/markup-panel.css              (devtools/markup-panel.css)
>         skin/classic/browser/devtools/orion.css                     (devtools/orion.css)
>         skin/classic/browser/devtools/orion-container.css           (devtools/orion-container.css)
>         skin/classic/browser/devtools/orion-task.png                (devtools/orion-task.png)
>         skin/classic/browser/devtools/orion-breakpoint.png          (devtools/orion-breakpoint.png)
>         skin/classic/browser/devtools/orion-debug-location.png      (devtools/orion-debug-location.png)
>         skin/classic/browser/devtools/toolbarbutton-close.png       (devtools/toolbarbutton-close.png)
>         skin/classic/browser/devtools/webconsole.css                  (devtools/webconsole.css)
>         skin/classic/browser/devtools/webconsole_networkpanel.css     (devtools/webconsole_networkpanel.css)

missed aero
Comment 11 Mihai Sucan [:msucan] 2012-08-13 12:01:51 PDT
Comment on attachment 651080 [details] [diff] [review]
v1

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

Tested this and I like it. Much better than the old HTML panel we have. I'm glad to see this one is keyboard-accessible.

Just a quick pass through the code...

::: browser/devtools/highlighter/inspector.jsm
@@ +13,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/TreePanel.jsm");
> +Cu.import("resource:///modules/markup-panel.jsm");

Shouldn't this be in modules/devtools/ ?

::: browser/themes/gnomestripe/devtools/markup-panel.css
@@ +8,5 @@
> +}
> +
> +body {
> +  font-family: Menlo, Andale Mono, monospace;
> +  font-size: 11px;

Can you please allow the system font size here? Being explicit about font sizes in pixels prevents users to change system-wide font size settings (making this an accessibility problem). Thank you!

You can use ems or percentages to adjust the font size, based on the system default size.
Comment 12 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-14 08:50:05 PDT
Comment on attachment 651080 [details] [diff] [review]
v1

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

::: browser/devtools/highlighter/markup-panel.css
@@ +16,5 @@
> +
> +.newattr {
> +	display: inline-block;
> +	width: 1em;
> +	height: 1ex;

Do we care about tabs in CSS?
And should these values be in theme CSS?

::: browser/devtools/highlighter/markup-panel.jsm
@@ +48,5 @@
> + *   UI.
> + * @param integer aMaxUndo Maximum number of undo steps.
> + *   defaults to 50.
> + */
> +function UndoStack(aChange, aMaxUndo)

Should we have this in devtools/shared/Undo.jsm?

@@ +51,5 @@
> + */
> +function UndoStack(aChange, aMaxUndo)
> +{
> +  this.maxUndo = aMaxUndo || 50;
> +  this.stack = [];

Nit: Do we mean to imply that stack is public? I don't think users can safely mess with stack.

@@ +52,5 @@
> +function UndoStack(aChange, aMaxUndo)
> +{
> +  this.maxUndo = aMaxUndo || 50;
> +  this.stack = [];
> +  this.change = aChange || function() {};

What we need is an event framework. Want me to write you one?

@@ +67,5 @@
> +  /**
> +   * Start a collection of related changes.  Changes will be batched
> +   * together into one undo/redo item until endBatch() is called.
> +   * Batches can be nested, in which case the outer batch will contain
> +   * all items from the inner batches.

The reason for needing *nested* batches isn't obvious. I think I understand from reading the code, but perhaps a doc note?

@@ +538,5 @@
> +  } else {
> +    this.editor = new GenericEditor(this.markup, aNode);
> +  }
> +
> +  this.markup.template("container", this);

One of the 'unexpected' things about templater/save is that it creates a bunch of variable on 'this' which the casual observer might not be aware of. So you could consider:

this.expander = null; // These elements setup by the templater process
this.attr = null;
...

this.markup.template("container", this);


Also:

var options = { stack: "markup-panel.html" };
this.markup.template("container", this, options);

Does 2 things.
- It tells the reader of the source that this could be linked to markup-panel.html
- It prefixes the stack of any error messages with what you passed in.
  For example
    Expected foo to resolve to a function, but got frog. At markup-panel.html > body > div > onload

@@ +681,5 @@
> + * @param Inspector aInspector
> + *        The inspector we're watching.
> + * @param iframe aFrame
> + *        An iframe in which the caller has kindly loaded markup-panel.html.
> + */

Might it help readability to move this to the top? (or maybe you're a read-it-from-the-bottom kind of guy).

::: browser/themes/gnomestripe/devtools/markup-panel.css
@@ +8,5 @@
> +}
> +
> +body {
> +  font-family: Menlo, Andale Mono, monospace;
> +  font-size: 11px;

GCLI went for 'font: message-box;' for what I think you're trying to do here.
My recollection is that 'font-family: monospace' looks poor on Ubuntu.

@@ +40,5 @@
> +
> +/* Give some padding to focusable elements to match the editor input
> + * that will replace them. */
> +span[tabindex] {
> +  display: inline-block;

content css?
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-14 08:59:34 PDT
(In reply to Joe Walker from comment #12)
...
> @@ +52,5 @@
> > +function UndoStack(aChange, aMaxUndo)
> > +{
> > +  this.maxUndo = aMaxUndo || 50;
> > +  this.stack = [];
> > +  this.change = aChange || function() {};
> 
> What we need is an event framework. Want me to write you one?

To clarify a totally unclear comment, why don't we use bug 723904?
The answer to that is obvious, it's not done. I wonder how easy it would be to get that in?
Paul?
Comment 14 Paul Rouget [:paul] 2012-08-14 09:03:53 PDT
(In reply to Joe Walker from comment #13)
> (In reply to Joe Walker from comment #12)
> ...
> > @@ +52,5 @@
> > > +function UndoStack(aChange, aMaxUndo)
> > > +{
> > > +  this.maxUndo = aMaxUndo || 50;
> > > +  this.stack = [];
> > > +  this.change = aChange || function() {};
> > 
> > What we need is an event framework. Want me to write you one?
> 
> To clarify a totally unclear comment, why don't we use bug 723904?
> The answer to that is obvious, it's not done. I wonder how easy it would be
> to get that in?
> Paul?

IIRC, the code is ready. It even has tests.
Comment 15 Dave Camp (:dcamp) 2012-08-14 15:34:39 PDT
(In reply to Blair McBride (:Unfocused) from comment #7)
> (In reply to Dave Camp (:dcamp) from comment #6)
> > StudlyCaps.jsm
> 
> This is what most of the rest of the tree uses.

That's what I'm going to do.
Comment 16 Dave Camp (:dcamp) 2012-08-14 15:44:45 PDT
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 651080 [details] [diff] [review]
> v1
> 
> >--- /dev/null
> >+++ b/browser/devtools/highlighter/markup-panel.css
> 
> "highlighter/markup-panel.css" doesn't seem to make sense. Should the folder
> be renamed (in a separate bug) or should this new code just live somewhere
> else?

Yeah, I agree - we can rename highlighter to 'inspector' and maybe merge in styleinspector and layoutview, or embrace the hierarchy and move the markup panel into its own dir.  Opinion, Rob?

(In reply to Dão Gottwald [:dao] from comment #9)
> I would also appreciate it if you could package the new jsm in
> /modules/devtools/ rather than /modules/.

Yep.  While I'm at it I'll clean up the remaining modules in the highlighter dir that install into modules/

(In reply to Joe Walker [:jwalker] from comment #13)
> > What we need is an event framework. Want me to write you one?
> 
> To clarify a totally unclear comment, why don't we use bug 723904?
> The answer to that is obvious, it's not done. I wonder how easy it would be
> to get that in?

If that gets in before this does, I'll be happy to change it.
 
> Should we have this in devtools/shared/Undo.jsm?

Quite possibly.  I have that listed in the comments as a follow-to-file, but I'm happy to do it here if you think it's a good idea.

> @@ +538,5 @@
> > +  } else {
> > +    this.editor = new GenericEditor(this.markup, aNode);
> > +  }
> > +
> > +  this.markup.template("container", this);
> 
> One of the 'unexpected' things about templater/save is that it creates a
> bunch of variable on 'this' which the casual observer might not be aware of.
> So you could consider:
> 
> this.expander = null; // These elements setup by the templater process
> this.attr = null;

That's a good idea.  I do it with a comment in one place, but I think that's more clear.

> Also:
> 
> var options = { stack: "markup-panel.html" };
> this.markup.template("container", this, options);
> 
> Does 2 things.
> - It tells the reader of the source that this could be linked to
> markup-panel.html
> - It prefixes the stack of any error messages with what you passed in.
>   For example
>     Expected foo to resolve to a function, but got frog. At
> markup-panel.html > body > div > onload

Today I learned.  Thanks.
 
> @@ +681,5 @@
> > + * @param Inspector aInspector
> > + *        The inspector we're watching.
> > + * @param iframe aFrame
> > + *        An iframe in which the caller has kindly loaded markup-panel.html.
> > + */
> 
> Might it help readability to move this to the top? (or maybe you're a
> read-it-from-the-bottom kind of guy).

You caught me, I'm a recovering C programmer.  Yeah, I can move that to the top.

Thanks in particular for the css and html/xhtml comments that I haven't responded to but will fix in the next patch.
Comment 17 Dave Camp (:dcamp) 2012-08-16 13:56:23 PDT
(In reply to Joe Walker [:jwalker] from comment #12)

> @@ +40,5 @@
> > +
> > +/* Give some padding to focusable elements to match the editor input
> > + * that will replace them. */
> > +span[tabindex] {
> > +  display: inline-block;
> 
> content css?

I don't think so?  The padding that needs to be added is theme-dependent, and the inline-block is really there to make the padding work... I think it would make things less clear to move half of that declaration to content css.
Comment 18 Dave Camp (:dcamp) 2012-08-16 14:04:00 PDT
Created attachment 652559 [details] [diff] [review]
v2

* I ended up moving Undo out to its own jsm.
* Rather than use EventEmitter to manage the command controller, I think it makes more sense to actually let the UndoStack act as the command controller itself, so I sidestepped that issue.
Comment 19 Dave Camp (:dcamp) 2012-08-16 14:08:05 PDT
Created attachment 652561 [details] [diff] [review]
v3
Comment 20 Dave Camp (:dcamp) 2012-08-16 14:08:29 PDT
Created attachment 652562 [details] [diff] [review]
v3 tests
Comment 21 Dave Camp (:dcamp) 2012-08-17 11:27:28 PDT
Created attachment 652846 [details] [diff] [review]
v3.1
Comment 22 Dave Camp (:dcamp) 2012-08-17 11:35:33 PDT
Created attachment 652847 [details] [diff] [review]
v3.2

One more try.
Comment 23 Mihai Sucan [:msucan] 2012-08-17 11:39:42 PDT
Thanks for addressing my comment about font sizes.

A couple of suggestions:

(1) trim whitespace in text nodes (at least by default, we may have an option later to show whitespace). Currently text nodes look weird when expanded with nice source-formatted code that has a lot of whitespace in elements.
(2) auto-expand elements that contain only one small text node (where small is whatever we arbitrarily pick, like 100 chars).

Suggestions came from playing with:
https://orion.eclipse.org/examples/textview/demo.html

With (1) and (2) going through the markup of the demo would be a nicer experience.
Comment 24 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-20 10:58:33 PDT
Comment on attachment 652847 [details] [diff] [review]
v3.2

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

::: browser/devtools/markupview/MarkupView.jsm
@@ +1098,5 @@
> +
> +  previousSibling: function DW_previousSibling() this.walker.previousSibling(),
> +  nextSibling: function DW_nextSibling() this.walker.nextSibling(),
> +
> +  // XXX: not doing previousNode or nextNode, which would sure be useful.

I hate myself for suggesting this, but maybe raise a bug and put the bug number in here?

::: browser/devtools/markupview/markup-view.css
@@ +16,5 @@
> +
> +.newattr {
> +	display: inline-block;
> +	width: 1em;
> +	height: 1ex;

Probably s/\t/ /g ?

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +1883,4 @@
>   * @param function aCallback
>   *        Called when the editor is activated.
>   */
> +function editableItem(aOptions, aCallback)

It seems a bit strange to me to have something that is mandatory after something that is optional. But I'm not sure I've a better suggestion.

::: browser/themes/pinstripe/devtools/gcli.css
@@ +4,5 @@
>  
>  .gcli-body {
>    margin: 0;
>    font: message-box;
> +  font-size: 80%;

Hmm, see my next comment.
OR maybe I'm missing something??

::: browser/themes/pinstripe/devtools/markup-view.css
@@ +8,5 @@
> +}
> +
> +body {
> +  font-family: message-box;
> +  font-size: .9em;

I think you want:
-  font-family: message-box;
-  font-size: .9em;
+  font: message-box;

message-box is magic voodoo.
Comment 25 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-20 11:00:50 PDT
Comment on attachment 652562 [details] [diff] [review]
v3 tests

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

::: browser/devtools/highlighter/test/browser_inspector_treePanel_output.js
@@ -87,5 @@
> -  gBrowser.removeCurrentTab();
> -  finish();
> -}
> -
> -function test()

Not worth fixing, but maybe this belongs at the top?
Comment 26 Dave Camp (:dcamp) 2012-08-23 11:02:57 PDT
Created attachment 654695 [details] [diff] [review]
Patch to land.
Comment 28 Panos Astithas [:past] 2012-08-24 01:23:41 PDT
https://hg.mozilla.org/mozilla-central/rev/844da7b047d2

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