DevTools Inspector: Box Model should be editable

VERIFIED FIXED in Firefox 31

Status

()

Firefox
Developer Tools: Inspector
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: sys.sgx, Assigned: mossop)

Tracking

Trunk
Firefox 31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Hardware: x86 → All
(Reporter)

Comment 1

5 years ago
Created attachment 724084 [details]
Box Model image

Updated

5 years ago
Component: Untriaged → Developer Tools: Inspector
(Reporter)

Comment 2

4 years ago
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.
OS: Windows Vista → All
Version: 22 Branch → Trunk

Comment 3

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → dtownsend+bugmail
(Assignee)

Comment 4

4 years ago
Created attachment 8379936 [details] [diff] [review]
patch1

This applies on top of the patch in bug 663778
(Assignee)

Updated

4 years ago
Depends on: 663778
(Assignee)

Updated

4 years ago
Blocks: 974639
(Assignee)

Comment 5

4 years ago
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.
Attachment #8379936 - Flags: feedback?(pbrosset)
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.
Attachment #8379936 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 8391472 [details] [diff] [review]
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.
Attachment #8379936 - Attachment is obsolete: true
Attachment #8391472 - Flags: review?(pbrosset)
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
Attachment #8391472 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 9

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

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

Comment 10

4 years ago
Created attachment 8393100 [details] [diff] [review]
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.
Attachment #8391472 - Attachment is obsolete: true
Attachment #8393100 - Flags: review?(pbrosset)
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.
Attachment #8393100 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 12

4 years ago
Let's see if it sticks this time... https://hg.mozilla.org/integration/fx-team/rev/ed03ee30b1b2
(Assignee)

Comment 13

4 years ago
Nope, backed out again in https://hg.mozilla.org/integration/fx-team/rev/4961e0e7bba1
(Assignee)

Comment 14

3 years ago
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
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]"
(Assignee)

Comment 16

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/e3564d9cd015
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Please confirm that this is fixed for you in the latest Beta build, http://beta.mozilla.org
QA Whiteboard: [qa-]
Flags: needinfo?(sys.sgx)
(Reporter)

Comment 19

3 years ago
@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.
Flags: needinfo?(sys.sgx)
Thanks, please file your remaining issues in follow-up bug reports.
Status: RESOLVED → VERIFIED
status-firefox31: --- → verified
You need to log in before you can comment on or make changes to this bug.