1277007 - Box Model Visual Editor

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Maximillian von Briesen [:moby]]

Hey guys, I'm the intern working on some of the designer tools to be added to Firefox. The one I'm working on right now is the box model editor. This will allow users to select an element and visually modify its margins, padding, borders, etc...

I'd like to get some feedback and some suggestions on how to make this tool practical and elegant. I have a prototype up here that you can test out: http://mobyvb.com/box-model-editor/
To use it, click an element on the page (such as the outer dark grey div). The editor will then appear overlaid on that element. You can click and drag the handles to modify the box model (right now, it is limited to margin and padding).

Anyway, play around with that and let me know if you see significant flaws or changes that can be made to make this feature better.

Comment 1

[Maximillian von Briesen [:moby]]

I've made a number of changes to the editor today and yesterday:
* Changed handle colors to match the attributes they modify (ie all padding handles have the same color, and all margin handles have the same color)
* Removed handles that did not do anything (may potentially be added back in the future when more functionality is designed)
* Mouse event listeners are on the document now rather than the body, which allows you to pull a handle past the bottom of the current page without the handle losing "grip"
* Added a tooltip that shows up near the mouse when modifying an attribute. It displays the attribute being modified as well as the current value of said attribute
* Refactored box model editor code into a JS prototype that can be constructed on any node using `new BoxModelEditor(someNode)`. This allows you to have multiple editors open at a time and might also make it easier to transfer code from the current version to an actual devtools version.
* Added a close button to the box model editor
* Added a rudimentary snap-to-grid option. There is an option to turn the grid on, and another option to input grid size (in pixels).
* Added a mirroring option. When this option is selected, modifying any attribute also modifies its "mirror". So if you are modifying paddingLeft, then paddingRight will be modified by the same amount.

Tentative goals:
* Make handles better. I think they should automatically be centered, but slightly offset when they are about to overlap with another handle. Alternatively, allow the user to position handles where desired.
* Rather than just bordering each box model component, lines should extend to the end of the screen
* Allow top, left, bottom, right, width, and height attributes to be modified for fixed, absolute, or relative positioned elements. This will be difficult as it may involve having handles on the same edge

Comment 2

[Maximillian von Briesen [:moby]]

In order to get this into devtools, it would probably be ideal to modify the geometry editor that was recently added (Bug 1264992). This already takes care of moving absolutely positioned elements, so it would feel natural to also modify the box model with the same editor.

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Maximillian von Briesen [:moby] from comment #2)
> In order to get this into devtools, it would probably be ideal to modify the
> geometry editor that was recently added (Bug 1264992). This already takes
> care of moving absolutely positioned elements, so it would feel natural to
> also modify the box model with the same editor.
For the sake of keeping the code simple, I would recommend creating a new highlighter in 
\devtools\server\actors\highlighters\
In this folder we have geometry-editor.js which is meant at modifying element's positions and sizes. I would recommend creating a new file like box-model-editor.js for instance that is meant at modifying element's margin, padding, border.
These 2 highlighters would probably have a lot in common and you might need to extract some helper functions into common files to reuse them.
In terms of UI, I'm worried that if we do both at the same time: position (top, right, bottom, left) and size (width, height) AND box-model (margin, border, padding, on all 4 sides) that we'll end up with too many handles and too many lines that would make the UI confusing.
I think they should definitely look and behave somewhat similarly, but might be better as 2 separate editors.
In terms of how we want it to be enabled/disabled from the toolbox, it might be easy to start with a button in the box-model sidebar, next to the one we have for the position editor. At least there is code there already that you can copy/paste in order to have something that works quickly.
And then we should think of a better way to expose the tool, in a second step.
I'm thinking the rule-view might be a good place for it, when you're editing styles in a rule, it'd be nice if you could edit them in the page and have them apply to all elements that match this rule.

Comment 4

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Here's a functional version of the box model editor, based on the geometry editor code. It needs some design work, but the difficult part is done :)

Comment 5

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Adds different colors for margin/padding handles, and offsets them slightly so they do not overlap when close together.

Comment 6

[Patrick Brosset <:pbro>]

Thanks for the patch Moby, this looks great.
I've quickly tested it, would need to do more thorough tests, but I did find out a few things:

- first of all, this is great, really useful and intuitive,
- when the box model is really small (i.e. the element itself is small and has small margins and paddings), it's really hard to see anything because of the labels. See this screenshot:
https://dl.dropboxusercontent.com/u/714210/tiny-box-model-lots-of-labels.png
I think we should hide the labels by default and only show them as you resize things.
- if a handle happens to be at the very bottom of the viewport, near the toolbox resize splitter for example, then it's impossible to grab:
https://dl.dropboxusercontent.com/u/714210/box-model-editor-handle-at-the-bottom.png
Trying to resize the bottom padding here actually resizes the toolbox instead.
- on mozilla.org, faces of people in the background have a 5px border, which I didn't manage to resize.
See: https://dl.dropboxusercontent.com/u/714210/cant-resize-border.png
What's weird is that I'm not seeing the handles for the borders, but instead I'm seeing handles for both padding and margin, with labels showing 0px. I was expecting to see handles for the borders instead. And the trying to move the handles for the margin and padding doesn't do anything.

So, there are a few rough edges to polish, but this is looking like a really good start!

Comment 7

[Maximillian von Briesen [:moby]]

I can hide the labels unless a component is currently being resized.

Could you give me some advice on what to do to solve the problem where the handle is at the bottom of the viewport?

Right now, I don't have border resizing functionality. I talked to Brian about this, and we think that since most of the time, borders are very small, adding the ability to resize them would overly clutter the interface. Maybe we could have exceptions for borders with a certain size or above. I tried going to mozilla.org myself, and I was able to resize the margin and padding on the background images.

Comment 8

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

This only displays a label if its attribute is currently being modified

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Maximillian von Briesen [:moby] from comment #7)
> I can hide the labels unless a component is currently being resized.
Yes, I think that would be great.
> Could you give me some advice on what to do to solve the problem where the
> handle is at the bottom of the viewport?
We could remove the toolbox splitter and instead make the toolbox toolbar be the resize handle. But that doesn't solve all the use cases, because you could have the toolbox docked to the side instead, and then you'd be resizing the browser instead.
Chrome has the same problem, but their handles seem to be easier to grab. Part of this is because they are bigger, but also they are displayed on the inside of the margin or padding, not centered on it, so they don't overlap with the browser window, or toolbox splitter handles.
> Right now, I don't have border resizing functionality. I talked to Brian
> about this, and we think that since most of the time, borders are very
> small, adding the ability to resize them would overly clutter the interface.
> Maybe we could have exceptions for borders with a certain size or above.
Yeah that's a good point, but padding and margin can also be very small in many cases, like padding in a text field for instance is often 2px or something.
Hiding the labels while the user is not resizing helps a lot I think. But you're right, maybe having the handles for margin, border and padding is going to make the UI just impossible to use.
I guess if we consider this as an editor for the layout, then having only margin and padding is fine, because border is most often used for decoration, not layout purpose. But if we think of this as a "box model" editor, that is toggled from the box-model tab in the inspector, then it's kind of weird not handling the border. Because we're presenting the tool as a simpler way to change the box model in the page directly rather than by using the box-model diagram in the inspector.
> I tried going to mozilla.org myself, and I was able to resize the margin and
> padding on the background images.
Ok good, then I must have run into some sort of other bug that we can investigate later.

Comment 10

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

This allows you to modify properties that are not defined by user; If a property is not defined, the computed value is used for editing handles (usually 0px)

I also fixed a small bug where you couldn't drag edges all the way to 0. It would stop somewhere arbitrary like 0.0158%.

Comment 11

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

You can mirror properties now if you hold shift while modifying one.

Comment 12

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Box model editor handles are positioned properly for various types of positioned elements (there are still many cases where handles are not positioned properly)

Comment 13

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

This fixes a ton of the weird edge cases where handles were not being placed in the right spot on the box model. Some of the cases this deals with:

* Elements where 'box-sizing' is set to 'border-box', and 'width' and/or 'height' are defined
* Elements where 'display' is set to 'inline-block'
* Elements where 'float' is set to 'left' or 'right'

I will need to test this more extensively to be sure it works in all cases, but the editor at least works in significantly more situations than it did before.

Comment 14

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Clean up code and comment important sections. I think this is ready for a review while I work on some tests.

Comment 15

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

(In reply to Maximillian von Briesen [:moby] from comment #14)
> Created attachment 8773056 [details] [diff] [review]
> box-model-editor.patch
> 
> Clean up code and comment important sections. I think this is ready for a
> review while I work on some tests.

This probably needs to be rebased with the latest movement of the box model into the computed view. Sorry :(

Comment 16

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

deal with merge conflicts in latest nightly

Comment 17

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Pressing escape now exits out of the box model and geometry editors

Comment 18

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor-entry-ff.png

Comment 19

[Maximillian von Briesen [:moby]]

Attachment: layout-editor-entry-chrome.png

Comment 20

[Maximillian von Briesen [:moby]]

Helen, I have a couple questions about the design of this feature. Specifically about how the user accesses it. Currently, the user navigates to the "Computed" tab in the style panel and clicks the box model editor icon. The attachment above called box-model-editor-entry-ff.png is a screenshot of this entry point. Currently, the box model editor is using the same icon as the geometry editor, which needs to change. Do you have any suggestions for icons to use?

I've also attached layout-editor-entry-chrome.png, which is how the same feature is accessed in Chrome Canary once enabled. One notable difference is that the icon for Chrome's editor is in the rule view rather than the Computed view as it is for Firefox. This makes it much more accessible as the rule view is displayed by default when the inspector is opened. Keeping the box model and geometry editors in the computed view makes it far less likely that users will see them. I don't know if you're the person to talk to about that, but I figured I'd mention it in case you or anyone else has insight on where these features should be located.

Comment 21

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Maximillian von Briesen [:moby] from comment #20)
> Helen, I have a couple questions about the design of this feature.
> Specifically about how the user accesses it. Currently, the user navigates
> to the "Computed" tab in the style panel and clicks the box model editor
> icon. The attachment above called box-model-editor-entry-ff.png is a
> screenshot of this entry point. Currently, the box model editor is using the
> same icon as the geometry editor, which needs to change. Do you have any
> suggestions for icons to use?
> 
> I've also attached layout-editor-entry-chrome.png, which is how the same
> feature is accessed in Chrome Canary once enabled. One notable difference is
> that the icon for Chrome's editor is in the rule view rather than the
> Computed view as it is for Firefox. This makes it much more accessible as
> the rule view is displayed by default when the inspector is opened. Keeping
> the box model and geometry editors in the computed view makes it far less
> likely that users will see them. I don't know if you're the person to talk
> to about that, but I figured I'd mention it in case you or anyone else has
> insight on where these features should be located.

The box model doesn't have an editor, so far as I know—that first icon was meant to be the geometry editor icon. So delete that first instance of the icon that isn't hooked up to anything and you should be done.

Patrick, Gabe and I are moving a lot of functionality into the box model area, and I believe the geometry editor fits best there. For one, we'll have more room to explain to the user what it is there (I think that this feature probably needs a link next to it when it first lands explaining that it's new and a link to where to use it). While most of these reviews are related to the box model designs, you can see some of the iterations in bug 1150496.

Figuring out how to surface things in devtools is always an issue—unfortunately, we can't put everything on the first page, it's just not feasible. Because Chrome still has this under a flag, we can't even extrapolate that where they've put that button is where it's permanently going to live, so in this instance, because it doesn't work for us, I think it fits better where we've made plans for it.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #21)
> (In reply to Maximillian von Briesen [:moby] from comment #20)
> > Helen, I have a couple questions about the design of this feature.
> > Specifically about how the user accesses it. Currently, the user navigates
> > to the "Computed" tab in the style panel and clicks the box model editor
> > icon. The attachment above called box-model-editor-entry-ff.png is a
> > screenshot of this entry point. Currently, the box model editor is using the
> > same icon as the geometry editor, which needs to change. Do you have any
> > suggestions for icons to use?
> > 
> > I've also attached layout-editor-entry-chrome.png, which is how the same
> > feature is accessed in Chrome Canary once enabled. One notable difference is
> > that the icon for Chrome's editor is in the rule view rather than the
> > Computed view as it is for Firefox. This makes it much more accessible as
> > the rule view is displayed by default when the inspector is opened. Keeping
> > the box model and geometry editors in the computed view makes it far less
> > likely that users will see them. I don't know if you're the person to talk
> > to about that, but I figured I'd mention it in case you or anyone else has
> > insight on where these features should be located.
> 
> The box model doesn't have an editor, so far as I know—that first icon was
> meant to be the geometry editor icon. So delete that first instance of the
> icon that isn't hooked up to anything and you should be done.

This patch is adding a new 'box model editor' feature that's similar to the geometry editor, but implemented separately.  In the current work in progress, it's just a copy of geometry editor icon so that there's a way to toggle this new editor.  So I think we'll either need to come up with a new icon for the box model editor, find a different way/place to toggle it, or consolidate geometry and box model editors into a single editor.

(I was originally thinking that it would make sense to consolidate both into a single editor, but each one has a lot of UI already - Moby and Matteo could I'm sure elaborate on some challenges with that)

Comment 23

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> This patch is adding a new 'box model editor' feature that's similar to the
> geometry editor, but implemented separately.  In the current work in
> progress, it's just a copy of geometry editor icon so that there's a way to
> toggle this new editor.  So I think we'll either need to come up with a new
> icon for the box model editor, find a different way/place to toggle it, or
> consolidate geometry and box model editors into a single editor.
> 
> (I was originally thinking that it would make sense to consolidate both into
> a single editor, but each one has a lot of UI already - Moby and Matteo
> could I'm sure elaborate on some challenges with that)

Oh, I'm sorry, I obviously misunderstood.

I think re: placement issues, the editor itself definitely belongs in the Computed box-model area. It's a box model editor. I think that probably means that the geometry editor might need to move, although I don't have any good ideas on where that would go just yet.

Let's log a follow-up bug for an icon for the box model editor. I've pinged Mateo about the geometry editor and discussing where it should live.

Comment 24

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

1. Prevent user from opening box model editor and geometry editor at the same time
2. Do not show handles for properties that are defined as "auto"

Comment 25

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Comment 26

[Maximillian von Briesen [:moby]]

Attachment: box-model-editor.patch

Fixes a bug with the px to defined unit ratio. When a property with a unit other than px was dragged to 0, then was dragged back, the property would move at a different rate than the mouse.

Comment 27

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8776068 [details] [diff] [review]
box-model-editor.patch

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

Thanks Moby for this new highlighter!
I'm setting a positive feedback instead of a review, mostly because I wasn't able to test it properly manually; maybe it needs a rebase – I tried to do so locally, but still it didn't work.

Also, there are a couple of things to discuss with Helen and Patrick, as mentioned.

But so far, great job!

::: devtools/client/inspector/layout/layout.js
@@ +198,5 @@
>    this.sizeHeadingLabel = this.doc.getElementById("layout-element-size");
>    this._geometryEditorHighlighter = null;
> +  this._boxModelEditorHighlighter = null;
> +  this.boxModelEditorenabled = false;
> +  this.geometryEditorenabled = false;

Typo, I believe. They should be both `Enabled` with uppercase `E`.

@@ +330,5 @@
>      let nodeGeometry = this.doc.getElementById("layout-geometry-editor");
>      this.onGeometryButtonClick = this.onGeometryButtonClick.bind(this);
>      nodeGeometry.addEventListener("click", this.onGeometryButtonClick);
> +
> +

Just one empty line here.

@@ +798,5 @@
>      // instantiate Geometry Editor highlighter
>      toolbox.highlighterUtils
>        .getHighlighterByType("GeometryEditorHighlighter").then(highlighter => {
>          highlighter.show(nodeFront).catch(console.error);
> +        highlighter.on("canceled", this.hideGeometryEditor);

Is there a reason why we couldn't reuse `hide` instead of introducing a new event?

@@ +903,5 @@
>  
>      let nodeGeometry = this.doc.getElementById("layout-geometry-editor");
>      nodeGeometry.style.visibility = isEditable ? "visible" : "hidden";
> +
> +    let nodeBoxModel = this.doc.getElementById("layout-box-model-editor");

In my case the button was always visible – and not working. It could be that during the resolving conflict something went bad, so please rebase your patch and I will test it again.

In addition, I believe you need a separate `updateBoxModelButton`, and you will need to add a new method to `pageStyle`; since `isPositionEditable` works using `getDefinedGeometryProperties` of GeometryEditor, where you have to use `getDefinedBoxModelProperties`.

Probably the best course of action here is, unified those as mentioned above, and modifying `isPositionEditable` to take options that let knows if we're interested in the position or in the box-model.

Unless of course, we want to have this button always visible, as it seems from the line below. In case of the Geometry Editor, we want to edit only the properties that are actually defined; so if no properties are defined, the Geometry Editor is useless.

::: devtools/client/themes/layout.css
@@ +36,5 @@
> +  visibility: hidden;
> +}
> +
> +#layout-box-model-editor::before {
> +  background: url(images/geometry-editor.svg) no-repeat center center / 16px 16px;

We should definitely have a different icon for the box model editor; currently is not clear at all which one is which, if both of them are activated.

Maybe we could have only one single button that can switch between the two functionalities, I'm not sure.

I'd like to have Helen take a look on that.

::: devtools/server/actors/highlighters.css
@@ +234,1 @@
>    /* The geometry editor can be interacted with, so it needs to react to

Update the comment introducing box model editor too.

::: devtools/server/actors/highlighters.js
@@ +427,5 @@
>  
>      // The assumption is that all custom highlighters need the canvasframe
>      // container to append their elements, so if this is a XUL window, bail out.
>      if (!isXUL(this._inspector.tabActor.window)) {
> +      this._highlighterEnv = new HighlighterEnvironment(this);

I'm not sure about passing the direct reference to `CustomHighlighterActor` to the `HighlighterEnvironment`, creating a circular reference; I'd like to have a feedback from Patrick on this topic too.

::: devtools/server/actors/highlighters/box-model-editor.js
@@ +19,5 @@
> +// Box Model Editor highlighter
> +const DOM_EVENTS = ["mousemove", "mouseup", "pagehide", "keydown"];
> +const events = require("sdk/event/core");
> +
> +const _dragging = Symbol("geometry/dragging");

That's not the geometry editor, you should call it "box-model/dragging" or something alike.

@@ +25,5 @@
> +/**
> + * Element geometry properties helper that gives names of position, margin, padding, and size
> + * properties.
> + */
> +var GeoProp = {

It seems to me that this is almost the same object we have in `GeometryEditor`. I would suggest to uniform the two objects and put in a separate module and requires it from both `box-model-editor` and `geometry-editor`. For the time being, you could save it as `highlighters/utils/geometry.js` or `highlighters/utils/geometry-props.js` or anything sounds good to you.

@@ +45,5 @@
> +
> +  isHorizontal: function (name) {
> +    return name === "margin-left" || name === "margin-right"
> +      || name === "padding-left" || name === "padding-right";
> +  },

Probably you can simplify – especially if we're going to uniform this object – with something like:

 isHorizontal: function (name) {
   let side = this.getSideFromProp(name);
   
   return side === "left" || side === "right";
 }

@@ +50,5 @@
> +
> +  isInverted: function (name) {
> +    return name === "margin-right" || name === "padding-right"
> +      || name === "margin-bottom" || name === "padding-bottom";
> +  },

As above.

@@ +53,5 @@
> +      || name === "margin-bottom" || name === "padding-bottom";
> +  },
> +
> +  getSideFromProp: function (name) {
> +    if (name === "margin-top" || name === "padding-top") {

We could probably have a dictionary with all those props, it would be easier, and it could be reusable from other part of `GeoProp` too. Especially if we're going to have `GeoProp` in it's own module, so that is doesn't need to be exported. Something like:

  // feel free to come with a better name!
  const PROPS2SIDE = {
    "top": "top",
    "margin-top": "top",
    "padding-top": "top",
    "bottom": "bottom",
    "margin-bottom": "bottom",
    "padding-bottom": "bottom",
    /* etc */
  }

Then, for all props, we could just get the `Object.keys`, `getSideFromProp` it would be simply:

   function (name) {
     return PROPS2SIDE[name] || null;
   }

@@ +93,5 @@
> +    return this.isHorizontal(name) ? "y" : "x";
> +  },
> +
> +  getMirror: function(name) {
> +    if (name.indexOf("right") >= 0) {

You could probably use `if (name.endsWith("right")) {` instead (and so on).

Another approach could be also declaring a const as `PROP2SIDES`, like `MIRRORS_PROPS` or similar:

   const MIRRORS_PROPS = {
     "top": "bottom",
     "right": "left",
     "bottom": "top",
     "left": "right"
   }

And then having something like:

    return name.replace(/(top|right|bottom|left)$/, m => MIRRORS_PROPS[m]);

@@ +117,5 @@
> + * @param {nsIDOMNode} node The node to analyze.
> + * @return {Map} A map indexed by property name and where the value is an
> + * object having the cssRule property.
> + */
> +function getDefinedBoxModelProperties(node) {

As for the GeoProp, I believe this is way too similar to the counterpart in the geometry editor, that I think is possible to uniform both of them in one, and put in the same external module, that both highlighters will requires.

@@ +165,5 @@
> + * The highlighter displays lines and labels for each margin and padding property
> + * (8 in total).
> + *
> + * The lines have handles that are draggable so each margin or padding property can be
> + * modified easily. If the shift key is held while dragging a property, the mirror of that

I think it's a nice feature that we should have on Geometry Editor too. Could you please a follow up bug for that, so that the UX will be consistent?

Also, I believe it would be better using "alt" instead of "shift", I'd like to double check with Helen, but usually in similar software the "shift" key is used to keep a 1:1 proportion, where the "alt" is used to center and move the side by the same amount; so I think it would be more familiar for the user.

@@ +211,5 @@
> +        "id": "root",
> +        "class": "root",
> +        "hidden": "true"
> +      },
> +      prefix: this.ID_CLASS_PREFIX

Nit: you can declare a `prefix` variable before the root, so you don't have to repeat `this.ID_CLASS_PREFIX` for all the element:

  let prefix = this.ID_CLASS_PREFIX;

  let root = createNode(this.win, {
    parent: container,
    attributes: {
      "id": "root",
      "class": "root",
      "hidden": "true"
    },
    prefix // this is equals to `prefix: prefix`
  }

@@ +385,5 @@
> +
> +        if (handlerSide) {
> +          let side = handlerSide;
> +
> +          let { value, unit, ratio, dir } = this.getSideDragInfo(side);

With the introduction of `getSideDragInfo`, you could probably simplify a bit and makes everything more consistent. For start, you could have something like:

   if (handler) {
     let current = this.getSideDragInfo(handlerSide);
     let mirror = (shiftKey) ? this.getSideDragInfo(GeoProp.getMirror(handlerSide)) : null;

     this[_dragging] = {
       x: pageX,
       y: pageY,
       current,
       mirror
     };

Of course `getSideDragInfo` needs to returns the `side` too in its object. But at this point you will have everything you need to perform your operation. The `ratio` and the `dir` will be included in both to calculate the `inc` when you need it. It would be also much easier "mirroring" all the four sides at the same time if needed in the future. Also, you don't need to pass `mirroring` since you can just check if `mirror` is `null`.

@@ +771,5 @@
> +      rightFixed = false;
> +    }
> +    if ((position === "absolute" || position === "fixed")
> +      && (topStyle || bottomStyle || leftStyle || rightStyle)) {
> +      topFixed = topStyle ? true : false;

You could use either `topFixed = !!topStyle` or `topFixed = Boolean(topStyle)` – the same is applied to the rest of the props below.

::: devtools/server/actors/highlighters/moz.build
@@ +9,5 @@
>  ]
>  
>  DevToolsModules(
>      'auto-refresh.js',
> +    'box-model-editor.js',

If the build step doesn't complain it's fine to me; but I honestly thought 'box-model.js' would be considered before 'box-model-editor.js'.

Comment 28

[Matteo Ferretti [:zer0] [:matteo]]

Patrick, I'd like to have a feedback about the part wheere a direct reference to `CustomHighlighterActor` is passed to the `HighlighterEnvironment`, creating a circular reference. What do you think about that? I'm wondering if there is a better way.

Comment 29

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #23)

> > (I was originally thinking that it would make sense to consolidate both into
> > a single editor, but each one has a lot of UI already - Moby and Matteo
> > could I'm sure elaborate on some challenges with that)
> 
> Oh, I'm sorry, I obviously misunderstood.
> 
> I think re: placement issues, the editor itself definitely belongs in the
> Computed box-model area. It's a box model editor. I think that probably
> means that the geometry editor might need to move, although I don't have any
> good ideas on where that would go just yet.

As was stated, ideally Geometry Editor and Box Model Editor would be the same. In fact, the latter was based on the first one. The user should be able to switch on-the-fly between the properties to edit, instead of showing everything immediately. But that's is something we'll deal in the future, because it requires more work on both unifying the code but, especially, on UI / UX thinking. So I don't think we should split them up – placing geometry editor somewhere different than the box model editor.
That's, of course, if we intend to keep the original idea to have them as one tool.

However, if we want to have the box model editor landed, we definitely need to come up with some UI: a different icon at least, or maybe we could share the same icon but have a popup menu to choose which editor display, I don't know. So, any thoughts will be appreciated!

Plus, I'd like to have a feedback on the "mirroring" thing: in the current patch, if the user drag the left side, and then keep the "shift" pressed, also the right side will be adjusted by the same amount. It's true also likewise, and if you're dragging the top and the bottom. I was thinking that, if we want to keep this feature, probably the "alt" key would be a better candidate – at least in the program I recall, the "shift" is for keep a 1:1 proportion during the dragging, where the "alt" is used for centering, and then move the sides by the same amount.

But, we also have to keep in mind that, once both of them will be unified, maybe one key could be used to "switch" between modalities / properties.

What do you think?

> Let's log a follow-up bug for an icon for the box model editor.

Personally I think bug 1277007 should be a blocker for this one. Having such icons one close to the other, and they looks exactly the same, it's too way confused to land as is.

> I've pinged Mateo about the geometry editor and discussing where it should live.

As said on IRC, I'm available anytime for a chat!

Comment 30

[Helen V. Holmes (:helenvholmes) (:✨)]

Talked this over with Brian Grindstead; sounds like there's enough UX ork here that I'm not sure I can wrap it up in a needinfo.

Bryan, for your ni?: Moby has put a lot of work during his internship into a box model editor that covers a lot of the use cases the geometry editor does not handle. Ideally, we'd like these two tools to be one tool, but merging them would probably require a fair amount of UX work. Any idea how I should slot this work in? I would imagine in Q4 if our engineering plans stay on track.

Comment 31

[Maximillian von Briesen [:moby]]

I was talking to Gabriel the other day about this and he suggested I merge the geometry editor and box model editor. I've been working on doing so, so you can edit everything from the geometry editor. This would mean we wouldn't need a separate button or a toggle switch. I don't think the UI would look too cluttered if I do this. I'll be busy moving next week but I'll get a patch for this up as soon as I can. Since a lot of the code for the two editors is shared, I don't think this will be too difficult to do.

Comment 32

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Maximillian von Briesen [:moby] from comment #31)
> I was talking to Gabriel the other day about this and he suggested I merge
> the geometry editor and box model editor. I've been working on doing so, so
> you can edit everything from the geometry editor. This would mean we
> wouldn't need a separate button or a toggle switch. I don't think the UI
> would look too cluttered if I do this. I'll be busy moving next week but
> I'll get a patch for this up as soon as I can. Since a lot of the code for
> the two editors is shared, I don't think this will be too difficult to do.

Hey Moby, I'm mostly figuring out how to triage any additional UX work that would need to be done for this bug, and when to slot it in to ongoing work. Is there a question I can address with your needinfo, or did you just want to make sure I saw your message?

Comment 33

[Maximillian von Briesen [:moby]]

That was just to make sure you saw it, especially since Bug 1290145 probably won't be necessary if the editors end up being unified.

Comment 34

[Maximillian von Briesen [:moby]]

Attachment: unified-editor.png

Comment 35

[Maximillian von Briesen [:moby]]

Attachment: unified-editor.patch

Ta-da!

I didn't expect to have something functional this soon. It's still pretty buggy but it kinda works. I've attached a screenshot so everyone can see what this looks like.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #30)
> Bryan, for your ni?: Moby has put a lot of work during his internship into a
> box model editor that covers a lot of the use cases the geometry editor does
> not handle. Ideally, we'd like these two tools to be one tool, but merging
> them would probably require a fair amount of UX work. Any idea how I should
> slot this work in? I would imagine in Q4 if our engineering plans stay on
> track.

I believe so, inspector work is planned for Q4 and I think this would fit well with that.

Comment 37

[Patrick Brosset <:pbro>]

Comment on attachment 8776068 [details] [diff] [review]
box-model-editor.patch

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

I've taken a look at the change to the HighlighterEnvironment.

If I understand correctly, the need here is to let the front-end know that the highlighter was dismissed when the user pressed ESCAPE. The solution proposed in this patch is to pass the instance of the CustomHighlighterActor (through the HighlighterEnvironment) to the highlighters (box-model-editor and geometry-editor) so that, when their ESCAPE key listeners are executed, they can use the actor to send an event to the front-end.

I see 4 design problems with this:
- First, as Matteo said, using the HighlighterEnvironment for this creates a circular reference, but also it makes the HighlighterEnvironment aware of something it really doesn't care about. This class was always meant to be a proxy to a window where a highlighter would be shown. Here a property is added to it that has no other purpose than to be passed down to something else.
- Secondly the HighlighterEnvironment isn't only meant at being used by CustomHighlighterActors, the plain HighlighterActor also uses it, so adding a this.customHighlighterActor property on it doesn't feel right.
- Thirdly, the code location where the event is sent is the highlighters themselves (box-model-editor and geometry-editor), but the event really belongs to the actor. Ultimately, consumers will see this event as being sent from the actor. So it makes sense for the actor to send it. Dispatching events for objects from code outside of the object definition itself is making things hard for people to follow how the code works.
- And finally, one of the designs principles behind the highlighters is that they have no need to know what uses them. They don't know they are being used by a CustomHighlighterActor instance, or by another other actor instance at all. They are just given an environment to work in, and that's all the use. They don't even use a tabActor like most of our server-side code does. This is an advantage of the design because it means they are more easily reusable, and should try and keep it this way.

So, here's another solution for the use case:
- Roll back the changes made to the HighlighterEnvironment class, it shouldn't need to know about the actor.
- Come up with a list of events that we think the various CustomHighlighterActors will need. Start with just one event for now: "canceled", because that's the only one you really need now, and we can add more later. And define this event in the actor spec (which you've done already).
- Now, keep the keyboard handling in box-model-editor.js and geometry-editor.js, but when ESCAPE is pressed, just emit a regular event to whoever wants to listen: this.emit("canceled");
- In CustomHighlighterActor, right after the highlighter has been created, also start listening for this event:
this._highlighter = new constructor(this._highlighterEnv);
this._highlighter.on("canceled", this.onHighlighterCanceled);
- And use this to send the event to the front-end:
onHighlighterCanceled: function(e) {
  events.emit(this, "canceled");
}

This way, we keep the protocol event where it belongs, and we leave the HighlighterEnvironment as it was. The only thing we add is the garanty that if a highlighter instantiated by the CustomHighlighterActors fires a "cancelled" event, then that will be sent to the front-end.
But the highlighter itself doesn't need to care about this, it only signals to its consumer(s) that it was cancelled.

Comment 38

[Patrick Brosset <:pbro>]

Forgot to clear the needinfo flag.

Comment 39

[Maximillian von Briesen [:moby]]

Attachment: unified-editor.patch

Fix issue where non-px units were not changing by the correct amount and where certain handles were incorrectly inverted.

Comment 40

[Maximillian von Briesen [:moby]]

Attachment: unified-editor.patch

Unify some of the box model vs. position code, and get mirroring working for position handles (only when a position handle and its mirror are both defined).

Comment 41

[Maximillian von Briesen [:moby]]

Attachment: unified-editor.patch

I'm sorry it's been so long since I made an update. I've finished going through and unifying all of the box model/geometry editor code. There might be one or two things that still need to be fixed (like preventing negative values for certain attributes), but other than that, I believe this is ready for an initial review before I start working on unified editor tests.

Comment 42

[Maximillian von Briesen [:moby]]

Attachment: unified-editor.patch

Forgot to mark other patch as obsolete. Still ready for review.

Comment 43

[Brian Grinstead [:bgrins]]

(In reply to Maximillian von Briesen [:moby] from comment #41)
> Created attachment 8791998 [details] [diff] [review]
> unified-editor.patch
> 
> I'm sorry it's been so long since I made an update. I've finished going
> through and unifying all of the box model/geometry editor code. There might
> be one or two things that still need to be fixed (like preventing negative
> values for certain attributes), but other than that, I believe this is ready
> for an initial review before I start working on unified editor tests.

Thanks!

Comment 44

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8791999 [details] [diff] [review]
unified-editor.patch

Unassigned me from the review, as we discussed with Patrick. I talked with him and we want to wait before actually merge this and land it.

Comment 45

[Sebastian Zartner [:sebo]]

What's the actual status of this? Maximilian, are you still working on this?

Sebastian

Comment 46

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Sebastian Zartner [:sebo] from comment #45)
> What's the actual status of this? Maximilian, are you still working on this?

We decided to pause the work on this feature for the time being, it might be take back and land in the next quarter / year.