The default bug view has changed. See this FAQ.

Allow to edit width and height in the box model

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
3 years ago
4 hours ago

People

(Reporter: canuckistani, Assigned: sroddick)

Tracking

(Blocks: 3 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(1 attachment, 10 obsolete attachments)

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
Recently suggested on uservoice as well: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/7076113-assept-to-edit-sizes-of-blocks
Adding this, not because I want it in 40 but because I remembered this bug existed today and want it prioritized in the backlog.
Priority: -- → P2
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [polish-backlog]
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]

Comment 3

a year ago
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.
Blocks: 991806
OS: Mac OS X → All
Hardware: x86 → All
Summary: Implement Editable width/height in the box model → Allow to edit width and height in the box model

Updated

a month ago
Assignee: nobody → rahulch95

Updated

a month ago
Status: NEW → ASSIGNED

Updated

a month ago
Assignee: rahulch95 → sheldon.roddick
(Assignee)

Comment 4

17 days ago
Created attachment 8843883 [details] [diff] [review]
edit_width_height.patch v1
Attachment #8843883 - Flags: review?(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.
Attachment #8843883 - Flags: review?(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
(Assignee)

Comment 7

16 days ago
Created attachment 8844350 [details] [diff] [review]
edit_width_height.patch v2
Attachment #8843883 - Attachment is obsolete: true
Attachment #8844350 - Flags: review?(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.
Attachment #8844350 - Flags: review?(gl)
(Assignee)

Comment 9

15 days ago
Created attachment 8844852 [details] [diff] [review]
edit_width_height.patch v3
Attachment #8844350 - Attachment is obsolete: true
Attachment #8844852 - Flags: review?(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}.
Attachment #8844852 - Flags: review?(gl)
(Assignee)

Comment 11

10 days ago
Created attachment 8846511 [details] [diff] [review]
edit_width_height.patch v4
Attachment #8844852 - Attachment is obsolete: true
Attachment #8846511 - Flags: review?(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
Attachment #8846511 - Flags: review?(gl)
(Assignee)

Comment 13

9 days ago
Created attachment 8847014 [details] [diff] [review]
edit_width_height.patch v5
Attachment #8846511 - Attachment is obsolete: true
Attachment #8847014 - Flags: review?(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.
Attachment #8847014 - Flags: review?(gl)
(Assignee)

Comment 15

8 days ago
Created attachment 8847455 [details] [diff] [review]
edit_width_height.patch v6
Attachment #8847014 - Attachment is obsolete: true
Attachment #8847455 - Flags: review?(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,
Attachment #8847455 - Flags: review?(gl)
Blocks: 150496
Blocks: 1347964
(Assignee)

Comment 17

7 days ago
Created attachment 8848377 [details] [diff] [review]
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.
Attachment #8847455 - Attachment is obsolete: true
Attachment #8848377 - Flags: review?(gl)
> Blocks: 150496

Obviously the wrong bug number.

Sebastian
No longer blocks: 150496
Blocks: 1150496
QA Contact: cristian.comorasu
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".
Attachment #8848377 - Flags: review?(gl)
One more new issue is that this should only work for box-sizing: content-box.
(Assignee)

Comment 21

4 days ago
Created attachment 8848847 [details] [diff] [review]
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.
Attachment #8848377 - Attachment is obsolete: true
Attachment #8848847 - Flags: review?(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.
Attachment #8848847 - Flags: review?(gl) → review+
(Assignee)

Comment 23

a day ago
Created attachment 8849867 [details] [diff] [review]
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.
Attachment #8848847 - Attachment is obsolete: true
Attachment #8849867 - Flags: feedback?(gl)
Created attachment 8850262 [details] [diff] [review]
1061823.patch
Attachment #8849867 - Attachment is obsolete: true
Attachment #8849867 - Flags: feedback?(gl)
Attachment #8850262 - Flags: review+
Created attachment 8850263 [details] [diff] [review]
1061823.patch
Attachment #8850262 - Attachment is obsolete: true
Attachment #8850263 - Flags: review+

Comment 26

15 hours ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb064bc054f9
Allow to edit width and height in the box model. r=gl

Comment 27

4 hours ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb064bc054f9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 hours ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.