A new markup panel

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

13 Branch
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
sec-review +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 645512 [details] [diff] [review]
WIP 1

Here's a WIP

Updated

5 years ago
Attachment #645512 - Attachment is patch: true

Comment 1

5 years ago
Does this need a feedback?
(Assignee)

Comment 2

5 years ago
Created attachment 651080 [details] [diff] [review]
v1
Attachment #645512 - Attachment is obsolete: true
Attachment #651080 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #651080 - Flags: review?(fayearthur)
(Assignee)

Comment 3

5 years ago
Created attachment 651081 [details] [diff] [review]
v1 tests
(Assignee)

Comment 4

5 years ago
I pinged Joe and Heather for review here, I'd be happy for review from either or both of you.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
(In reply to Dave Camp (:dcamp) from comment #6)
> StudlyCaps.jsm

This is what most of the rest of the tree uses.
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).
(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 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
Attachment #651080 - Flags: review-
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 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?
Attachment #651080 - Flags: review?(jwalker)
(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

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
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.
Attachment #651080 - Attachment is obsolete: true
Attachment #651080 - Flags: review?(fayearthur)
Attachment #652559 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #652559 - Attachment is obsolete: true
Attachment #652559 - Flags: review?(jwalker)
(Assignee)

Comment 19

5 years ago
Created attachment 652561 [details] [diff] [review]
v3
Attachment #652561 - Flags: review?(jwalker)
(Assignee)

Comment 20

5 years ago
Created attachment 652562 [details] [diff] [review]
v3 tests
Attachment #652561 - Attachment is obsolete: true
Attachment #652561 - Flags: review?(jwalker)
Attachment #652562 - Flags: review?(jwalker)
(Assignee)

Comment 21

5 years ago
Created attachment 652846 [details] [diff] [review]
v3.1
Attachment #651081 - Attachment is obsolete: true
Attachment #652846 - Flags: review?(jwalker)
(Assignee)

Comment 22

5 years ago
Created attachment 652847 [details] [diff] [review]
v3.2

One more try.
Attachment #652846 - Attachment is obsolete: true
Attachment #652846 - Flags: review?(jwalker)
Attachment #652847 - Flags: review?(jwalker)
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 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.
Attachment #652847 - Flags: review?(jwalker) → review+
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?
Attachment #652562 - Flags: review?(jwalker) → review+
(Assignee)

Comment 26

5 years ago
Created attachment 654695 [details] [diff] [review]
Patch to land.
Attachment #652562 - Attachment is obsolete: true
Attachment #652847 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/844da7b047d2
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 785150
https://hg.mozilla.org/mozilla-central/rev/844da7b047d2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
Depends on: 785335

Updated

5 years ago
Depends on: 785380

Updated

5 years ago
Depends on: 785421
(Assignee)

Updated

5 years ago
Depends on: 785186
(Assignee)

Updated

5 years ago
Flags: sec-review?(mgoodwin)

Updated

5 years ago
Blocks: 789902
Depends on: 797052
Depends on: 787481

Updated

5 years ago
Depends on: 801424
Flags: sec-review?(mgoodwin) → sec-review+
You need to log in before you can comment on or make changes to this bug.