Closed Bug 683954 Opened 13 years ago Closed 12 years ago

[Layout] Implement an abstract view of the layout of the selected node

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox14 disabled, firefox15 fixed)

RESOLVED FIXED
Firefox 14
Tracking Status
firefox14 --- disabled
firefox15 --- fixed

People

(Reporter: paul, Assigned: paul)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [minotaur][firefox14-wanted])

Attachments

(2 files, 14 obsolete files)

312.82 KB, image/jpeg
Details
69.75 KB, patch
Details | Diff | Splinter Review
This view should live into a Panel.
Whiteboard: [minotaur]
Depends on: 674871, 663831
Blocks: 663778
Assignee: nobody → paul
Attached image Possible design (obsolete) —
See bug 663778 for a preliminary patch.
Status: NEW → ASSIGNED
Depends on bug 663831 because the tool will register itself in initTools.
Attached patch patch v0.1 WIP (obsolete) — Splinter Review
The code lie in the devtools/ folder.
I use a floating panel.

The current implementation has 2 bugs I can't explain yet:
x The 2nd time the panel is select to be dragged, it becomes "click insensitive" (only on Mac). I believe we have the same problem with the HTML panel.
x Some times, the SVG document is not refresh correctly (screenshot to come)
Attachment #558438 - Attachment is obsolete: true
I came across this CSS grid today: http://leaverou.me/css3patterns/#blueprint-grid
It doesn't look as good as your SVG one, but you might want to borrow some ideas for a simpler/faster solution.
Blocks: 663830
Attached patch patch v0.3 WIP rebase (obsolete) — Splinter Review
Attachment #559427 - Attachment is obsolete: true
Blocks: 674887
Attached patch patch v1 (obsolete) — Splinter Review
The View is included in a <panel>. This is a temporary situtation. It will be
moved to a better place in bug 674887.
Attachment #567764 - Flags: review?(rcampbell)
Comment on attachment 567764 [details] [diff] [review]
patch v1

In LayoutView.jsm:

+function LayoutView(aContentWindow) {
+  this._init(aContentWindow);
+}

Nit: for function definitions, we usually put the open bracket on the following line. (Not my style choice but, pretty prevalent across mozcode).

+  onIframeReady: function() {

should include the message signature here and everywhere.

function LayoutView_onIframeReady() {

One pattern we've found useful is to pass in the instance of the InspectorUI and pull out the chrome and/or content windows from that. Also allows us to move the registration inside the tool itself at initialization time which cuts down on the amount that the Inspector has to know about the tool itself.

+    this._iframeReady = false;
+    this._iframe.addEventListener("load", this.onIframeReady.bind(this), true);

You should cache the bound method onIframeReady with something like:

this.boundIframeReady = this.onIframeReady.bind(this);

so you can remove it in the listener.

nit:

+  /**
+    * Select a node to draw
+    *
+    * @param aNode The node to draw

don't need the extra gap between Select comment and the @param line. You should also use punctuation in your comment lines, e.g., end your sentences with '.'.

+
+  hide: function() {

needs a javadoc.

typo:

+    * Hide and destrou the popup
                destroy
+  unregister: function() {
+    this.hide();

do we know the popup is being shown?

+    this._iframe = null;

you should probably removeChild() this from the panel before deleting the reference. This caused problems in the style inspector before.

should include function LayoutView_unregister()

in _computeLayout:
+      l.top = parseInt(valuetop);
+      l.right = parseInt(valueright);
+      l.bottom = parseInt(valuebottom);
+      l.left = parseInt(valueleft);

do we want to use parseInt for these? We're potentially throwing away fractions here.

+    layout.boundingbox.width = Math.round(rect.width);
+    layout.boundingbox.height = Math.round(rect.height);
+    layout.boundingbox.top = Math.round(rect.top);
+    layout.boundingbox.left = Math.round(rect.left);

and here, except you're rounding up. Not sure if these are going to ever be relevant.

+   * Memonized lookup of a l10n string from a string bundle.

I think you mean "Memoized". (I'm correcting those strings in the styleinspector.jsm too).

in layoutview.xhtml:

+    const RATIO_THRESHOLD = 0.3;

what's this? (ok, I found it later on, but maybe a comment would help)

+      if (ratio < 1 - RATIO_THRESHOLD) {

ratio < 2/3 ?

+      } else {
+        if (ratio > 1 + RATIO_THRESHOLD) {

You could rewrite this as:

} else if (ratio > 1 + RATIO_THRESHOLD) {

and move:

+        } else {

out of the nested if.

...

no issues with css, but you should ask dao.

in browser/themes/winstripe/browser/jar.mn:

+        skin/classic/browser/devtools/layoutview.css              (devtools/layoutview.css)

should be skin/classic/aero/browser/devtools/layoutview.css

Great start, this is going to be awesome. R- for now because of the method signatures, missing comments and various nits.
Attachment #567764 - Flags: review?(rcampbell) → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
Thank you Rob for the review. I addressed most of your comments.

(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> Comment on attachment 567764 [details] [diff] [review] [diff] [details] [review]
> patch v1
> You should cache the bound method onIframeReady with something like:
> 
> this.boundIframeReady = this.onIframeReady.bind(this);
> 
> so you can remove it in the listener.

Ok, I now remove the listener. But is that really useful since the listener
won't hold any strong reference to the iframe (the iframe get destroyed at
the end)?

> in _computeLayout:
> +      l.top = parseInt(valuetop);
> +      l.right = parseInt(valueright);
> +      l.bottom = parseInt(valuebottom);
> +      l.left = parseInt(valueleft);
> 
> do we want to use parseInt for these? We're potentially throwing away
> fractions here.
> 
> +    layout.boundingbox.width = Math.round(rect.width);
> +    layout.boundingbox.height = Math.round(rect.height);
> +    layout.boundingbox.top = Math.round(rect.top);
> +    layout.boundingbox.left = Math.round(rect.left);
> 
> and here, except you're rounding up. Not sure if these are going to ever be
> relevant.

These values are only displayed (not involved in any computation), that is why
I don't want any fractionned values.

> +      if (ratio < 1 - RATIO_THRESHOLD) {
> 
> ratio < 2/3 ?

Sorry?
Attachment #567764 - Attachment is obsolete: true
Attachment #565927 - Attachment is obsolete: true
Attachment #568023 - Flags: review?(rcampbell)
Comment on attachment 568023 [details] [diff] [review]
patch v1.1

Dao, can you please take a look at the CSS (it's the same for Linux, Mac and Windows)? Thanks!
Attachment #568023 - Flags: review?(dao)
Comment on attachment 568023 [details] [diff] [review]
patch v1.1

>+  <meta charset="UTF-8" />

This is invalid html.

>+<!ENTITY nonodeselected     "No node selected">

noNodeSelected

>+<!ENTITY dimension          "dimension">
>+<!ENTITY margins            "margins">
>+<!ENTITY borders            "borders">
>+<!ENTITY padding            "padding">
>+<!ENTITY content            "content">

These could probably use some l10n instructions and/or clearer entity names.

>+++ b/browser/themes/gnomestripe/browser/devtools/layoutview.css

>+body {
>+  width: 300px;
>+  height: 300px;

As I understand it, these numbers are fixed, i.e. it wouldn't make sense for a theme to change them. If so, please move them to a content stylesheet.

>+body:hover > #marginbox:not(:hover) > #borderbox:not(:hover) > #paddingbox:not(:hover) > #contentbox:not(:hover) > #dimension-label {

When #marginbox is not hovered, it's impossible for its children to be hovered, is it?
Attachment #568023 - Flags: review?(dao) → review-
OS: Mac OS X → All
Hardware: x86 → All
Attached patch patch v1.2 (obsolete) — Splinter Review
Addressed Dao's comments.
Attachment #568023 - Attachment is obsolete: true
Attachment #568023 - Flags: review?(rcampbell)
Attachment #568706 - Flags: review?(rcampbell)
Attachment #568706 - Flags: checkin?(dao)
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

removing checkin flag until review is complete.
Attachment #568706 - Flags: checkin?(dao)
Attachment #568706 - Flags: review?(dao)
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

>--- a/browser/devtools/jar.mn
>+++ b/browser/devtools/jar.mn
>@@ -2,9 +2,10 @@ browser.jar:
> *   content/browser/inspector.html                (highlighter/inspector.html)
>     content/browser/NetworkPanel.xhtml            (webconsole/NetworkPanel.xhtml)
> *   content/browser/scratchpad.xul                (scratchpad/scratchpad.xul)
> *   content/browser/scratchpad.js                 (scratchpad/scratchpad.js)
>     content/browser/csshtmltree.xhtml             (styleinspector/csshtmltree.xhtml)
>     content/browser/orion.js                      (sourceeditor/orion/orion.js)
>     content/browser/orion.css                     (sourceeditor/orion/orion.css)
>     content/browser/orion-mozilla.css             (sourceeditor/orion/mozilla.css)
>-
>+*   content/browser/layoutview.xhtml              (layoutview/layoutview.xhtml)
>+    content/browser/layoutview.css                (layoutview/layoutview.css)

Why are these folded into content/browser/ rather than content/browser/devtools/?
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

>+++ b/browser/themes/gnomestripe/browser/devtools/layoutview.css

>+#textbox,
>+#arrowbox {
>+  pointer-events: none;
>+  position: absolute;
>+  top: 0; left: 0;

line break after ;

>+  width: 300px;
>+  height: 300px;

>+.square > #marginbox { margin: 39px 40px 40px 39px; }
>+.vertical > #marginbox { margin: 39px 60px 40px 59px; }
>+.horizontal > #marginbox { margin: 59px 40px 60px 39px; }

>+#arrowbox .vertical::before,
>+#arrowbox .vertical::after {
>+  content: "";
>+  display: block;
>+  position: absolute;
>+  width: 4px;
>+  height: 4px;

>+#arrowbox .vertical::before {
>+  top: 0px;
>+  -moz-transform: rotate(45deg);
>+}
>+
>+#arrowbox .vertical::after {
>+  bottom: 0px;
>+  -moz-transform: rotate(225deg);
>+}

>+.square > #arrowbox > .top {
>+  top: 8px;
>+  left: 39px;
>+  height: 24px;
>+}
>+
>+.square > #arrowbox > .left {
>+  top: 39px;
>+  left: 8px;
>+  width: 24px;
>+}
>+
>+.square > #arrowbox > .width {
>+  bottom: 27px;
>+  left: 70px;
>+  width: 160px;
>+}
>+
>+.square > #arrowbox > .height {
>+  right: 27px;
>+  top: 70px;
>+  height: 160px;
>+}
>+
>+/* square */
>+
>+.square > #arrowbox > .top {
>+  top: 8px;
>+  left: 39px;
>+  height: 24px;
>+}
>+
>+.square > #arrowbox > .left {
>+  top: 39px;
>+  left: 8px;
>+  width: 24px;
>+}
>+
>+.square > #arrowbox > .width {
>+  bottom: 27px;
>+  left: 70px;
>+  width: 160px;
>+}
>+
>+.square > #arrowbox > .height {
>+  right: 27px;
>+  top: 70px;
>+  height: 160px;
>+}
>+
>+/* horizontal */
>+
>+.horizontal > #arrowbox > .top {
>+  top: 28px;
>+  left: 39px;
>+  height: 24px;
>+}
>+
>+.horizontal > #arrowbox > .left {
>+  top: 59px;
>+  left: 8px;
>+  width: 24px;
>+}
>+
>+.horizontal > #arrowbox > .width {
>+  bottom: 47px;
>+  left: 70px;
>+  width: 160px;
>+}
>+
>+.horizontal > #arrowbox > .height {
>+  right: 27px;
>+  top: 90px;
>+  height: 120px;
>+}
>+
>+/* vertical */
>+
>+.vertical > #arrowbox > .top {
>+  top: 8px;
>+  left: 59px;
>+  height: 24px;
>+}
>+
>+.vertical > #arrowbox > .left {
>+  top: 39px;
>+  left: 28px;
>+  width: 24px;
>+}
>+
>+.vertical > #arrowbox > .width {
>+  bottom: 27px;
>+  left: 90px;
>+  width: 120px;
>+}
>+
>+.vertical > #arrowbox > .height {
>+  right: 47px;
>+  top: 70px;
>+  height: 160px;
>+}

>+body[nonode] #marginbox,
>+body[nonode] #arrowbox,
>+body[nonode] #textbox > p {
>+  visibility: hidden;
>+}
>+body[nonode] #textbox > #nonodemessage {
>+  visibility: visible;
>+}

>+.label {
>+  pointer-events: none;

>+#dimension-label {
>+  top: 4px;
>+  right: 4px;
>+}
>+
>+#margin-label {
>+  top: 40px;
>+  right: 45px;
>+}
>+
>+#border-label {
>+  top: 70px;
>+  right: 75px;
>+}
>+
>+#padding-label {
>+  top: 90px;
>+  right: 95px;
>+}
>+
>+#content-label {
>+  top: 110px;
>+  right: 115px;
>+}
>+
>+.horizontal .label:not(#dimension-label) {
>+  margin-top: 20px;
>+}
>+
>+.vertical .label:not(#dimension-label) {
>+  top: 4px;
>+  right: 4px;
>+}

>+#nonodemessage {
>+  width: 300px;
>+  line-height: 300px;
>+  visibility: hidden;
>+}
>+
>+/* Values */
>+
>+#boundingbox-top {
>+  top: 14px;
>+  left: 48px;
>+  width: 40px;
>+  text-align: left!important;
>+}
>+
>+#boundingbox-left {
>+  top: 48px;
>+  left: 0px;
>+  width: 40px;
>+}
>+
>+#boundingbox-height {
>+  width: 25px;
>+  right: 0px;
>+}
>+
>+#boundingbox-width {
>+  bottom: 8px;
>+}
>+
>+.vertical #boundingbox-height {
>+  right: 20px;
>+}
>+
>+.vertical #boundingbox-top {
>+  left: 68px;
>+}
>+
>+.vertical #boundingbox-left {
>+  left: 20px;
>+}
>+
>+.horizontal #boundingbox-width {
>+  bottom: 28px;
>+}
>+
>+.horizontal #boundingbox-top {
>+  top: 34px;
>+}
>+
>+.horizontal #boundingbox-left {
>+  top: 68px;
>+}
[etc.]

Is this application logic or theme specific stuff?
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P2
Attachment #568706 - Flags: review?(dao)
This is likely going to need an update.
Attached patch patch v1.3 - rebased (obsolete) — Splinter Review
Attachment #568706 - Attachment is obsolete: true
Attachment #568706 - Flags: review?(rcampbell)
Depends on: 707809
Attached image mockup
No longer depends on: 707809
Attached patch patch 2 (obsolete) — Splinter Review
Todo:
- tests
- accesskey / tooltiptext
Depends on: 717922
Attachment #558439 - Attachment is obsolete: true
Attachment #557568 - Attachment is obsolete: true
Attachment #589869 - Attachment is obsolete: true
Attachment #601237 - Flags: feedback?(mratcliffe)
Comment on attachment 601237 [details] [diff] [review]
patch 2

I really like it, f+ with the external image addressed.

> background: url(http://i.imgur.com/Lyp1h.png), -moz-radial-gradient(50% 70%, circle cover, hsl(210,53%,45%) 0%, hsl(210,54%,33%) 100%);
We should probably keep external images to a minimum.

> this.iframe.setAttribute("style", "width: 240px; height: 155px");
Really, why not use a class? Or is this to keep our CSS reviewable within our module?
Attachment #601237 - Flags: feedback?(mratcliffe) → feedback+
Attached patch patch v2.1 (obsolete) — Splinter Review
Attachment #601237 - Attachment is obsolete: true
Attachment #601956 - Flags: review?(jwalker)
Attachment #601956 - Flags: feedback?(fayearthur)
Attached patch patch v2.1 (rebased) (obsolete) — Splinter Review
Attachment #601956 - Attachment is obsolete: true
Attachment #601956 - Flags: review?(jwalker)
Attachment #601956 - Flags: feedback?(fayearthur)
Attachment #602360 - Flags: review?(jwalker)
Attachment #602360 - Flags: feedback?(fayearthur)
Blocks: 718250
review/feedback ping
Comment on attachment 602360 [details] [diff] [review]
patch v2.1 (rebased)

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

::: browser/devtools/layoutview/LayoutView.jsm
@@ +67,5 @@
> +    this.highlighter.addListener("nodeselected", this.update);
> +    this.browser.addEventListener("MozAfterPaint", this.update, true);
> +
> +    this.map = {
> +      marginTop: [".margin.top > span", "margin-top"],

Please could you document the purpose of this map.
Also, wouldn't this be easier to understand?:

    marginTop: { selector: ".margin.top > span", property: "margin-top", value: undefined },
    ...

@@ +133,5 @@
> +
> +    popupset.appendChild(this.panel);
> +  },
> +
> +  onDocumentReady: function() {

Nit: onDocumentReady: function LV_onDocumentReady() {
and for the following 2 functions.

@@ +170,5 @@
> +    let node = this.highlighter.node;
> +    let clientRect = node.getBoundingClientRect();
> +
> +    let width = Math.round(clientRect.width);
> +    let height = Math.round(clientRect.height);

Hmm, if we've got fractional width/height, wouldn't we want to see it?

::: browser/devtools/shared/LayoutHelpers.jsm
@@ -212,5 @@
>  
>      return aRectScaled;
>    },
>  
> -

Did you mean to add this? The file is otherwise unchanged?
Attachment #602360 - Flags: review?(jwalker) → review+
Comment on attachment 602360 [details] [diff] [review]
patch v2.1 (rebased)

This looks really nice. One thing I found in practice is that it's hard to grok the values quickly. Maybe it's the font size or the background. One element I selected had a margin of 6 and it took a bit to see that it wasn't a 0.

Might be something worth iterating on later, but everything is correct, and looks nice.
Attachment #602360 - Flags: feedback?(fayearthur) → feedback+
Attached patch patch v2.2 (obsolete) — Splinter Review
Attachment #602360 - Attachment is obsolete: true
Comment on attachment 604908 [details] [diff] [review]
patch v2.2

Tim, can you review this? We add a button to browser.xul, so we need browser review.
Attachment #604908 - Flags: review?(ttaubert)
Thanks for the r+ Joe! I addressed your comments in attachment 604908 [details] [diff] [review].

(In reply to Joe Walker from comment #27)
> @@ +170,5 @@
> > +    let node = this.highlighter.node;
> > +    let clientRect = node.getBoundingClientRect();
> > +
> > +    let width = Math.round(clientRect.width);
> > +    let height = Math.round(clientRect.height);
> 
> Hmm, if we've got fractional width/height, wouldn't we want to see it?

I don't know about this yet. If needed, I'll fix that in a follow-up bug.
(In reply to Paul Rouget [:paul] from comment #30)
> Comment on attachment 604908 [details] [diff] [review]
> patch v2.2
> 
> Tim, can you review this? We add a button to browser.xul, so we need browser
> review.

What's the keyboard access story for this button?
(In reply to Dão Gottwald [:dao] from comment #32)
> (In reply to Paul Rouget [:paul] from comment #30)
> > Comment on attachment 604908 [details] [diff] [review]
> > patch v2.2
> > 
> > Tim, can you review this? We add a button to browser.xul, so we need browser
> > review.
> 
> What's the keyboard access story for this button?

There's no accesskey for this button. There's no text to show the accesskey and we can't build an accurate tooltip dynamically. See bug 717923 comment 24. Let me know if you think this is a problem, because this also affects bug 717922.
Comment on attachment 604908 [details] [diff] [review]
patch v2.2

Yes, this is a problem. If not a shortcut key, making the button tabbable is the minimum to satisfy our accessibility principles.
Attachment #604908 - Flags: review?(ttaubert) → review-
(In reply to Dão Gottwald [:dao] from comment #34)
> Yes, this is a problem. If not a shortcut key, making the button tabbable is
> the minimum to satisfy our accessibility principles.

However, please think hard whether you really want to meet the minimum bar only in this case. For frequently used actions, you really want shortcut or accelerator keys.
(In reply to Dão Gottwald [:dao] from comment #35)
> (In reply to Dão Gottwald [:dao] from comment #34)
> > Yes, this is a problem. If not a shortcut key, making the button tabbable is
> > the minimum to satisfy our accessibility principles.
> 
> However, please think hard whether you really want to meet the minimum bar
> only in this case. For frequently used actions, you really want shortcut or
> accelerator keys.

You're right. We need to come with an efficient way to toggle the Layout View from the keyboard as it's an important feature.
triage, filter on centaur
Whiteboard: [minotaur] → [minotaur][firefox14-wanted]
Assignee: paul → nobody
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
Assignee: nobody → paul
We want the Layout View to be part of the Sidebar. So this invalidate this current work.
Depends on: 707809
No longer depends on: 717922
Attachment #604908 - Attachment is obsolete: true
Todo (follow-ups):
* what to do for margin:auto?
* we need proper icons
* we need a keyboard shortcut
* if the view has the focus when the sidebar is closed, the inspector keyboard shortcuts won't work (the content needs to get the focus back)
Comment on attachment 615886 [details] [diff] [review]
patch v3 - layout view in sidebar

Dave, the CSS code has already been reviewed previously by Joe. Focus on the JSM and on the way I register/destroy the layoutview.

Also, I don't touch any browser/ related code.

Thanks.
Attachment #615886 - Flags: review?(dcamp)
Comment on attachment 615886 [details] [diff] [review]
patch v3 - layout view in sidebar

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

JS looks fine to me, a few comments below.

Please file a followup for preffing on, and make sure it gets a ui-r+ before turning it on.

::: browser/devtools/layoutview/LayoutView.jsm
@@ +66,5 @@
> +
> +    // Is the layout view was open before?
> +    // If there's nothing in the store, then it's the first time
> +    // it is open, we fallback to the preference.
> +    if (this.inspector._getStoreValue("layoutview-is-open") == null) {

Sorry, I just broke this - _getStoreValue is removed.  Just add a property to this.inspector.

@@ +79,5 @@
> +      this.undim();
> +      this.update();
> +      // We make sure we never add 2 listeners.
> +      if (!this.trackingPaint) {
> +        this.browser.addEventListener("MozAfterPaint", this.update, true);

Do you think the inspector object should do something to help here?  Some sort of "element display may have changed, please refresh yourself" signal rather than digging through chomeWindow->gBrowser->MozAfterPaint?

Just a thought...

@@ +99,5 @@
> +    // Build the layout view in the sidebar.
> +    this.buildView();
> +
> +    // Get messages from the iframe.
> +    this.inspector.chromeWindow.addEventListener("message", this.onMessage, true);

Is there a reason you're preferring using message handling here?  Chrome docs can talk to other chrome docs, so you could just provide a reference to your message handling function directly to the frame.

Just curious, I'm not opposed to it.
Attachment #615886 - Flags: review?(dcamp) → review+
(In reply to Dave Camp (:dcamp) from comment #43)

> Please file a followup for preffing on, and make sure it gets a ui-r+ before
> turning it on.

Make sure that bug blocks on a keybinding too.
Attached patch patch v3.1Splinter Review
Addressed Dave's comments.
Attachment #615886 - Attachment is obsolete: true
Bug 747218 - [layout view] Pref on the layout view
https://tbpl.mozilla.org/?tree=Fx-Team&rev=dde2fce2058a
Whiteboard: [minotaur][firefox14-wanted] → [minotaur][firefox14-wanted][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dde2fce2058a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][firefox14-wanted][fixed-in-fx-team] → [minotaur][firefox14-wanted]
Target Milestone: --- → Firefox 14
Depends on: 747919
this has now been enabled for Firefox 15 (I can't change the status-firefox15 bit, but I'm sure akeybl can!)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.