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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Paul Rouget [:paul]]

This view should live into a Panel.

Comment 1

[Paul Rouget [:paul]]

Attachment: Possible design

Comment 2

[Paul Rouget [:paul]]

See bug 663778 for a preliminary patch.

Comment 3

[Paul Rouget [:paul]]

Depends on bug 663831 because the tool will register itself in initTools.

Comment 4

[Paul Rouget [:paul]]

Attachment: 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]]

Attachment: panel bug (document not completly refreshed)

Comment 6

[Paul Rouget [:paul]]

Attachment: patch v0.2 WIP rebase (new register tool API)

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

Attachment: patch v0.3 WIP rebase

Comment 9

[Paul Rouget [:paul]]

Attachment: 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)]

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]]

Attachment: 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]]

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]]

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]]

Attachment: patch v1.2

Addressed Dao's comments.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 568706 [details] [diff] [review]
patch v1.2

removing checkin flag until review is complete.

Comment 16

[Dão Gottwald [: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 17

[Dão Gottwald [:dao]]

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

We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

Comment 19

[Rob Campbell [:rc] (:robcee)]

This is likely going to need an update.

Comment 20

[Paul Rouget [:paul]]

Attachment: patch v1.3 - rebased

Comment 21

[Paul Rouget [:paul]]

Attachment: mockup

Comment 22

[Paul Rouget [:paul]]

Attachment: patch 2

Todo:
- tests
- accesskey / tooltiptext

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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]]

Attachment: patch v2.1

Comment 25

[Paul Rouget [:paul]]

Attachment: patch v2.1 (rebased)

Comment 26

[Paul Rouget [:paul]]

review/feedback ping

Comment 27

[Joe Walker [:jwalker]]

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]]

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]]

Attachment: patch v2.2

Comment 30

[Paul Rouget [:paul]]

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]]

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]]

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

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

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]]

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

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

triage, filter on centaur

Comment 39

[Paul Rouget [:paul]]

We want the Layout View to be part of the Sidebar. So this invalidate this current work.

Comment 40

[Paul Rouget [:paul]]

Attachment: patch v3 - layout view in sidebar

Comment 41

[Paul Rouget [:paul]]

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]]

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

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

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

Attachment: patch v3.1

Addressed Dave's comments.

Comment 46

[Paul Rouget [:paul]]

Bug 747218 - [layout view] Pref on the layout view

Comment 47

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=ccead923d5b6

Comment 48

[Dave Camp (:dcamp)]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=dde2fce2058a

Comment 49

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/dde2fce2058a

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/dde2fce2058a

Comment 52

[Kevin Dangoor]

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]]

Documented: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Box_model_view, so marking as dev-doc-complete.