Last Comment Bug 683954 - [Layout] Implement an abstract view of the layout of the selected node
: [Layout] Implement an abstract view of the layout of the selected node
Status: RESOLVED FIXED
[minotaur][firefox14-wanted]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: Firefox 14
Assigned To: Paul Rouget [:paul]
:
:
Mentors:
: 674887 (view as bug list)
Depends on: 663831 674871 707809 747919
Blocks: 663778 663830 674887
  Show dependency treegraph
 
Reported: 2011-09-01 10:35 PDT by Paul Rouget [:paul]
Modified: 2014-12-19 09:23 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
fixed


Attachments
Possible design (69.63 KB, image/png)
2011-09-01 10:41 PDT, Paul Rouget [:paul]
no flags Details
patch v0.1 WIP (28.99 KB, patch)
2011-09-06 03:47 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
panel bug (document not completly refreshed) (54.56 KB, image/png)
2011-09-06 03:48 PDT, Paul Rouget [:paul]
no flags Details
patch v0.2 WIP rebase (new register tool API) (29.77 KB, patch)
2011-09-09 05:23 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.3 WIP rebase (35.13 KB, patch)
2011-10-10 06:45 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1 (64.46 KB, patch)
2011-10-18 09:02 PDT, Paul Rouget [:paul]
rcampbell: review-
Details | Diff | Splinter Review
patch v1.1 (65.94 KB, patch)
2011-10-19 06:09 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1.2 (68.22 KB, patch)
2011-10-21 10:42 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.3 - rebased (70.63 KB, patch)
2012-01-19 07:43 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
mockup (312.82 KB, image/jpeg)
2012-01-23 05:32 PST, Paul Rouget [:paul]
no flags Details
patch 2 (27.57 KB, patch)
2012-02-28 04:31 PST, Paul Rouget [:paul]
mratcliffe: feedback+
Details | Diff | Splinter Review
patch v2.1 (62.50 KB, patch)
2012-03-01 07:10 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.1 (rebased) (62.52 KB, patch)
2012-03-02 07:27 PST, Paul Rouget [:paul]
jwalker: review+
fayearthur: feedback+
Details | Diff | Splinter Review
patch v2.2 (63.16 KB, patch)
2012-03-12 06:40 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v3 - layout view in sidebar (70.50 KB, patch)
2012-04-17 14:43 PDT, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Splinter Review
patch v3.1 (69.75 KB, patch)
2012-04-19 15:46 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-09-01 10:35:19 PDT
This view should live into a Panel.
Comment 1 Paul Rouget [:paul] 2011-09-01 10:41:17 PDT
Created attachment 557568 [details]
Possible design
Comment 2 Paul Rouget [:paul] 2011-09-01 10:44:09 PDT
See bug 663778 for a preliminary patch.
Comment 3 Paul Rouget [:paul] 2011-09-02 07:40:00 PDT
Depends on bug 663831 because the tool will register itself in initTools.
Comment 4 Paul Rouget [:paul] 2011-09-06 03:47:03 PDT
Created attachment 558438 [details] [diff] [review]
patch v0.1 WIP

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)
Comment 5 Paul Rouget [:paul] 2011-09-06 03:48:31 PDT
Created attachment 558439 [details]
panel bug (document not completly refreshed)
Comment 6 Paul Rouget [:paul] 2011-09-09 05:23:24 PDT
Created attachment 559427 [details] [diff] [review]
patch v0.2 WIP rebase (new register tool API)
Comment 7 Panos Astithas [:past] 2011-09-29 00:21:29 PDT
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.
Comment 8 Paul Rouget [:paul] 2011-10-10 06:45:26 PDT
Created attachment 565927 [details] [diff] [review]
patch v0.3 WIP rebase
Comment 9 Paul Rouget [:paul] 2011-10-18 09:02:42 PDT
Created attachment 567764 [details] [diff] [review]
patch v1

The View is included in a <panel>. This is a temporary situtation. It will be
moved to a better place in bug 674887.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-10-18 11:03:43 PDT
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.
Comment 11 Paul Rouget [:paul] 2011-10-19 06:09:29 PDT
Created attachment 568023 [details] [diff] [review]
patch v1.1

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?
Comment 12 Paul Rouget [:paul] 2011-10-19 06:43:36 PDT
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!
Comment 13 Dão Gottwald [:dao] 2011-10-20 13:01:43 PDT
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?
Comment 14 Paul Rouget [:paul] 2011-10-21 10:42:18 PDT
Created attachment 568706 [details] [diff] [review]
patch v1.2

Addressed Dao's comments.
Comment 15 Rob Campbell [:rc] (:robcee) 2011-10-21 10:50:42 PDT
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

removing checkin flag until review is complete.
Comment 16 Dão Gottwald [:dao] 2011-10-24 12:32:42 PDT
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 17 Dão Gottwald [:dao] 2011-10-24 12:38:48 PDT
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?
Comment 18 Dave Camp (:dcamp) 2011-10-27 09:13:10 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 19 Rob Campbell [:rc] (:robcee) 2011-12-13 13:21:05 PST
This is likely going to need an update.
Comment 20 Paul Rouget [:paul] 2012-01-19 07:43:48 PST
Created attachment 589869 [details] [diff] [review]
patch v1.3 - rebased
Comment 21 Paul Rouget [:paul] 2012-01-23 05:32:55 PST
Created attachment 590681 [details]
mockup
Comment 22 Paul Rouget [:paul] 2012-02-28 04:31:43 PST
Created attachment 601237 [details] [diff] [review]
patch 2

Todo:
- tests
- accesskey / tooltiptext
Comment 23 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-28 07:22:13 PST
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?
Comment 24 Paul Rouget [:paul] 2012-03-01 07:10:53 PST
Created attachment 601956 [details] [diff] [review]
patch v2.1
Comment 25 Paul Rouget [:paul] 2012-03-02 07:27:35 PST
Created attachment 602360 [details] [diff] [review]
patch v2.1 (rebased)
Comment 26 Paul Rouget [:paul] 2012-03-08 09:45:56 PST
review/feedback ping
Comment 27 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-03-09 01:39:49 PST
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?
Comment 28 Heather Arthur [:harth] 2012-03-09 12:58:02 PST
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.
Comment 29 Paul Rouget [:paul] 2012-03-12 06:40:11 PDT
Created attachment 604908 [details] [diff] [review]
patch v2.2
Comment 30 Paul Rouget [:paul] 2012-03-12 06:48:56 PDT
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.
Comment 31 Paul Rouget [:paul] 2012-03-12 06:51:23 PDT
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.
Comment 32 Dão Gottwald [:dao] 2012-03-12 07:09:19 PDT
(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?
Comment 33 Paul Rouget [:paul] 2012-03-12 07:39:36 PDT
(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 34 Dão Gottwald [:dao] 2012-03-12 08:25:58 PDT
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.
Comment 35 Dão Gottwald [:dao] 2012-03-12 09:32:50 PDT
(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.
Comment 36 Paul Rouget [:paul] 2012-03-12 10:14:14 PDT
(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.
Comment 37 Paul Rouget [:paul] 2012-03-14 09:12:24 PDT
triage, filter on centaur
Comment 38 Paul Rouget [:paul] 2012-03-14 09:57:16 PDT
*** Bug 718250 has been marked as a duplicate of this bug. ***
Comment 39 Paul Rouget [:paul] 2012-04-05 12:27:54 PDT
We want the Layout View to be part of the Sidebar. So this invalidate this current work.
Comment 40 Paul Rouget [:paul] 2012-04-17 14:43:45 PDT
Created attachment 615886 [details] [diff] [review]
patch v3 - layout view in sidebar
Comment 41 Paul Rouget [:paul] 2012-04-17 14:45:55 PDT
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 42 Paul Rouget [:paul] 2012-04-17 14:50:10 PDT
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.
Comment 43 Dave Camp (:dcamp) 2012-04-19 14:46:00 PDT
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.
Comment 44 Dave Camp (:dcamp) 2012-04-19 15:00:20 PDT
(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.
Comment 45 Paul Rouget [:paul] 2012-04-19 15:46:12 PDT
Created attachment 616782 [details] [diff] [review]
patch v3.1

Addressed Dave's comments.
Comment 46 Paul Rouget [:paul] 2012-04-19 15:56:02 PDT
Bug 747218 - [layout view] Pref on the layout view
Comment 47 Paul Rouget [:paul] 2012-04-19 16:03:06 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ccead923d5b6
Comment 48 Dave Camp (:dcamp) 2012-04-20 14:36:52 PDT
https://tbpl.mozilla.org/?tree=Fx-Team&rev=dde2fce2058a
Comment 50 Tim Taubert [:ttaubert] 2012-04-20 23:38:07 PDT
https://hg.mozilla.org/mozilla-central/rev/dde2fce2058a
Comment 51 Paul Rouget [:paul] 2012-05-09 08:30:28 PDT
*** Bug 674887 has been marked as a duplicate of this bug. ***
Comment 52 Kevin Dangoor 2012-06-01 07:26:17 PDT
this has now been enabled for Firefox 15 (I can't change the status-firefox15 bit, but I'm sure akeybl can!)
Comment 53 Will Bamberg [:wbamberg] 2014-12-19 09:23:55 PST
Documented: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Box_model_view, so marking as dev-doc-complete.

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