Allow moving elements in content

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: bgrins, Assigned: pbro)

Tracking

(Depends on 1 bug, Blocks 4 bugs, {dev-doc-complete})

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, relnote-firefox 48+)

Details

(Whiteboard: [devtools-ux][creative-tools])

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
There is a lot of discussion already about this in 1123851.

See the description there - "The very high-level requirement is for devtools users to be able to move and resize elements easily in the page, almost as if modifying a layer in Photoshop or a similar program."
(Reporter)

Updated

4 years ago
Depends on: 1139186
(Assignee)

Comment 1

4 years ago
I have a couple of patches attached to bug 1123851 already, I will move them here soon.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Stephen, this is a follow-up to 1123851 where I started to implement a way to move and resize DOM elements in content. I had pinged you on the original bug for UX help, but we re-organized things a bit and the UX discussion should happen here now.

Some background info you should read: https://gist.github.com/captainbrosset/8a337290ba99b01f6425
Flags: needinfo?(shorlander)
(Assignee)

Comment 5

4 years ago
Renamed and rebased patches from bug 1123851. These should work on top of the patches in bug 1139186. Let me know if they don't.
(Reporter)

Comment 6

4 years ago
Comment on attachment 8576728 [details] [diff] [review]
1 - Add drag/drop support to the GeometryHighlighter to move positioned elements in content

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +2314,5 @@
>      }
>  
>      setIgnoreLayoutChanges(true);
>  
> +    svg.removeAttribute("hidden");

This line throws an error 'svg is not defined'
(Assignee)

Comment 7

4 years ago
I have rebased the patches, merged them together and added in the gcli command too. This patch is to help testing only. I'll now be uploading the patches for review on mozreview.
Attachment #8576727 - Attachment is obsolete: true
Attachment #8576728 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Duplicate of this bug: 733745
(Assignee)

Comment 9

4 years ago
Canary now ships with a "Layout editor" (go to the settings, experimental, then press SHIFT 6 times, then select layout editor).

It's very similar to the work I've been doing here, except it seems to only work for box-model regions: margin and padding.
The way this works right now is quite basic, I believe this is only a first step toward a better tool. As of now, you only get 2 controls, left and right of the selected element, dragging one of them will change the corresponding margin or padding (I don't know how they decide if margin or padding should be changed).
If you then drag the other control, the first margin or padding gets back to its original value (which I think is only a bug).
You can't change top/bottom for now.
The same sort of UX problems we were facing when working on this bug also show up on this new tool.

This I sort of like: the tool can be used when using the element picker icon. After you select an element, the box-model highlighter remains visible on screen, and you can start resizing the margin. If you then hover over elements in the markup-view, it disappears as it should, so that other elements get highlighted.

This is somehow counter-intuitive because it goes back to the always-on selection we used to have a couple of years ago with the highlighter, which people got frustrated with, but it's only when you use the element picker, and it's easy to dismiss, so it looks like a good idea to me.

However, I think the number of UI controls to change the size, position, border, padding, margin is bound to increase a lot from what they have today, so I wouldn't show these controls by default, maybe bring back the nodeinfobar menu we used to have to allow more things to be done once the highlighter is displayed.
(Assignee)

Updated

4 years ago
Whiteboard: [devtools-ux]
(Assignee)

Updated

4 years ago
Whiteboard: [devtools-ux] → [devtools-ux][creative-tools]
Attachment #8700586 - Flags: review?(pbrosset)
Helen, I did the icon used Sketch – based on the icon's size we already have in SVG – using the pixel mode and `svgo` to clean it; however the square of the icon seems blur compared to others in non retina display. I tried to fixed that, but I'm not sure why is happening. Any hint? We could also add a follow up bug to fix that.
Flags: needinfo?(hholmes)
Matteo, do you have a link to the SVG? I suspect this is a pixel-snapping issue that we saw in Bug 1210752.
Flags: needinfo?(shorlander)
Here the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9653c55e37cb&selectedJob=14781903

Unfortunately, it seems the test is failing on Windows 7 opt and Windows 8 x64 opt; but I'm not sure why. Patrick, do you have any idea about that?
Flags: needinfo?(pbrosset)
Posted image geometry-editor.svg
The image is in the patch, but for convenience I just attached as separate file.

Let me know if you're able to figure it out, and especially I will appreciate if you could tell me how I can avoid this effect the next time :)

Thanks!
Thanks for attaching it Matteo! I figured you might have it more handy... :)

So I unfortunately can't test this properly right now (I'm working from my Mac without an external monitor today, so I'm working at 300dpi). So, let me know if the new attachment still looks fuzzy.

So, I believe the blurriness is the result of pixel snapping. There are a few ways to avoid this that I've typed up for Illustrator here >> https://github.com/nt1m/firefox-svg-icons but that doesn't help you a bunch + it's in a weird spot.

For Sketch,

1. Build your icon at 16x16 with the Pixel Grid turned on. (View > Canvas > Show Pixels.)
2. Make sure that all x/y coordinates are full pixels for lines/rectangles (Position in the upper right hand corner of Sketch).
3. Expand all your paths so strokes expand properly as SVG gets resized. (Layer > Paths > Vectorize Stroke)
4. Align anything that isn't boxy to the pixel grid with item selected then Layer > Round to Nearest Pixel Edge.

The above docs I've written so far are currently in James + Lin's gitbook docs, but I'll make sure that I add these notes for Sketch in them and update nt1m's repo too. Er, unless the svg is still blurry. Then ~~back to the drawing board~~
Flags: needinfo?(hholmes)

Comment 16

3 years ago
Comment on attachment 8700586 [details] [diff] [review]
Allow moving and resizing elements in content

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

I've just tried it locally, works great !

Some feedback about the UX:
- It would be nice it could sync values with the rules view
- I think it makes more sense to have bug 940275 before we add this to the layout view
- It'd be nice to be able to edit the height/width if it's set

A bug I've found:
- Open geometry editor
- Edit a top/left/right/bottom value with the rules view
- You can't drag the geometry editor anymore

Also, it seems like that editor doesn't work for position: sticky elements, it's probably best to hide it in that case.

::: devtools/client/themes/images/geometry-editor.svg
@@ +1,2 @@
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
> +    <g fill="none" fill-rule="evenodd">

A small trick you can use in case you end up with fills and strokes both in the same file. 
Set the color="#f5f5f5" on the parent, then set stroke/fill="currentColor" on the children.

Or you could just Vectorize the strokes as Helen suggested.

Past that, the current fill for toolbar icons is #babec3. (but we might unify that later)

::: devtools/client/themes/layoutview.css
@@ +349,5 @@
> +  position: absolute;
> +  left: -1000em;
> +}
> +
> +#geometry-editor + label {

Can't we just use a normal button and just use the devtools-button class ?
It really just adds 2 lines to your JS, and spares you 20 lines of CSS.

Comment 17

3 years ago
(In reply to Tim Nguyen [:ntim] from comment #16)
> Comment on attachment 8700586 [details] [diff] [review]
> Allow moving and resizing elements in content
> 
> Review of attachment 8700586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've just tried it locally, works great !
> 
> Some feedback about the UX:
> - It would be nice it could sync values with the rules view
> - I think it makes more sense to have bug 940275 before we add this to the
> layout view
> - It'd be nice to be able to edit the height/width if it's set
One other thing, it might be nice as well to be able to remove/add properties. Let's say you want to get rid of the bottom property (because you want bottom: auto;), you can't do it with the highlighter.

Comment 18

3 years ago
Comment on attachment 8700756 [details]
geometry-editor-3.svg

This icon also seems blurry on Windows. Probably because it's resized from 17x17 to 16x16.

Comment 19

3 years ago
Here's a non-blurry version sized at 16x16 pixels. (well 15x15 with 1px offset)
(Assignee)

Comment 20

3 years ago
Comment on attachment 8700586 [details] [diff] [review]
Allow moving and resizing elements in content

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

This is really nice. I like it a lot.

Some general comments first:
- the width x height label that's displayed on the element may unfortunately sometimes hide the drag handle
For instance, with an element like this:
	width: 50px;
	height: 50px;
	background: gold;
	position: relative;
	top: 10px;
	left: 100px;
The 50px 50px label will hide the left drag handle.

- There are a couple of bugs related to rule-view not updating.
If 'top' is defined as an inline style, it only gets updated in the rule-view if the rule-view is the active panel in the sidebar (start the geometry editor, switch to the rule-view, and then drag the element around), but if the layout-view is the active panel, then it doesn't get updated (start the geometry editor, move the element around, then switch to the rule-view).
And if 'top' is defined in a CSS rule instead, then it's never updated when you move the element around.
Having new tests checking that the rule-view and computed-view are updated when the layout editor is used would be nice too.

- I like the changes you've made to the test (introducing this helper to simplify the tests), but I wished that was done as a separate commit, that makes it easier to review each one and determine what's code logic changes vs. test changes only.

- I think we should automatically hide the highlighter when we switch to another node. For example: select a positioned element, enable the geometry editor, and then select the <body>.
I think the editor should just go away in this case because otherwise it's impossible to get out of it (except if you re-select the same positioned element, but in some cases (large DOM tree) it might be hard to find).

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_01.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> + /* Globals defined in: devtools/client/inspector/test/head.js */
> + /* global openInspectorForURL, getHighlighterHelperFor */

So, we have an eslint plugin that's supposed to handle this for you automatically. We shouldn't have to maintain this list of globals here. The plugin basically checks if the file it's reviewing is a test, and if so tries to load the nearest head.js file and imports globals from there.
Please make sure it's installed correctly for you (ping mikeratcliffe if needed) and if it still doesn't work, file a bug for it, but we shouldn't have these globals here.

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_02.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> + /* Globals defined in: devtools/client/inspector/test/head.js */
> + /* global openInspectorForURL, getHighlighterHelperFor */

Same comment about eslint globals

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_03.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* Globals defined in: devtools/client/inspector/test/head.js */
> +/* global openInspectorForURL, getHighlighterHelperFor, TEST_URL_ROOT */

ditto

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_04.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> + /* Globals defined in: devtools/client/inspector/test/head.js */
> + /* global openInspectorForURL, getHighlighterHelperFor, TEST_URL_ROOT */

same here?

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_06.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + /* Globals defined in: devtools/client/inspector/test/head.js */
> + /* global openInspectorForURL, getHighlighterHelperFor, TEST_URL_ROOT */

Same comment about eslint globals here.

Also, this test fails for more locally
- left handler's x axis updated. - Got 77, expected 87
- top handler's y axis updated. - Got 42, expected 22
- left handler's x axis updated. - Got 87, expected 67

(I haven't run the others)

@@ +15,5 @@
> +const HIGHLIGHTER_TYPE = "GeometryEditorHighlighter";
> +
> +const SIDES = ["top", "right", "bottom", "left"];
> +
> +const TESTS = {

It'd help to have a comment before this object that explains what each test object is supposed to contain.

@@ +16,5 @@
> +
> +const SIDES = ["top", "right", "bottom", "left"];
> +
> +const TESTS = {
> +  "Drag top's handler along x and y, south-est direction": {

est should be east here (also several more times below)

@@ +17,5 @@
> +const SIDES = ["top", "right", "bottom", "left"];
> +
> +const TESTS = {
> +  "Drag top's handler along x and y, south-est direction": {
> +    "expects": "Only y axis is used to updated the top's element value",

Hm, this needs a bit of rephrasing. What about:
"The position is only updated along the Y axis when the we drag the top handler"
or something like this (same comments for other test cases below).

@@ +18,5 @@
> +
> +const TESTS = {
> +  "Drag top's handler along x and y, south-est direction": {
> +    "expects": "Only y axis is used to updated the top's element value",
> +    "steps": {

why steps, plural? This name makes me think this should be an array with several steps.

@@ +100,5 @@
> +
> +  ok((yield areMovingStepsCorrect(helper, data.steps)), data.expects);
> +}
> +
> +function* areMovingStepsCorrect(helper, steps) {

I'm a bit confused with this function's name.
Are "MovingSteps" a thing we are checking for correctness? Or should this be something like "areElementAndHighlighterMovedCorrectly" instead?

::: devtools/client/inspector/test/head.js
@@ +527,5 @@
>      let highlighter = yield front.getHighlighterByType(type);
>  
>      let prefix = "";
>  
> +    // internals for mouse events

This is a super nit, but please start comments with a capital letter and end with a period. That's the way most of our comments are written:

// Internals for mouse events.

@@ +528,5 @@
>  
>      let prefix = "";
>  
> +    // internals for mouse events
> +    let _x, _y;

nit: I don't think I've seen local variables being named with the _ prefix because there were internal stuff. We sort of sometimes to it for private object properties, but not even consistently, and I think there's a tendency in the team not to do it anymore.
So I suggest just naming these x and y (same for _node below).

@@ +530,5 @@
>  
> +    // internals for mouse events
> +    let _x, _y;
> +
> +    // node highlighted

// Highlighted node.

@@ +579,4 @@
>          yield testActor.synthesizeMouse(options);
>        },
>  
> +      mouse: new Proxy({}, {

Nice :)
I knew about proxies but rarely had the chance to use them.
Could you perhaps add a comment for the 'mouse' object letting people know how it's supposed to be used.

::: devtools/client/layoutview/view.js
@@ +541,5 @@
>      toolbox.highlighterUtils.unhighlight();
>    },
>  
> +  /**
> +   * Show the box-model highlighter on the currently selected element

s/box-model/geometry

@@ +552,5 @@
> +
> +    let toolbox = this.inspector.toolbox;
> +    let nodeFront = this.inspector.selection.nodeFront;
> +
> +    toolbox.highlighterUtils.getHighlighterByType("GeometryEditorHighlighter")

This method creates a new instance of the GeometryEditorHighlighter every time. You should call it just once and then use this._geometryEditorHighlighter instead.

@@ +560,5 @@
> +      });
> +  },
> +
> +  /**
> +   * Hide the box-model highlighter on the currently selected element

s/box-model/geometry

::: devtools/client/locales/en-US/layoutview.dtd
@@ +22,5 @@
> +<!ENTITY padding.tooltip          "padding">
> +<!ENTITY content.tooltip          "content">
> +
> +<!-- LOCALIZATION NOTE: The tooltip below appears as tooltip when the
> +  - geometry editor button is hovered -->

Please be a little bit more descriptive here, something like:
"This label is displayed as a tooltip that appears when hovering over the button that allows users to edit the position of an element in the page".
This is important for localizers.

::: devtools/client/themes/layoutview.css
@@ +349,5 @@
> +  position: absolute;
> +  left: -1000em;
> +}
> +
> +#geometry-editor + label {

I agree with Tim on this one, you should use a .devtools-button instead.

::: devtools/server/actors/highlighters/geometry-editor.js
@@ +117,5 @@
>    };
>  }
>  
>  /**
> + * Get the list of geometry properties that are actually set on the node given.

"on the provided node" instead of "on the node given"

@@ +212,5 @@
> +
> +  let { pageListenerTarget } = highlighterEnv;
> +
> +  // register the geometry editor instance to all events we're interested in.
> +  DOM_EVENTS.forEach(type => pageListenerTarget.addEventListener(type, this));

The problem with doing this in the constructor is that as soon as one of these highlighters is constructed, then the listeners are added and since your event handler doesn't check that the highlighter is shown first, clicking on the page won't work anymore.
I suggest only listening while the highlighter is visible. So starting to listen in 'show' and removing listeners in 'hide'.

I have another comment regarding listening to mousedown on the whole page target: doing this means that you later need to access the originalTarget to retrieve the nativeAnonymous node and check that it's a <circle> element. This is bad for the reasons detailed here: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/utils/markup.js#293
I suggest using addEventListenerForElement from CanvasFrameAnonymousContentHelper instead, at least for mousedown only. Then once drag is initiated, you'll need to listen to mousemove and up on the whole page target anyway.

@@ +401,5 @@
>      this.definedProperties = null;
>      this.offsetParent = null;
>    },
>  
> +  handleEvent(event) {

As commented earlier, this should only really execute when the highlighter is shown. Right now it's always called.

@@ +411,5 @@
> +        break;
> +      case "mousedown":
> +        // The mousedown event stars the drag & drop, so ignore everything else
> +        // but handlers
> +        if (originalTarget.tagName !== "circle") {

Maybe checking a class-name is more future-proof if we change the tagname or add more circles at some stage.

@@ +418,5 @@
> +          return false;
> +        }
> +
> +        // Get the side from the handler's id
> +        let m = originalTarget.id.match(/[^-]+$/);

If you use the addEventListenerForElement as suggested earlier, you wouldn't need to access the originalTarget here (which is precisely what we don't want to be doing anyway).
Also, why not add a data attribute to the element instead of having to extract the side using a regexp, like:
getAttributeForElement(theId, "data-side");

@@ +472,5 @@
> +        }
> +
> +        let delta = (GeoProp.isHorizontal(side) ? pageX - x : pageY - y) * inc;
> +
> +        sideProps.cssRule.style.setProperty(side, (value + delta) + unit);

So, here's a thing we haven't discussed yet: what exactly should be updated when we move an element: its inline style only? Or the source css rule that defined the position?

Right now, it'll update the source css rule.
While it may be a very powerful feature in some situations, it might not always be what users want.
Ultimately, I think this should almost be a mode the user should choose.

To illustrate this, create a test page with 10 divs with the same class, and in a CSS stylesheet, define a rule for this class with some position.
Then select one of the divs and start moving it around.

I had summarized this issue in this document a while ago: https://gist.github.com/captainbrosset/8a337290ba99b01f6425#use-cases-solved-by-the-tool

So, I think for the sake of landing something, we should just take a decision now and not worry about the 2 different modes, just pick one.
This is a relatively experimental feature that we don't want to expose too much just now (which is why having it a small icon in the box-model tab is good for now), so we can just take decisions and increment from there.
My take on this would be to always only update the inline style (just this element, not all elements that happen to share the same rule), but maybe we can get Helen to take that decision instead.

::: devtools/server/actors/styles.js
@@ +610,5 @@
> +    // editable
> +    return props.has("top") ||
> +          props.has("right") ||
> +          props.has("left") ||
> +          props.has("bottom");

Shouldn't this also check if the node is actually positioned?
Attachment #8700586 - Flags: review?(pbrosset)
(Assignee)

Comment 21

3 years ago
I've noticed something else while testing this: it's relatively easy to have 2 highlighters at the same time on the page: geometry and box-model, and when that happens it doesn't look very good (labels and infobars overlap, and are hard to read).

1 way is to enable the geometry editor and then move your mouse over the markup-view. I don't think we should really find a way to fix other than making sure the box-model highlighter has a higher z-index than the geometry editor maybe.

Another way is to enable the geometry editor and then move your mouse over the box-model diagram. I think we should fix this one though because it's really easy to reproduce and leads to the same sort of ugly results. One easy way would be to just not show the box-model highlighter if the geometry editor is enabled.

Somewhat related: if you enable the geometry editor and then click on the element picker icon, then you can't select an element, because mousemove events are not reaching the box-model highlighter (or something).
I think as soon as the user clicks on that icon to select another element, we should disable the geometry editor (you can listen to the toolbox's "picker-started" event).
Flags: needinfo?(pbrosset)
Attachment #8700586 - Attachment is obsolete: true
Comment on attachment 8732156 [details]
MozReview Request: Bug 1139187 - Unit Test for Geometry Editor; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41029/diff/1-2/
(In reply to Patrick Brosset [:pbro] from comment #20)

> Some general comments first:
> - the width x height label that's displayed on the element may unfortunately
> sometimes hide the drag handle

As discussed, I made the size label hidden by default – I didn't remove them, due the fact that we'll likely enable again in the future.

> - There are a couple of bugs related to rule-view not updating.
> If 'top' is defined as an inline style, it only gets updated in the
> rule-view if the rule-view is the active panel in the sidebar (start the
> geometry editor, switch to the rule-view, and then drag the element around),
> but if the layout-view is the active panel, then it doesn't get updated
> (start the geometry editor, move the element around, then switch to the
> rule-view).

That should be fixed now.

> - I like the changes you've made to the test (introducing this helper to
> simplify the tests), but I wished that was done as a separate commit, that
> makes it easier to review each one and determine what's code logic changes
> vs. test changes only.

I created a separate commit for the unit test; hope I didn't mixed anything in the process. :)

> - I think we should automatically hide the highlighter when we switch to
> another node. For example: select a positioned element, enable the geometry
> editor, and then select the <body>.
> I think the editor should just go away in this case because otherwise it's
> impossible to get out of it (except if you re-select the same positioned
> element, but in some cases (large DOM tree) it might be hard to find).

Done. Basically now the markup view emits event to the inspector when is hovering or leaving. I do not pass any object to those event 'cause we don't need atm, we can always do that in the future, if needed.

> > + /* Globals defined in: devtools/client/inspector/test/head.js */
> > + /* global openInspectorForURL, getHighlighterHelperFor */
> 
> So, we have an eslint plugin that's supposed to handle this for you
> automatically. We shouldn't have to maintain this list of globals here.

They should be removed from all files now.

> Also, this test fails for more locally
> - left handler's x axis updated. - Got 77, expected 87
> - top handler's y axis updated. - Got 42, expected 22
> - left handler's x axis updated. - Got 87, expected 67
> 
> (I haven't run the others)

I *hope* is fixed now. We'll see once try builds are over – I don't have it locally, but I also think depends by the platform.
I forced the reflow before checking the values, and also I reduce the element's size to ensure that devtools' toolbox won't cover it on small screen (e.g. in try server).

> > +  "Drag top's handler along x and y, south-est direction": {
> > +    "expects": "Only y axis is used to updated the top's element value",
> > +    "steps": {
> 
> why steps, plural? This name makes me think this should be an array with
> several steps.

I removed totally the `steps` property, it was anyway a leftover from a previous object's structure.

> I'm a bit confused with this function's name.
> Are "MovingSteps" a thing we are checking for correctness? Or should this be
> something like "areElementAndHighlighterMovedCorrectly" instead?

Should be fixed, hope now the name is better!

> > +      mouse: new Proxy({}, {
> 
> Nice :)
> I knew about proxies but rarely had the chance to use them.
> Could you perhaps add a comment for the 'mouse' object letting people know
> how it's supposed to be used.

Hope the description is clear enough, but let me know.

> I agree with Tim on this one, you should use a .devtools-button instead.

Now is using a .devtools-button, thanks Tim for the hint!

> > +  let { pageListenerTarget } = highlighterEnv;
> > +
> > +  // register the geometry editor instance to all events we're interested in.
> > +  DOM_EVENTS.forEach(type => pageListenerTarget.addEventListener(type, this));
> 
> The problem with doing this in the constructor is that as soon as one of
> these highlighters is constructed, then the listeners are added and since
> your event handler doesn't check that the highlighter is shown first,
> clicking on the page won't work anymore.
> I suggest only listening while the highlighter is visible. So starting to
> listen in 'show' and removing listeners in 'hide'.

As discussed, I left it on constructor, but avoiding to handle events if the root element is hidden.

> I have another comment regarding listening to mousedown on the whole page
> target: doing this means that you later need to access the originalTarget to
> retrieve the nativeAnonymous node and check that it's a <circle> element.
> This is bad for the reasons detailed here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> highlighters/utils/markup.js#293
> I suggest using addEventListenerForElement from
> CanvasFrameAnonymousContentHelper instead, at least for mousedown only. Then
> once drag is initiated, you'll need to listen to mousemove and up on the
> whole page target anyway.

At the end I decided to do that for `mousedown` as you suggested. I would love to improve the handling event as discussed, but it was out of the bug's scope.

> getAttributeForElement(theId, "data-side");

Done!

> > +        sideProps.cssRule.style.setProperty(side, (value + delta) + unit);
> 
> So, here's a thing we haven't discussed yet: what exactly should be updated
> when we move an element: its inline style only? Or the source css rule that
> defined the position?

As mentioned, currently the best approach is modifying the inline style for an element – it also means, pseudo element cannot be moved. We already discussed, but if you want I can list why currently we choose to do so.

> Ultimately, I think this should almost be a mode the user should choose.

Yes, my idea would be having the same icon we have now in box model, in the rule view (maybe on `position` css rule?), in this way the user would be able to choose if resize only the current element (via the current approach, box model and inline style), or modifying the css rule (and therefore potentially applying the changes to any element that use such rule).

> ::: devtools/server/actors/styles.js
> @@ +610,5 @@
> > +    // editable
> > +    return props.has("top") ||
> > +          props.has("right") ||
> > +          props.has("left") ||
> > +          props.has("bottom");

> Shouldn't this also check if the node is actually positioned?

It's already done, the method is removing such props before this check, if the `position` value is not absolute or relative.
Unfortunately, it seems I added a new intermittent – it doesn't fails now all the time, and on all platform now:  338 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_highlighter-geometry_06.js | right handler's x axis updated. - Got 322, expected 302 .

I'm trying to improve the test above, but any suggestion is more than welcome!
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41027/diff/1-2/
Comment on attachment 8732156 [details]
MozReview Request: Bug 1139187 - Unit Test for Geometry Editor; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41029/diff/2-3/
(Assignee)

Comment 30

3 years ago
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

https://reviewboard.mozilla.org/r/41027/#review38099

A few general comments first:
- the svg icon looks a little bit blurry on my external monitor (non-retina), or maybe that's intentional, I can't be sure, here's a screenshot: https://dl.dropboxusercontent.com/u/714210/geometry-editor-icon.png
- I think there should be a tooltip when hovering the button, otherwise it's hard to guess what clicking the button will do (maybe "Toggle the position editor tool to move the element in the page"). Update: after looking at the code, I see there's one defined, but somehow it doesn't show up on hover. I've heard similar problems of HTML tooltips not working in XUL documents before, I think jdescottes knows why, so you should ping him.
- I've found ways to break the feature a couple of times. Not sure if this is a stable way to reproduce, or if there are other ways, but here goes:
  - open data:text/html,<div style="width:100px;height:100px;background:pink;position:absolute;top:10px;">
  - enable the geometry editor on the div element
  - refresh the page
  - try to enable the geometry editor again => nothing appears

::: devtools/client/inspector/inspector.xul:284
(Diff revision 1)
> +                <!--
> +                <html:input type="checkbox" id="layout-geometry-editor"/>
> +                <html:label for="layout-geometry-editor" title="&geometry.button.tooltip;"></html:label>
> +                -->

This section is commented out, please remove it.

::: devtools/client/inspector/inspector.xul:322
(Diff revision 1)
>                <html:p class="layout-padding layout-right"><html:span data-box="padding" class="layout-editable" title="padding-right"></html:span></html:p>
>                <html:p class="layout-padding layout-bottom"><html:span data-box="padding" class="layout-editable" title="padding-bottom"></html:span></html:p>
>                <html:p class="layout-padding layout-left"><html:span data-box="padding" class="layout-editable" title="padding-left"></html:span></html:p>
>  
>                <html:p class="layout-size"><html:span data-box="content" title="&content.tooltip;"></html:span></html:p>
> +

This looks like an unrelated change (added an empty line).

::: devtools/client/inspector/layout/layout.js:259
(Diff revision 1)
> +    nodeGeometry.addEventListener("click", ({target}) => {
> +      if (target.hasAttribute("checked")) {
> +        target.removeAttribute("checked");
> +        this.hideGeometryEditor();
> +      } else {
> +        target.setAttribute("checked", "true");
> +        this.showGeometryEditor();
> +      }
> +    });

It would make the `init` function easier to read if you defined a method on the prototype instead of defining the function right into the `addEventListener`.

Maybe `this.onGeometryButtonClick`.

We do this for `onHighlightMouseOver` for example.

Also, it would be nice to remove this event listener in the destroy function, this way it makes it easier to reuse the box-model view in other places later, which we will want to do as part of the visual tools project.

::: devtools/client/inspector/layout/layout.js:417
(Diff revision 1)
>     * Selection 'new-node-front' event handler.
>     */
>    onNewSelection: function() {
>      let done = this.inspector.updating("layoutview");
> -    this.onNewNode().then(done, err => {
> +    this.onNewNode()
> +      .then(() => this.updateGeometryButton())

We already do `yield this.updateGeometryButton();` inside `update`. Shouldn't we call it only once?

::: devtools/client/inspector/layout/layout.js:631
(Diff revision 1)
> +   *   Indicates if the Geometry Editor have to been shown only if it's
> +   *   currently active but hidden.

Some rewording needed here:

"Indicates if the Geometry Editor should be shown only if it's active but hidden".

::: devtools/client/inspector/layout/layout.js:645
(Diff revision 1)
> +    if (showOnlyIfActive && !isActive) {
> +      return;
> +    }
> +
> +    if (this._geometryEditorHighlighter) {
> +      this._geometryEditorHighlighter.show(nodeFront);

If you don't care about returning the promise returned by `show`, at least you should catch potential rejections:

```
this._geometryEditorHighlighter.show(nodeFront).catch(e => console.error(e));
```

::: devtools/client/inspector/layout/layout.js:651
(Diff revision 1)
> +      return;
> +    }
> +
> +    toolbox.highlighterUtils
> +      .getHighlighterByType("GeometryEditorHighlighter").then(highlighter => {
> +        highlighter.show(nodeFront);

Same here.

::: devtools/client/inspector/layout/layout.js:655
(Diff revision 1)
> +        toolbox.on("picker-started", () => this.hideGeometryEditor());
> +
> +        // Temporary hide the geometry editor
> +        this.inspector.on("markup-view-leave", () =>
> +          this.showGeometryEditor(true));
> +        this.inspector.on("markup-view-hovered", () =>
> +          this.hideGeometryEditor(false));

These 3 new event handlers should be removed in destroy, so that we can potentially re-instantiate the layout-view later.

::: devtools/client/inspector/layout/layout.js:672
(Diff revision 1)
> +   * @param {Boolean} [updateButton=true]
> +   *   Indicates if the Geometry Editor's button needs to be unchecked too
> +   */
> +  hideGeometryEditor: function(updateButton = true) {
> +    if (this._geometryEditorHighlighter) {
> +      this._geometryEditorHighlighter.hide();

Same comment about catching rejections here.

::: devtools/client/inspector/layout/layout.js:683
(Diff revision 1)
> +    }
> +  },
> +
> +  /**
> +   * Update the visibility and the state of the geometry editor button,
> +   * based on the node highlighted.

"based on the selected node" makes more sense.

::: devtools/client/inspector/markup/markup.js:213
(Diff revision 1)
>          this._hideBoxModel();
>        }
>      }
>      this._showContainerAsHovered(container.node);
> +
> +    this._inspector.emit("markup-view-hovered");

Why not `this.emit("markup-view-hovered")` instead?
There's no real reason for the inspector to be the one broadcasting this event.
And you can easily access the markup-view from the inspector when you need to listen tol this event:
`inspector.markup`

::: devtools/client/inspector/markup/markup.js:347
(Diff revision 1)
>      if (this._hoveredNode) {
>        this.getContainer(this._hoveredNode).hovered = false;
>      }
>      this._hoveredNode = null;
> +
> +    this._inspector.emit("markup-view-leave");

Same comment here.

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_06.js:5
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + /* Globals defined in: devtools/client/inspector/test/head.js */

Don't think you need to add this comment, other tests don't do it, and this is common behavior for all our tests.

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_06.js:8
(Diff revision 1)
> +// Test that the arrows/handlers and offsetparent and currentnode elements of
> +// the geometry highlighter only appear when needed.

This test description comment is incorrect.

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_06.js:92
(Diff revision 1)
> +  ok((yield areElementAndHighlighterMovedCorrectly(
> +    helper, data.drag, data.by)), data.expects);
> +}
> +
> +function* areElementAndHighlighterMovedCorrectly(helper, side, by) {
> +  let { mouse, reflow, highlightedNode } = helper;

I don't see why `reflow` should be in the helper only. This is a useful function to have around, and it only depends on having access to a `testActor`, which all inspector tests do.
So I would suggest moving `reflow` from within `getHighlighterHelperFor` and to a standalone function inside head.js.

::: devtools/client/locales/en-US/layoutview.dtd:28
(Diff revision 1)
> -<!ENTITY content.tooltip        "content">
> +<!ENTITY content.tooltip          "content">
> +
> +<!-- LOCALIZATION NOTE: This label is displayed as a tooltip that appears when
> +  -  hovering over the button that allows users to edit the position of an
> +  -  element in the page. -->
> +<!ENTITY geometry.button.tooltip  "Edit layout">

"Edit position" is probably more correct here.

::: devtools/server/actors/highlighters.css:237
(Diff revision 1)
> +  stroke: #08c;
> +  fill: #87ceeb;

We have these colors, and more, all over the place in this file. We can't use theme variables here unfortunately, but at the very least, we could create new variables somwhere in this file to avoid duplication.
Let's do this later in another bug though, could you file one please?

::: devtools/server/actors/highlighters.css:262
(Diff revision 1)
> +
> +:-moz-native-anonymous .geometry-editor-handler-top.dragging,
> +:-moz-native-anonymous .geometry-editor-handler-right.dragging,
> +:-moz-native-anonymous .geometry-editor-handler-bottom.dragging,
> +:-moz-native-anonymous .geometry-editor-handler-left.dragging {
> +  fill: #08c; 

nit: trailing whitespace

::: devtools/server/actors/highlighters/geometry-editor.js:171
(Diff revision 1)
> +  // Post-process the list for invalid properties. This is done after the fact
> +  // because of cases like relative positioning with both top and bottom where
> +  // only top will actually be used, but both exists in css rules and computed
> +  // styles.
> +  for (let [name] of props) {
> +    let pos = getComputedStyle(node).position;

No need to get the computed style at each iteration of this loop.

::: devtools/server/actors/highlighters/geometry-editor.js:421
(Diff revision 1)
>      this.definedProperties.clear();
>      this.definedProperties = null;
>      this.offsetParent = null;
>    },
>  
> -  getElement: function(id) {
> +  handleEvent(event, id) {

Other methods in this object do not use this syntax, so either change all other methods to be the same (but in a separate patch) or be consistent here and use:
`handleEvent: function(event, id) {`

::: devtools/server/actors/highlighters/geometry-editor.js:427
(Diff revision 1)
> -    return this.markup.getElement(this.ID_CLASS_PREFIX + id);
> -  },
> +    // No event handling if the highlighter is hidden
> +    if (this.getElement("root").hasAttribute("hidden")) {
> +      return;
> +    }
> +
> +    const { originalTarget, type, pageX, pageY } = event;

originalTarget isn't used here, please remove it.

::: devtools/server/actors/highlighters/geometry-editor.js:435
(Diff revision 1)
> +      case "pagehide":
> +        this.destroy();
> +        break;
> +      case "mousedown":
> +        // The mousedown event is intended only for the handler
> +        if (!id) return;

ESLint error here: need to wrap the body in { }

::: devtools/server/actors/highlighters/geometry-editor.js:440
(Diff revision 1)
> -    if (!this.currentNode) {
> -      return props;
> -    }
>  
> -    // Get the list of css rules applying to the current node.
> -    let cssRules = getCSSStyleRules(this.currentNode);
> +        if (handlerSide) {
> +          let side = handlerSide;

The `handlerSide` variable is very short lived, maybe call it `side` instead directly and avoid creating 2 variables for the same thing.

::: devtools/server/actors/highlighters/geometry-editor.js:444
(Diff revision 1)
> -    // Get the list of css rules applying to the current node.
> -    let cssRules = getCSSStyleRules(this.currentNode);
> -    for (let i = 0; i < cssRules.Count(); i++) {
> -      let rule = cssRules.GetElementAt(i);
> -      for (let name of GeoProp.allProps()) {
> -        let value = rule.style.getPropertyValue(name);
> +        if (handlerSide) {
> +          let side = handlerSide;
> +          let sideProp = this.definedProperties.get(side);
> +
> +          if (!sideProp) {
> +            return false;

ESLint complains about mixed return types here, because a few lines earlier you have a simple `return` with no value. We have a rule that checks that you return consistent types from functions.

::: devtools/server/actors/highlighters/geometry-editor.js:495
(Diff revision 1)
> -        props.delete(name);
> -      }
> +        }
>  
> -      // Bottom/right on relative positioned elements are only used if top/left
> -      // are not defined.
> -      let hasRightAndLeft = name === "right" && props.has("left");
> +        let delta = (GeoProp.isHorizontal(side) ? pageX - x : pageY - y) * inc;
> +
> +        //sideProps.cssRule.style.setProperty(side, (value + delta) + unit);

Commented out code should be removed.

::: devtools/server/actors/highlighters/geometry-editor.js:497
(Diff revision 1)
>  
> -      // Bottom/right on relative positioned elements are only used if top/left
> -      // are not defined.
> -      let hasRightAndLeft = name === "right" && props.has("left");
> -      let hasBottomAndTop = name === "bottom" && props.has("top");
> -      if (pos === "relative" && (hasRightAndLeft || hasBottomAndTop)) {
> +        let delta = (GeoProp.isHorizontal(side) ? pageX - x : pageY - y) * inc;
> +
> +        //sideProps.cssRule.style.setProperty(side, (value + delta) + unit);
> +        this.currentNode.style.setProperty(
> +          side, (value + delta) + unit, "important");

Please add a comment explaining in which cases !important is needed and why we have to have it here.

::: devtools/server/actors/highlighters/geometry-editor.js
(Diff revision 1)
>  
>      // Update the highlighters and arrows.
>      this.updateOffsetParent();
>      this.updateCurrentNode();
>      this.updateArrows();
> -    this.updateSize();

What is the rationale for removing this? Now the method `updateSize` isn't being called anymore.

::: devtools/server/actors/highlighters/geometry-editor.js:645
(Diff revision 1)
> +  // NOTE: Size labels are present but always hidden, since in some conditions
> +  //       they cover the handles. Because we do not use width / height
> +  //       currently as properties for resizing, that's not a big deal; but we
> +  //       keep the code because we'll likely enable it in the future.

Ah ok, I see now.
I don't know about keeping dead code around however. I think I'd prefer to remove this entirely than to keep it around potentially for ever if we never re-enable them.
This highlight has been completely hidden for a very long time, so it's not like we'de be removing something that is used already. I'd rather ship something simple than keep complex code around that we're not using.

::: devtools/server/actors/highlighters/utils/markup.js:462
(Diff revision 1)
>  
>    getElement: function(id) {
> -    let self = this;
> +    let classList = new ClassList(this.getAttributeForElement(id, "class"));
> +
> +    on(classList, "update", () => {
> +      this.setAttributeForElement(id, "class", classList.toString())

nit: missing semi-colon

::: devtools/server/actors/styles.js:612
(Diff revision 1)
> +    return props.has("top") ||
> +          props.has("right") ||
> +          props.has("left") ||
> +          props.has("bottom");

nit: the subsequent lines should be vertically aligned with the first `props`.
Attachment #8732155 - Flags: review?(pbrosset)
(Assignee)

Updated

3 years ago
Attachment #8732156 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 31

3 years ago
Comment on attachment 8732156 [details]
MozReview Request: Bug 1139187 - Unit Test for Geometry Editor; r=pbro

https://reviewboard.mozilla.org/r/41029/#review38111

Thanks for the massive cleanup of existing tests!

::: devtools/client/inspector/test/browser.ini:60
(Diff revision 2)
>  [browser_inspector_highlighter-geometry_01.js]
>  [browser_inspector_highlighter-geometry_02.js]
>  [browser_inspector_highlighter-geometry_03.js]
>  [browser_inspector_highlighter-geometry_04.js]
>  [browser_inspector_highlighter-geometry_05.js]
> +[browser_inspector_highlighter-geometry_06.js]

You're adding this new test in browser.ini in this patch, but in this patch, you're only modifying the file, not creating it. So you should move this browser.ini change to the patch that actually creates browser_inspector_highlighter-geometry_06.js.

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_01.js:5
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> + /* Globals defined in: devtools/client/inspector/test/head.js */

As said in my previous review, I don't think these comments are needed.

::: devtools/client/inspector/test/browser_inspector_highlighter-geometry_05.js:36
(Diff revision 2)
>    selector: "#absolute-container",
>    isOffsetParentVisible: false,
>    isCurrentNodeVisible: true,
> -  hasVisibleArrows: false,
> +  hasVisibleArrowsAndHandlers: false
> -  isSizeVisible: true
> -}, {
> +  }, {

nit: remove the 2 space indentation on this line.
Okay, I improved the unit test, now instead of just forcing the reflow, we also wait for the next repaint. That seems solved the issue at least on my local machine (using  --run-until-failure --repeat 3000 for the test).
I also fixed an issue on `isPositionEditable` – we process only nodes that are ELEMENT_NODE.

I'm currently running another try build.
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41027/diff/2-3/
Attachment #8732155 - Flags: review?(pbrosset)
Comment on attachment 8732156 [details]
MozReview Request: Bug 1139187 - Unit Test for Geometry Editor; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41029/diff/3-4/
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41027/diff/3-4/
Comment on attachment 8732156 [details]
MozReview Request: Bug 1139187 - Unit Test for Geometry Editor; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41029/diff/4-5/
Patrick, the last push is related only to removing `onWillNavigate` listener on `destroy` method.

Here the try builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c4414170d6a

The errors doesn't seem related to this patch.
Flags: needinfo?(pbrosset)
(Assignee)

Updated

3 years ago
Attachment #8732155 - Flags: review?(pbrosset)
(Assignee)

Comment 38

3 years ago
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

https://reviewboard.mozilla.org/r/41027/#review39763

Thanks for addressing all of my comments.
I think we're good to go.
One bug remains: the tooltip "edit position" still does not appear. Julian had shared a tip about this a while back on the mailing list: https://groups.google.com/forum/#!searchin/mozilla.dev.developer-tools/tooltip$20xul/mozilla.dev.developer-tools/1lWz5uZWSP0/JdplQiwzAwAJ

::: devtools/server/actors/highlighters/geometry-editor.js:366
(Diff revision 4)
> -
>      return container;
>    },
>  
>    destroy: function() {
> +    if (!this.highlighterEnv) return;

Please wrap `return` in curly braces, ESLint doesn't like `if` statements without them.
Also, this early return requires a comment that explain when/why this may happen.
(Assignee)

Updated

3 years ago
Flags: needinfo?(pbrosset)
Attachment #8732155 - Flags: review?(pbrosset)
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41027/diff/4-5/
Comment on attachment 8732156 [details]
MozReview Request: Bug 1139187 - Unit Test for Geometry Editor; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41029/diff/5-6/
(Assignee)

Comment 41

3 years ago
Comment on attachment 8732155 [details]
MozReview Request: Bug 1139187 - Allow moving and resizing elements in content; r=pbro

https://reviewboard.mozilla.org/r/41027/#review39835

I think we should ship this.
I have 2 requests for follow-ups:

- Find ways to deal with the labels covering up the handles. It's especially easy to do when the top/left dimensions are small enough. Then you end up with the labels hiding the handles and you can't grab them anymore.
- Also, I found another back/forward navigation problem, this time on http://jsbin.com/ . If you load the site, there should be a big head at the top with the jsbin logo and a bunch of links (features, blog, ...) (if you don't have it, click on the jsbin logo to expand the header). Now, inside this header, there's a close button, it's absolutely positioned. Enabling the geometry editor on this element works, but then if you hit back, it disapears and can't be enabled anymore. The problem seems to come from the fact that jsbin pushes history state in js and that might mess with how we destroy/init the highlighter.
Attachment #8732155 - Flags: review?(pbrosset) → review+
Thanks Patrick!
I filed bug 1260794 and bug 1260795. I think the "history state" thingy is affecting most of our highlighters too, so I made it more generic.

Comment 44

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc7bd1234a7f
https://hg.mozilla.org/mozilla-central/rev/5fd9689bcac1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1136594
Blocks: 1136594
No longer depends on: 1136594
While triaging I have spotted a few bugs directly linked to this one. Anybody knows if we have a meta-bug tracking the features/bugs related to the geometry highlighter? Should we create one?

Also did resizing land here? The summary and commit message mention moving and resizing, but I think only moving was implemented?
Flags: needinfo?(zer0)
Flags: needinfo?(pbrosset)
(Assignee)

Comment 46

3 years ago
(In reply to Julian Descottes [:jdescottes] from comment #45)
> While triaging I have spotted a few bugs directly linked to this one.
> Anybody knows if we have a meta-bug tracking the features/bugs related to
> the geometry highlighter? Should we create one?
afaik we don't. So, yeah, we should create one.
> Also did resizing land here? The summary and commit message mention moving
> and resizing, but I think only moving was implemented?
Only moving was implemented. We could create another bug for resizing.
Flags: needinfo?(zer0)
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset [:pbro] from comment #46)
> (In reply to Julian Descottes [:jdescottes] from comment #45)
> > While triaging I have spotted a few bugs directly linked to this one.
> > Anybody knows if we have a meta-bug tracking the features/bugs related to
> > the geometry highlighter? Should we create one?
> afaik we don't. So, yeah, we should create one.
> > Also did resizing land here? The summary and commit message mention moving
> > and resizing, but I think only moving was implemented?
> Only moving was implemented. We could create another bug for resizing.

I've changed the summary accordingly. Resizing is already covered in bug 1136594.

Sebastian
Summary: Allow moving and resizing elements in content → Allow moving elements in content
(Assignee)

Comment 49

3 years ago
(In reply to Sebastian Zartner [:sebo] from comment #48)
> Described at
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_the_box_model#Editing_box_model_in_content and listed in
> https://developer.mozilla.org/en-US/Firefox/Releases/48.
> 
> Please let me know whether the description is ok.
> 
> Sebastian
Thanks Sebastian, this looks great. One detail though:
- for absolute nodes, the dashed line represent the offset parent
- for relative nodes, the dashed line represent the original position of the node
(In reply to Patrick Brosset [:pbro] from comment #49)
> (In reply to Sebastian Zartner [:sebo] from comment #48)
> > Described at
> > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> > Examine_and_edit_the_box_model#Editing_box_model_in_content and listed in
> > https://developer.mozilla.org/en-US/Firefox/Releases/48.
> > 
> > Please let me know whether the description is ok.
> > 
> > Sebastian
> Thanks Sebastian, this looks great. One detail though:
> - for absolute nodes, the dashed line represent the offset parent
> - for relative nodes, the dashed line represent the original position of the
> node

Thank you for the note! I've adjusted the description accordingly.

Sebastian
I've added a little video for this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_the_box_model#Editing_box_model_in_content.

But, I don't think "editing the box model" is the right place for this. This isn't what I think of when I think of editing the box model. Maybe it should have its own page under https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to, named "Reposition elements in the page" or something.

What do you think?
Flags: needinfo?(pbrosset)
Flags: needinfo?(sebastianzartner)
(Assignee)

Comment 52

3 years ago
(In reply to Will Bamberg [:wbamberg] from comment #51)
> I've added a little video for this:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_the_box_model#Editing_box_model_in_content.
Thanks, this looks great.
> But, I don't think "editing the box model" is the right place for this. This
> isn't what I think of when I think of editing the box model. Maybe it should
> have its own page under
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to, named
> "Reposition elements in the page" or something.
You're right, positioning is different from box model. We added the button in the "box model" tool as a first iteration, but it's likely to change in the future. So in that sense, yes, having its own HowTo page would make sense, this way we can easily edit it and link it from various places.
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #52)
> (In reply to Will Bamberg [:wbamberg] from comment #51)
> > But, I don't think "editing the box model" is the right place for this. This
> > isn't what I think of when I think of editing the box model. Maybe it should
> > have its own page under
> > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to, named
> > "Reposition elements in the page" or something.
> You're right, positioning is different from box model. We added the button
> in the "box model" tool as a first iteration, but it's likely to change in
> the future. So in that sense, yes, having its own HowTo page would make
> sense, this way we can easily edit it and link it from various places.

One of the next iterations is to allow adjusting the margins, borders and paddings in-content (bug 1136594). So, I thought that "Examine and edit the box model" would be the right place for this. Though I'm fine with moving the description to a separate page. If it's named "Reposition elements in the page", we'll probably have to renamed it later, though.

Sebastian
Flags: needinfo?(sebastianzartner)
(In reply to Sebastian Zartner [:sebo] from comment #53)
> (In reply to Patrick Brosset <:pbro> from comment #52)
> > (In reply to Will Bamberg [:wbamberg] from comment #51)
> > > But, I don't think "editing the box model" is the right place for this. This
> > > isn't what I think of when I think of editing the box model. Maybe it should
> > > have its own page under
> > > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to, named
> > > "Reposition elements in the page" or something.
> > You're right, positioning is different from box model. We added the button
> > in the "box model" tool as a first iteration, but it's likely to change in
> > the future. So in that sense, yes, having its own HowTo page would make
> > sense, this way we can easily edit it and link it from various places.
> 
> One of the next iterations is to allow adjusting the margins, borders and
> paddings in-content (bug 1136594). So, I thought that "Examine and edit the
> box model" would be the right place for this. Though I'm fine with moving
> the description to a separate page. If it's named "Reposition elements in
> the page", we'll probably have to renamed it later, though.
> 

That's a good point. What if we called it "Reposition and edit elements in the page" (or "in content"?), and noted that editing is not yet supported? Or "Edit element geometry in the page"?


> Sebastian
Flags: needinfo?(sebastianzartner)
We may call it "Reposition elements in the page" now, but rename it later to something like "Reposition elements and edit their geometry in the page" later. Does that sound good?

Sebastian
Flags: needinfo?(sebastianzartner)
Sounds good. I've now done this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Reposition_elements_in_the_page. Thanks for the thoughts, everyone (and for writing the docs, in particular, Sebastian).

Comment 57

3 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: new devtools feature
[Suggested wording]: Geometry editor to move absolute and fixed positioned elements.
[Links (documentation, blog post, etc)]: see comment 56
relnote-firefox: --- → ?
This was added a while back :)

Updated

2 years ago
Depends on: 1326939

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.