1061823 - Allow to edit width and height in the box model

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Following on from bug 850336, we should implement the ability to manually input width/height for elements where that makes sense, eg display: block ( but nont :none or :inline ):

* I should be able to manually set the width and height in the box model panel
* I should see live updates
* I should not be able to set width/height for elements where it would have no visual effect, OR
* I should get visual feedback of why width/height adjustments are not having any visual effect

Reference:

https://twitter.com/JohnBristowe/status/506251631474642945

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Recently suggested on uservoice as well: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/7076113-assept-to-edit-sizes-of-blocks

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Adding this, not because I want it in 40 but because I remembered this bug existed today and want it prioritized in the backlog.

Comment 3

[Daniel Moore]

Would love to see this feature. I could be remembering wrong, but I believe what Firebug does is add manual width & height changes from the box model as element-specific, inline styling. Also, I'm not sure if Chrome's Dev Tools or Firebug do your bullets 3 & 4. That seems fairly more in-depth.

Comment 4

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v1

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8843883 [details] [diff] [review]
edit_width_height.patch v1

Needs to be rebased. All the boxmodel actually moved under boxmodel/ a couple of weeks ago.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8843883 [details] [diff] [review]
edit_width_height.patch v1

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

::: devtools/client/inspector/layout/components/BoxModelMain.js
@@ +153,5 @@
>              },
> +            dom.span(
> +              {
> +                className: "boxmodel-legend",
> +                "datta-box": "content",

s/datta-box/data-box

Comment 7

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v2

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8844350 [details] [diff] [review]
edit_width_height.patch v2

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

We only want to make the width and height editable.

::: devtools/client/inspector/boxmodel/components/BoxModelEditable.js
@@ +8,5 @@
>    require("devtools/client/shared/vendor/react");
>  const { editableItem } = require("devtools/client/shared/inplace-editor");
>  
>  const LONG_TEXT_ROTATE_LIMIT = 3;
> +const CONTENT_LONG_TEXT_ROTATE_LIMIT = 4;

I don't think we want to ever rotate the width and height.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +166,5 @@
>                className: "boxmodel-paddings",
>                "data-box": "padding",
>                title: BOXMODEL_L10N.getStr("boxmodel.padding"),
>              },
> +            dom.span(

We don't want to add a content legend.

Comment 9

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v3

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8844852 [details] [diff] [review]
edit_width_height.patch v3

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

::: devtools/client/inspector/boxmodel/components/BoxModelEditable.js
@@ +46,2 @@
>      let rotate = (direction == "left" || direction == "right") &&
> +                 box != "content" &&

We should be able to remove this check if we make direction not a requirement.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +166,5 @@
>                className: "boxmodel-paddings",
>                "data-box": "padding",
>                title: BOXMODEL_L10N.getStr("boxmodel.padding"),
>              },
> +            // dom.span(

Remove this.

@@ +268,5 @@
>          onShowBoxModelEditor,
>        }),
> +      BoxModelEditable({
> +        box: "content",
> +        direction: "left",

So, I can see direction is actually making this complicated, but I don't think we should be needing direction for this. 

I would remove direction for both of these content editables. You will need to remove isRequired from the PropTypes for direction. I would see if we can keep the position of the text/editables if we kept it bound by the originally dom.p({ className: "boxmodel-size" }) instead of manually setting its top/left/right position. Also, we want to keep the same string format {width}x{height}.

Comment 11

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v4

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8846511 [details] [diff] [review]
edit_width_height.patch v4

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

We are getting very close. I would like to see the boxmodel-content width and edit be more centered. Specifically the 'x' should be exactly centered. You need to squash your changes since BoxModelEditable appears 3 times.

::: devtools/client/inspector/boxmodel/components/BoxModelEditable.js
@@ +41,5 @@
>        property,
>        textContent,
>      } = this.props;
>  
> +    //Rotate BoxModelEditable field if text content is greater than the appropriate limit.

This comment can be removed.

@@ +46,1 @@
>      let rotate = (direction == "left" || direction == "right") &&

We need to do a check for direction since it is not always required and nullable.

@@ +46,2 @@
>      let rotate = (direction == "left" || direction == "right") &&
> +                 box != "content" &&

This can be removed.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +166,5 @@
>                className: "boxmodel-paddings",
>                "data-box": "padding",
>                title: BOXMODEL_L10N.getStr("boxmodel.padding"),
>              },
> +            // dom.span(

This can be removed.

::: devtools/client/themes/boxmodel.css
@@ +182,5 @@
>  .boxmodel-padding.boxmodel-left {
>    width: 21px;
>  }
>  
> +.boxmodel-content.boxmodel-right,

.boxmodel-content.boxmodel-left/right are never used

Comment 13

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v5

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8847014 [details] [diff] [review]
edit_width_height.patch v5

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

I think the changes look good, but you are still squashing the commits incorrectly. The patch doesn't apply cleanly on top of the current HEAD. You still need to squash all your commits since it seems like you only squashed the last 2 of 3 commits.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +24,5 @@
>    displayName: "BoxModelMain",
>  
>    propTypes: {
>      boxModel: PropTypes.shape(Types.boxModel).isRequired,
> +    onHideBoxModelHighlighter: PropTypes.func,

I don't think we need to change this.

Comment 15

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v6

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8847455 [details] [diff] [review]
edit_width_height.patch v6

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

This patch needs to be rebased. I rebased it locally, but was unable to get the editable fields to work when I click width or height. So, there might be still some lingering issues, but I know it used to work so hopefully you can resolve this.

The width and height still aren't perfectly vertically aligned such that it is in the centered in the boxmodel-size <p>.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +24,5 @@
>    displayName: "BoxModelMain",
>  
>    propTypes: {
>      boxModel: PropTypes.shape(Types.boxModel).isRequired,
> +    onHideBoxModelHighlighter: PropTypes.func,

Why did we remove the isRequired?

@@ +266,5 @@
> +        BoxModelEditable({
> +          box: "content",
> +          property: "width",
> +          textContent: width,
> +          onShowBoxModelEditor: onShowBoxModelEditor

This can just be onShowBoxModelEditor,

@@ +276,5 @@
> +        BoxModelEditable({
> +          box: "content",
> +          property: "height",
> +          textContent: height,
> +          onShowBoxModelEditor: onShowBoxModelEditor

This can just be onShowBoxModelEditor,

Comment 17

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v6a

Can't seem to reproduce the issues you Gabriel regarding the unclickable width/height fields and the off-centre width/height values. Are there specific steps to take to reproduce them?

Fixed the minor comments you made.

Comment 18

[Sebastian Zartner [:sebo]]

> Blocks: 150496

Obviously the wrong bug number.

Sebastian

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8848377 [details] [diff] [review]
edit_width_height.patch v6a

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

So, I think the code is all good, but there is still some edge cases with typing a long number. I would recommend some mixture of showing ellipsis on text overflow, and trying to align the texts to make use of any of the spaces available. Otherwise, I see we have a z-index problem with the editable field where we can see the "x".

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

One more new issue is that this should only work for box-sizing: content-box.

Comment 21

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v7

Made the editable field only work when box-sizing == "content-box". 

Regarding the issue where large numbers cause the text to cover other elements, I feel like that's a bit of scope creep on this feature. Entering a large number into (for example) padding-left will cause the same z-index issue where the number will overlap with the 'padding' legend.
Maybe we should make a new bug to track that. It can be assigned to me, but I don't think this feature should be blocked on that issue.

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8848847 [details] [diff] [review]
edit_width_height.patch v7

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

This needs unit test prior to landing.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js
@@ +134,5 @@
>      height = this.getHeightValue(height);
>      width = this.getWidthValue(width);
>  
> +    let contentBox = layout["box-sizing"] == "content-box" ?
> +        dom.p(

We should only indent this with 2 spaces.

@@ +155,5 @@
> +            textContent: height,
> +            onShowBoxModelEditor
> +          }),
> +        )
> +      : dom.p(

Move the dom.p( to a new line and just have : as its own line

@@ +161,5 @@
> +            className: "boxmodel-size",
> +          },
> +          dom.span(
> +            {},
> +            `${width}px \u00D7 ${height}px`,

This should be SHARED_L10N.getFormatStr("dimensions", width, height).

::: devtools/client/themes/boxmodel.css
@@ +221,5 @@
>  }
>  
> +.boxmodel-size > p {
> +  display: inline-block;
> +  width: 10%;

So, it turns out the z-index and overlapping problem would all be fixed if we would just remove this width. \o/

@@ +224,5 @@
> +  display: inline-block;
> +  width: 10%;
> +  margin: auto;
> +}
> +

NIT: Extra new line.

Comment 23

[Sheldon Roddick [:sroddick]]

Attachment: edit_width_height.patch v8

Gabriel mentioned this needed unit tests before landing, and then the "patch v7" got an r+. Not sure exactly what state that puts this issue in so I've included both the implementation and unit tests in this patch. If the unit tests need to be separated now then I'll do that.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1061823.patch

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1061823.patch

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb064bc054f9
Allow to edit width and height in the box model. r=gl

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Fahima Zulfath]

[bugday-20170412] 
The bug is no longer to be reproducible in Latest Nightly 55.0a1. Edit enabled in box-model.
OS:Windows 10

Comment 29

[[:Towkir] Ahmed]

This feature was not found on old nightly builds according to comment 0  on these:

------ old nightly on linux ------
Version 	34.0a1
Build ID 	20140902030202
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0

------ old nihgtly on windows ------
Version 	34.0a1
Build ID 	20140902030202
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:34.0) Gecko/20100101 Firefox/34.0


And the feature is implemented also the fix is hereby verified on :

------ latest DevEdition on windows ------
Version 	55.0b11
Build ID 	20170720101431
Update Channel 	aurora
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
OS 	        Windows_NT 6.1

------ latest DevEdition on linux ------
Version 	55.0b11
Build ID 	20170720171353
Update Channel 	aurora
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
OS 	        Linux 4.10.0-27-generic