Closed Bug 777085 Opened 12 years ago Closed 12 years ago

A new markup panel

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Version:
13 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 17

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(1 file, 8 obsolete files)

Patch to land.
158.25 KB, patch
Details | Diff | Splinter Review
Attached patch WIP 1 (obsolete) — — Splinter Review 
Here's a WIP
Attachment #645512 - Attachment is patch: true
Does this need a feedback?
Attached patch v1 (obsolete) — — Splinter Review 

        Attachment #645512 -
        Attachment is obsolete: true

        Attachment #651080 -
        Flags: review?(jwalker)

    
  

        Attachment #651080 -
        Flags: review?(fayearthur)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 tests (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
I pinged Joe and Heather for review here, I'd be happy for review from either or both of you.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 (obsolete)
        — 
          — Splinter Review
      

    


  
* 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)

    
  

        Attachment #652559 -
        Attachment is obsolete: true

        Attachment #652559 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #652561 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 tests (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #652561 -
        Attachment is obsolete: true

        Attachment #652561 -
        Flags: review?(jwalker)

        Attachment #652562 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3.1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #651081 -
        Attachment is obsolete: true

        Attachment #652846 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3.2 (obsolete)
        — 
          — Splinter Review
      

    


  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch to land.
        — 
          — Splinter Review
      

    


  

        Attachment #652562 -
        Attachment is obsolete: true

        Attachment #652847 -
        Attachment is obsolete: true

    
  
Blocks: 785150

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/844da7b047d2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17

    
  
Depends on: 785380

    
  
Depends on: 785421

    
  
Depends on: 785186

    
  
Flags: sec-review?(mgoodwin)

    
  
Blocks: 789902
Depends on: 787481

    
  
Depends on: 801424
Flags: sec-review?(mgoodwin) → sec-review+

    
  
Product: Firefox → DevTools

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: