850336 - DevTools Inspector: Box Model should be editable

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[sys.sgx]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:22.0) Gecko/20130312 Firefox/22.0
Build ID: 20130312031046

Steps to reproduce:

Open a page and then the devtools.
Click the Box Model.
Paul, there should be an easy way to edit the padding, margin, line width, etc. through the Box Model, by double clicking on the values in the Box.

Also, as seen in the screenshot, at least on windows builds, the text has a weired font which is very small. IMHO, the entire box should become quite bigger, the font something more stylish and bigger, and the total dimensions (green box) should move and go to where the red box is. This way you'll also save some space above the Box Model.

Comment 1

[sys.sgx]

Attachment: Box Model image

Comment 2

[sys.sgx]

I would like to propose the following:
Instead of displaying a simple dimensions number above the box model, as shown in the image, we could better have something like "Core width: x1", "Full width: x2", and then the box model should follow.

Plus, the font size should increase quite a bit, and of course the box model should be editable.

Comment 3

[gossi]

I would like to request support for the arrow keys to increment/decrement values as it is common behavior in other tools like firebug of chrome devtools.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch1

This applies on top of the patch in bug 663778

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8379936 [details] [diff] [review]
patch1

This can't get a full review until Mike is done in bug 663778 but it might be worth a feedback pass to spot any obvious errors. I've not really touched devtools before so there may be stuff I've missed.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8379936 [details] [diff] [review]
patch1

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

Thanks for the patch. This is a much needed feature.
The patch doesn't apply on fx-team anymore, might need some rebasing.
Could you upload a new patch so I can quickly play with it?

See my comments below.

::: browser/devtools/layoutview/test/browser_editablemodel.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

General comment about this test: it's getting a bit long and may cause timeout problems later during try builds, if we keep adding to it.
Because it's already running several different async test cases, it would be easy to split in many, smaller, test files. This way it would give us the chance to have a nice head.js that contains all the repetitive stuff, and will make sure all test files are small, fast to run and easier to debug.
Can you do this?

@@ +59,5 @@
> +      });
> +    });
> +  }
> +
> +  function selectNode(aNode, aCallback) {

Get rid of the callback parameter, it's not used as the function returns a promise.
Also, I think the function could be slightly better written as:

function selectNode(aNode) {
  let onSelect = inspector.once("inspector-updated");
  inspector.selection.setNode(aNode, "test");
  return onSelect.then(() => {
    view = inspector.sidebar.getWindowForTab("layoutview");
    ok(!!view.layoutview, "LayoutView document is alive.");
  });
}

@@ +62,5 @@
> +
> +  function selectNode(aNode, aCallback) {
> +    let deferred = promise.defer();
> +
> +    inspector.selection.setNode(aNode);

Please use `inspector.selection.setNode(aNode, "test")`
This avoids highlighting the node in the page on new selection (therefore avoids an extra server request which can easily become problematic when the test ends).

@@ +73,5 @@
> +
> +    return deferred.promise;
> +  }
> +
> +  function waitForUpdate() {

`once` now returns a promise when no callback is passed, so can be re-written as:

function waitForUpdate() {
  return inspector.once("layoutview-updated");
}

And since it's one line, a new function might not even be needed anymore.

@@ +91,5 @@
> +  });
> +
> +  let TESTS = [
> +    // Test that editing margin dynamically updates the document, pressing escape
> +    // cancels the changes

These comments are very useful, so much that I would actually put them into `info("...")` instead, making it easier to follow along when reading logs.

::: browser/devtools/layoutview/view.js
@@ +18,5 @@
>  const promise = devtools.require("sdk/core/promise");
> +const {InplaceEditor, editableItem} = devtools.require("devtools/shared/inplace-editor");
> +const {parseDeclarations} = devtools.require("devtools/styleinspector/css-parsing-utils");
> +
> +const NUMERIC = /^-?[\d\.]+$/;

I think `\` isn't required before `.` in a `[]` range.

@@ +20,5 @@
> +const {parseDeclarations} = devtools.require("devtools/styleinspector/css-parsing-utils");
> +
> +const NUMERIC = /^-?[\d\.]+$/;
> +
> +function EditingSession(rules) {

This new class could use some jsdoc comments. The name is pretty self-explanatory, but still, it would help having some comments detailing the why and how.

@@ +27,5 @@
> +}
> +
> +EditingSession.prototype = {
> +  getPropertyFromRule: function(rule, property) {
> +    let dummyElement = document.getElementById("dummy");

The fact that view.js is a toolbox "panel" means that it runs in a page rather than being loaded as a module, and therefore `document` exists.
Nevertheless, since this is a pretty generic class that could be extracted as a separate module, or reused by other modules, I wouldn't use the globally available `document` object here and instead have it being passed as a parameter in the `EditingSession` constructor.

@@ +35,5 @@
> +    return dummyStyle.getPropertyValue(property);
> +  },
> +
> +  getProperty: function(property) {
> +    for (let rule of this._rules) {

If I understand this correctly, this works because the rules returned by `getApplied` are sorted by priority order, so you just need to loop on these rules and stop at the first one that contain the property.
Is that right?
If yes, I think this requires a bit of documentation.

@@ +56,5 @@
> +      else
> +        modifications.setProperty(property.name, property.value, "");
> +    }
> +
> +    modifications.apply().then(null, console.error);

Please add `return` at the beginning of this line so that consumers of the setProperties function can use the returned promise too.

@@ +69,5 @@
> +      else
> +        modifications.removeProperty(property);
> +    }
> +
> +    modifications.apply().then(null, console.error);

Please add `return` at the beginning of this line so that consumers of the revert function can use the returned promise too.

@@ +132,4 @@
>                    value: undefined},
>        paddingRight: {selector: ".padding.right > span",
>                    property: "padding-right",
> +                  realProperty: "padding-right-value",

Cam you explain why top and bottom do not require this new `realProperty` property? I might be missing something here.

@@ +154,5 @@
> +
> +      let property = this.map[i].property;
> +      let realProperty = this.map[i].realProperty || property;
> +      let selector = this.map[i].selector;
> +      editableItem({ element: this.doc.querySelector(selector) }, (element, event) => {

I'm not familiar with this function yet, will the callback be called the next time a node is selected?
Because `editableItem` is called at `init()` time, and because its name isn't really self-explanatory, it looks as though editing won't work after a new node is selected.
What would help though is extracting your callback to a new method in the class, which would have a name that explains this.
This would reduce the size of `init` and make it easier to understand.

@@ +259,5 @@
>    },
>  
>    /**
>     * Compute the dimensions of the node and update the values in
>     * the layoutview/view.xhtml document.

The function now returns a promise, this should be documented in the jsdoc.

@@ +341,5 @@
>        if (this.sizeLabel.textContent != newValue) {
>          this.sizeLabel.textContent = newValue;
>        }
>  
> +      let entries = yield this.inspector.pageStyle.getApplied(node, {});

By the time this promise resolves, the current node selection may have been changed by the user.
We should cache the current selection before making the call and compare to the actual current selection after it resolves, and if they differ, just return.

::: browser/devtools/layoutview/view.xhtml
@@ +60,5 @@
>  
>      </div>
>  
> +    <div style="display: none">
> +      <p id="dummy"></p>

`EditingSession` should be the one dynamically creating this element, so it can easily be reused elsewhere if needed further down the track.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch 2

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Comment on attachment 8379936 [details] [diff] [review]
> patch1
> 
> Review of attachment 8379936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. This is a much needed feature.
> The patch doesn't apply on fx-team anymore, might need some rebasing.
> Could you upload a new patch so I can quickly play with it?

I waited until the highlight changes landed so this patch will now apply by itself. I've addressed everything but see some notes below

> @@ +35,5 @@
> > +    return dummyStyle.getPropertyValue(property);
> > +  },
> > +
> > +  getProperty: function(property) {
> > +    for (let rule of this._rules) {
> 
> If I understand this correctly, this works because the rules returned by
> `getApplied` are sorted by priority order, so you just need to loop on these
> rules and stop at the first one that contain the property.
> Is that right?
> If yes, I think this requires a bit of documentation.

That is my belief and what I saw happening but it might be useful for someone who knows more about how that actor works answer to be sure. Who would that be?

> @@ +132,4 @@
> >                    value: undefined},
> >        paddingRight: {selector: ".padding.right > span",
> >                    property: "padding-right",
> > +                  realProperty: "padding-right-value",
> 
> Cam you explain why top and bottom do not require this new `realProperty`
> property? I might be missing something here.

I've added some notes but basically there are some internal properties that are being hidden here.

> @@ +154,5 @@
> > +
> > +      let property = this.map[i].property;
> > +      let realProperty = this.map[i].realProperty || property;
> > +      let selector = this.map[i].selector;
> > +      editableItem({ element: this.doc.querySelector(selector) }, (element, event) => {
> 
> I'm not familiar with this function yet, will the callback be called the
> next time a node is selected?
> Because `editableItem` is called at `init()` time, and because its name
> isn't really self-explanatory, it looks as though editing won't work after a
> new node is selected.
> What would help though is extracting your callback to a new method in the
> class, which would have a name that explains this.
> This would reduce the size of `init` and make it easier to understand.

editableItem makes the span in the layoutview clickable and they remain editable no matter whether new nodes are selected. I've added some comments to that effect and split out the callback as you suggest.

> @@ +341,5 @@
> >        if (this.sizeLabel.textContent != newValue) {
> >          this.sizeLabel.textContent = newValue;
> >        }
> >  
> > +      let entries = yield this.inspector.pageStyle.getApplied(node, {});
> 
> By the time this promise resolves, the current node selection may have been
> changed by the user.
> We should cache the current selection before making the call and compare to
> the actual current selection after it resolves, and if they differ, just
> return.

Good catch, that turns out to be the source of intermittent test failures in my test. I've moved this call to before the lastRequest check which solves the problem.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8391472 [details] [diff] [review]
patch 2

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

Looking good to me.
Just a couple of console.log that need to be removed.

::: browser/devtools/layoutview/view.js
@@ +43,5 @@
> +   * @param rule      The CSS rule
> +   * @param property  The name of the property
> +   */
> +  getPropertyFromRule: function(rule, property) {
> +    console.log("Testing " + rule.cssText);

Remove this line

@@ +57,5 @@
> +   *
> +   * @param property  The name of the property as a string
> +   */
> +  getProperty: function(property) {
> +    console.log("Finding " + property);

Remove this line

Comment 9

[Dave Townsend [:mossop]]

Obviously I need to get better at scanning my patches for debugging bits before uploading!

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

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch 3

Backed out because while I did the work to help split up the test file I forgot to do so and as you predicted it ran too long and timed out. This is the same just splitting the parts of the test across a few different files.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8393100 [details] [diff] [review]
patch 3

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

Interdiff tells me the only difference is the couple of console.log statements you removed and the 3 new tests you created. Is that right?
If that's so, then I'm fine with this patch.
I have a couple of comments about the tests that apply to all 3 test files.

::: browser/devtools/layoutview/test/browser_editablemodel_allproperties.js
@@ +7,5 @@
> +
> +let doc;
> +let inspector;
> +
> +let test = Task.async(function*() {

I would extract this part as well as the `gBrowser.removeCurrentTab()` and `finish()` to head.js. It could look something like:

let test = asyncTest(function*() {
  // ... test content here ...
});

@@ +8,5 @@
> +let doc;
> +let inspector;
> +
> +let test = Task.async(function*() {
> +  waitForExplicitFinish();

I would move this line into head.js, somewhere at the start, accompanied with a comment that says "all tests are async" or something like this, because, honestly, I haven't yet seen any test that isn't using this.

Comment 12

[Dave Townsend [:mossop]]

Let's see if it sticks this time... https://hg.mozilla.org/integration/fx-team/rev/ed03ee30b1b2

Comment 13

[Dave Townsend [:mossop]]

Nope, backed out again in https://hg.mozilla.org/integration/fx-team/rev/4961e0e7bba1

Comment 14

[Dave Townsend [:mossop]]

Switched the event that selectNode waits for and that seems to solve the problem on try:

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

Comment 15

[Phil Ringnalda (:philor)]

Aaand backed out in https://hg.mozilla.org/integration/fx-team/rev/560143719f60 for linux debug bc2 "browser_markupview_copy_image_data.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/layoutview/view.xhtml]"

Comment 16

[Dave Townsend [:mossop]]

Fixed by resetting the sidebar after each test and re-landed hopefully it won't cause tests to fail randomly...

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

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[u279076]

Please confirm that this is fixed for you in the latest Beta build, http://beta.mozilla.org

Comment 19

[sys.sgx]

@ashughes:
It works on Nightly 33a1. :)
A few things I would like to be incorporated though are:
- Possible modification of the outer box (margin) color to something that better matches the color of the inner boxes.
- Please allow for more pixels as click area for the values in the Edit Box. Currently to change the values in the Edit Box, the user has to click almost exactly on the previous value; since there is no other text to be insterted in the boxes, allow more pixels left and right to be able to be clicked for the change value action.

Comment 20

[u279076]

Thanks, please file your remaining issues in follow-up bug reports.