1123851 - Add an element geometry highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

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.

I think there is a significant amount of users that are left out today by browser devtools because their UIs are often too programmer-oriented or too complex.

Being able to modify some elements of the design of a webpage by directly moving things around would be pretty powerful for some people.
It would help make our tools accessible to a broader audience.

Now, in order to make this a realistic bug to fix, there are various questions to be answered:

- which elements should be movable/resizable?
- how should this be activated?
- how should the changes be made to the underlying HTML/CSS?

I think there may be 2 distinct main ways to go about this:

1 - either allow any element to be resized and moved without constraints, setting the appropriate position/left/top/width/height/... properties (probably on element.style) as we go,
2 - or only allow "positioned" elements to be moved, even then, only allow those properties that are already set to be changed. For instance, an absolutely positioned element that has the properties left, bottom and width set should be movable along the x and y axis, from the bottom and left, and be resizable along the x axis only.

1 is more flexible and probably more fun to play with, but the big advantage of 2 is that it would only allow modifications within the constraints of the existing CSS rules, while 1 would mess up with the CSS a lot.
With 2, we could even make the tool update the right css property inside the right css rule (so if the element is moved to the left, the original left property would be updated).

The other advantage of 2 is that it avoids breaking the layout in weird ways. For instance if an element is absolutely positioned and has both left and right defined, 1 would allow resizing but that would mess up with the style because it would add a width property.
2 however would only allow the element to be "moved" from the left or the right. This would end up resizing it too, but keeping the original intent of the CSS code.

The other question is how should this element transformer tool be activated.
So far, the inspector is the preferred way to modify elements in the page. I think one way would be to add a button in the rule-view next to "position:absolute/relative/fixed" properties.

But given requirement of trying to make our tools available to users from various background, it's a bit sad to ask them to have to select the element in the inspector, and then hunt for the little button in the rule-view panel.

Comment 1

[Patrick Brosset <:pbro>]

Needinfo'ing several people as this bug can hardly be implemented as is for now. We need to refine what we want to do exactly.
For info, I started hacking on a little prototype along the lines of 2 (described above), to fist see how easy it was to implement on the server-side. Turns out it's not that bad. I'll upload the patch for info only so far. The front-end part is just awful for now, it just threw it together quickly to be able to test the new tool.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: element-transformer-highlighter.patch

Comment 3

[Victor Porof [:vporof][:vp]]

wow

Comment 4

[Patrick Brosset <:pbro>]

Attachment: element-transformer.gif

Here's an idea:

Once the transform tool is activated for an element:

- We highlight the offsetParent (the positioned ancestor, or documentElement if none), so that it's easy to see what left/top/right/bottom are measured from.
- We draw arrows with labels for each of the top/left/bottom/right properties that are set on the element. The arrows would go from the edges of the offsetParent to the edges of the element. The labels would contain the distances (e.g. "200px" or "5em", ...).
- We add drag handles where the arrows are pointing (i.e. around the element's edges) to allow users to change the top/left/bottom/right distances.
- We do the same for width and height, depending on which is set, if any.

The attached animated GIF shows an example element without and with the tool overlaid on top.
The element (div#footer-image-two) is absolutely positioned inside another, footer, element (div#footer-wrapper-wh). It is positioned with left and top and has its width and height set.
In this example, and with the proposed solution, we would draw 2 rectangles and 4 arrows, and let the user drag the handles up and down or left and right to resize the arrows and therefore move or resize the element.

This approach could work with anything that has at least one of width,height,top,left,right,bottom set.
This could be activated by a button next to these properties in the rule view, or from a new contextual menu item (when right-clicking on the node), or by adding a new button somewhere in the inspector.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I like the transform tool... awesome idea.

Firebug automatically outlines the positioned ancestor by default when highlighting or at least it did a gazillion years ago when I implemented it.

Do we really have to do all of this in content? I hate to use the D word but a lot can be said for Dreamweaver's property panel and the in content sizing etc. I don't think dragging elements into the page is a good idea but editing current elements makes sense.

We could get a heckuvalot of ideas from there.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> Do we really have to do all of this in content? I hate to use the D word but
> a lot can be said for Dreamweaver's property panel and the in content sizing
> etc. I don't think dragging elements into the page is a good idea but
> editing current elements makes sense.
One idea here would be to somewhat merge my proposal in attachment 8552471 [details] and the layout view we already have.
The layout view today already represents the node with various dimensions, and we have bugs on file to add things like top/left/right/bottom to it (bug 940275 I think), so maybe we could have a beefed up version of the layout-view that allowed changing more than just the box-model values.

Having said this, I still like the in-content element dragging behavior, the whole idea behind this is to make hacking on webpages more accessible to people that might not necessarily know how the information in the rule-view or style-editor is organized.

Comment 7

[Brian Grinstead [:bgrins]]

I really like the idea.  Some thoughts in no particular order:

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #0)
> I think there may be 2 distinct main ways to go about this:
> 
> 1 - either allow any element to be resized and moved without constraints,
> setting the appropriate position/left/top/width/height/... properties
> (probably on element.style) as we go,
> 2 - or only allow "positioned" elements to be moved, even then, only allow
> those properties that are already set to be changed. For instance, an
> absolutely positioned element that has the properties left, bottom and width
> set should be movable along the x and y axis, from the bottom and left, and
> be resizable along the x axis only.

> The other advantage of 2 is that it avoids breaking the layout in weird
> ways. For instance if an element is absolutely positioned and has both left
> and right defined, 1 would allow resizing but that would mess up with the
> style because it would add a width property.
> 2 however would only allow the element to be "moved" from the left or the
> right. This would end up resizing it too, but keeping the original intent of
> the CSS code.

I agree and think the only sane way to handle this would be to show overlays/editors only on things that make sense (i.e. won't break your layout in complicated ways).  The main concern with this is that if the feature only works sometimes (and it's not immediately obvious to a user when) then it could be frustrating when you want to move something around and the controls don't show up.  At this stage it doesn't hurt to experiment and see how useful it is though.

Additional tools could theoretically be added for other position/display types, but I'm not sure what they would look like (flex box, floats, tables).

We should look at http://bluegriffon.org/ for some prior work.  I know there is capability for doing things like CSS rotations, for instance: https://www.youtube.com/watch?v=hns2No8ehIs.

> 1 is more flexible and probably more fun to play with, but the big advantage
> of 2 is that it would only allow modifications within the constraints of the
> existing CSS rules, while 1 would mess up with the CSS a lot.
> With 2, we could even make the tool update the right css property inside the
> right css rule (so if the element is moved to the left, the original left
> property would be updated).

We should consider this use case this while designing and implementing undo/redo in rule view (bug 773510).  We will most likely want to add an undo state only once a drag has completely finished (similar to if you hold the up button on a numeric inplace editor).

> The other question is how should this element transformer tool be activated.
> So far, the inspector is the preferred way to modify elements in the page. I
> think one way would be to add a button in the rule-view next to
> "position:absolute/relative/fixed" properties.
> 
> But given requirement of trying to make our tools available to users from
> various background, it's a bit sad to ask them to have to select the element
> in the inspector, and then hunt for the little button in the rule-view panel.

I don't think we have a better way to do this right now (beyond hooking up a keyboard shortcut to do the same thing).  It'd be a good time to get some UX brainstorming about this feature in general, including new ways to select / edit elements.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead (unavailable through 1-26) [:bgrins] from comment #7)
> I agree and think the only sane way to handle this would be to show
> overlays/editors only on things that make sense (i.e. won't break your
> layout in complicated ways).  The main concern with this is that if the
> feature only works sometimes (and it's not immediately obvious to a user
> when) then it could be frustrating when you want to move something around
> and the controls don't show up.  At this stage it doesn't hurt to experiment
> and see how useful it is though.
I agree that is a concern with this approach indeed. I was discussing this with Joe yesterday and he said that perhaps there could be ways in the tool to "activate" more editors. So for instance if only width and height were set on an element but it wasn't positioned, there could be a button somewhere to make it positioned, and therefore the editors for top/left/right/bottom would then appear. So this would be a conscious action from the user, just like writing position:absolute; in the rule-view is, not something automatic.

> Additional tools could theoretically be added for other position/display
> types, but I'm not sure what they would look like (flex box, floats, tables).
Me neither, but there are some ideas for flex box in bug 1114973.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug1123851-element-transformer.patch

New approach, and cleaned up the code a lot.
With this patch, pressing T in the markup-view (that's just for testing) while a node is selected will display:
- a highlighter around the node
- a highlighter around the offsetParent if any
- arrows for any of the dimensions or distances that are set.
I haven't gone as far as to make the arrows resizable for now, but since I had done this in the previous patch, that should be possible.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: bug1123851-element-transformer WIP.patch

So, this patch sort of works.

There's probably plenty of unimplemented edge cases and bugs, but it's enough to demo the idea.
Also the corresponding style values in the rule-view do not get updated in real time.

There's no UI yet to active/deactivate the transformer tool (it just appears when I press the T key with the element focused in the markup-view).

Here's a quick screencast https://dl.dropboxusercontent.com/u/714210/element-transformer.mp4

Comment 11

[Joe Walker [:jwalker]]

I like it.

We're updating the rules at source rather than just adding an element style, right? I guess eventually we should make the Box Model use the same system.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #11)
> I like it.
> 
> We're updating the rules at source rather than just adding an element style,
> right?
Yes indeed, the tool retrieves the list of position/size-related properties from the css rules that apply to the element and makes changes to those rules when the element is moved/resized.
> I guess eventually we should make the Box Model use the same system.
Do you mean having an in-content editor for the box model (which I think we totally should), or making the updates in the "layout" tab of the inspector be done to the source css rules (which i also think we should do)?

Comment 13

[Joe Walker [:jwalker]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> > I guess eventually we should make the Box Model use the same system.
> Do you mean having an in-content editor for the box model (which I think we
> totally should), or making the updates in the "layout" tab of the inspector
> be done to the source css rules (which i also think we should do)?

I was thinking about the tab in the inspector, but you're right, the more we can do to push that kind of thing to content the better.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug1123851-1-element-geometry-highlighter v1.patch

Ok let's start simple. This part 1 adds a new highlighter whose job is only to highlight the node, its offsetParent if any, and all geometry properties that are set.
It does not make the element's dimension or position editable yet, but I figured I'd better try land this first.

Tests will come in a separate patch later.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug1123851-2-gcli-element-geometry-command v1.patch

This part 2 just adds a "geometry" parameter to the already existing highlight command. When passed, instead of using the BoxModelHighlighter, the new Geometry highlighter will be used instead and therefore, the dimension and position properties that are set will be shown too.
The new geometry highlighter accepts the same options as the box-model highlighter anyway, so all other options to the highlight command will still work.

I've also added a couple of new options to the new highlighter for future use.

Test for this command coming in a separate patch.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: bug1123851-3-tests v1.patch

Part 3 - Tests
Flagging both Joe and Brian for review (Joe because there's a couple of new gcli tests, and Brian for the rest).

Comment 17

[Joe Walker [:jwalker]]

Comment on attachment 8557886 [details] [diff] [review]
bug1123851-2-gcli-element-geometry-command v1.patch

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

::: toolkit/devtools/gcli/commands/highlight.js
@@ +115,5 @@
>  
>        let env = context.environment;
>  
>        // Unhighlight on navigate
> +      env.target.once("navigate", destroyHighlighters);

I wonder if this shouldn't be a feature of the highlighter system. Are we ever going to want highlighters that are cross page?
Perhaps this isn't the right place to fix this though.

::: toolkit/devtools/server/actors/highlighter.js
@@ +1781,5 @@
>    this.offsetParentNodeHighlighter.regionFill.padding = "transparent";
>  
>    // Used to highlight the current node.
>    this.nodeHighlighter = new BoxModelHighlighter(tabActor);
> +  this.nodeHighlighter.regionFill = this.regionFill = {};

Why is this aliasing is a good idea?, intuitively seems dangerous (to me at least!). Maybe we should have a comment?

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #17)
> Comment on attachment 8557886 [details] [diff] [review]
> bug1123851-2-gcli-element-geometry-command v1.patch
> 
> Review of attachment 8557886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/gcli/commands/highlight.js
> @@ +115,5 @@
> >  
> >        let env = context.environment;
> >  
> >        // Unhighlight on navigate
> > +      env.target.once("navigate", destroyHighlighters);
> 
> I wonder if this shouldn't be a feature of the highlighter system. Are we
> ever going to want highlighters that are cross page?
Yes, in fact highlighter actor instances are kept until the debugger server is stopped. And so, they live across page navigation. It's just that they are hidden, but they are kept around until needed again on the next page.

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +1781,5 @@
> >    this.offsetParentNodeHighlighter.regionFill.padding = "transparent";
> >  
> >    // Used to highlight the current node.
> >    this.nodeHighlighter = new BoxModelHighlighter(tabActor);
> > +  this.nodeHighlighter.regionFill = this.regionFill = {};
> 
> Why is this aliasing is a good idea?, intuitively seems dangerous (to me at
> least!). Maybe we should have a comment?
I will add a comment indeed. For some reason I can't fully remember, the regionFill configuration isn't a highlighter option like the other options, it's just a property on the instance, and I wanted this new highlighter to expose the same configuration property for the underlying BoxModelHighlighter used to highlight the node.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bug1123851-2-gcli-element-geometry-command v2.patch

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8557961 [details] [diff] [review]
bug1123851-2-gcli-element-geometry-command v2.patch

Carrying R+ over

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> Created attachment 8557885 [details] [diff] [review]
> bug1123851-1-element-geometry-highlighter v1.patch
> 
> Ok let's start simple. This part 1 adds a new highlighter whose job is only
> to highlight the node, its offsetParent if any, and all geometry properties
> that are set.
> It does not make the element's dimension or position editable yet, but I
> figured I'd better try land this first.
> 
> Tests will come in a separate patch later.

Starting the review, but first:

Do we want the future editor controls to all happen inside of the highlighter canvas frame API?  The challenge is that we don't have a DOM to work with, which is workaround-able for buttons, but seems it would become unwieldy for things with more interaction like text boxes or even sliders.  I'm concerned that we may end up halfway reimplementing many DOM APIs to make the controls work how we want.

The benefit is that it would display on remote devices, but I'm not sure how useful it would be on all screen sizes / input types.  It would be pretty cool to do some of this on a touch screen, but the controls may be too small to work with effectively, or too big so that they were covering up a lot of the screen.

I guess the alternative would be to draw the controls in browser chrome on top of the content.  There would still be highlighter bits for drawing the size, but the editing itself would happen in some kind of popup or overlay.  I'm not sure what is the right direction to go.  Any thoughts?

Comment 22

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #21)
> Do we want the future editor controls to all happen inside of the
> highlighter canvas frame API?  The challenge is that we don't have a DOM to
> work with, which is workaround-able for buttons, but seems it would become
> unwieldy for things with more interaction like text boxes or even sliders. 
> I'm concerned that we may end up halfway reimplementing many DOM APIs to
> make the controls work how we want.
> 
> The benefit is that it would display on remote devices, but I'm not sure how
> useful it would be on all screen sizes / input types.  It would be pretty
> cool to do some of this on a touch screen, but the controls may be too small
> to work with effectively, or too big so that they were covering up a lot of
> the screen.
> 
> I guess the alternative would be to draw the controls in browser chrome on
> top of the content.  There would still be highlighter bits for drawing the
> size, but the editing itself would happen in some kind of popup or overlay. 
> I'm not sure what is the right direction to go.  Any thoughts?

These are valid concerns.
One of the points we should keep in mind though is e10s: even if we restricted ourselves to firefox desktop, we'd still have to deal with its multi-process nature, and wouldn't be able to just overlay controls on top of the page by using the browser chrome.
Thought it would probably be simpler than inserting interactive in the canvas frame.

I think the geometry highlighter itself, without the drag/drop part, makes sense on all platforms though. Now I agree being able to move and resize an element on a phone screen might not be the main use case here, but that doesn't mean we shouldn't be using the canvas frame API.

As you said, the feature proposed in this bug is sufficiently simple that we can easily work around the lack of DOM access (I'm just making the element movable by tracking its coordinates and adding an event listener at global browser level), and so I haven't tried to look in more details at better solutions for now to allow more complex features to be implemented. But I'm pretty certain we can overcome the current limitation.
After all, native anonymous content is just DOM, and not having access to it from chrome JS doesn't mean we can't have access to it in any way.
For one, we could enrich the AnonymousContent API. And perhaps, we should look at how things like the <video> tag work:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp?force=1#109
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml?force=1

Comment 23

[Patrick Brosset <:pbro>]

Ehsan: Hey, you helped me add a way for chrome JS to inject content into a canvasFrame's native anonymous element (InsertAnonymousContent and AnonymousContent.webidl). Now, we're trying to find ways to have our chrome JS code interact with the inserted content. The current use case is inserting an element using this API and make this element movable (adding mousedown, mousemove, mouseup event listeners).
Since the API doesn't provide a reference to the elements, once inserted, the workaround we have is to add listeners at browser level and just using the event's mouse coordinates to know whether it happened on the inserted element.
Do you think there would be another solution to achieve this use case? Or an evolution of the API?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #22)
> I think the geometry highlighter itself, without the drag/drop part, makes
> sense on all platforms though. Now I agree being able to move and resize an
> element on a phone screen might not be the main use case here, but that
> doesn't mean we shouldn't be using the canvas frame API.

Agreed, the concerns are about the implementation of future editing tooling (and were provoked by the name of the highlighter and comment above it).  Don't think that should stop us from proceeding with the highlighter.

Comment 25

[Patrick Brosset <:pbro>]

Attachment: bug1123851-1-element-geometry-highlighter v2.patch

Rebased.

Comment 26

[Patrick Brosset <:pbro>]

Attachment: bug1123851-2-gcli-element-geometry-command v3.patch

Had to rebase this one too.

Comment 27

[Patrick Brosset <:pbro>]

Hi Olli, Ehsan being away at the moment, I was wondering if you could look into this question instead. Thanks!
> Ehsan: Hey, you helped me add a way for chrome JS to inject content into a
> canvasFrame's native anonymous element (InsertAnonymousContent and
> AnonymousContent.webidl). Now, we're trying to find ways to have our chrome
> JS code interact with the inserted content. The current use case is
> inserting an element using this API and make this element movable (adding
> mousedown, mousemove, mouseup event listeners).
> Since the API doesn't provide a reference to the elements, once inserted,
> the workaround we have is to add listeners at browser level and just using
> the event's mouse coordinates to know whether it happened on the inserted
> element.
> Do you think there would be another solution to achieve this use case? Or an
> evolution of the API?

Comment 28

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

I assume you add capturing event listener to get events before the web page, or
system group event listener to get the event after web page has accessed it
(stopPropagation in default group doesn't affect system group).

Don't you get access to the native anonymous elements using event.originalTarget?
That is a bit scary if one tries to make any changes to the native anonymous content, so please
use that only to detect that you're dealing with the elements you're interested in.
We could also add some [ChromeOnly] property to Event if that helps your case -
something which doesn't perhaps expose the actual original target object, but some id of it.

Comment 29

[Patrick Brosset <:pbro>]

(In reply to Olli Pettay [:smaug] (review overload. No more review requests before Feb 8, please) from comment #28)
> I assume you add capturing event listener to get events before the web page,
> or
> system group event listener to get the event after web page has accessed it
> (stopPropagation in default group doesn't affect system group).
Yeah we use capturing event listener at the browser level.
> Don't you get access to the native anonymous elements using
> event.originalTarget?
Right, that does work! (of course that's once I get rid of the pointer-event:none css rule I had added to this native anonymous element).
> That is a bit scary if one tries to make any changes to the native anonymous
> content, so please
> use that only to detect that you're dealing with the elements you're
> interested in.
I see, that indeed leaks the elements themselves, which we were trying to prevent in the first place with the insertion API.
> We could also add some [ChromeOnly] property to Event if that helps your
> case -
> something which doesn't perhaps expose the actual original target object,
> but some id of it.
Yeah that sounds like an option. We have a few features in mind for the devtools that will require event listeners for things we put in the native anonymous container, so I guess we'll need to experiment a bit with this first, and then see what works best.
Thanks a lot Olli!

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 8559766 [details] [diff] [review]
bug1123851-1-element-geometry-highlighter v2.patch

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

I've played around with the patch and ran into some things I'd like to see addressed before proceeding.  I've made a demo page so that you can test these things at https://bgrins.github.io/devtools-demos/inspector/positions.html.

* Drawing a line from the offsetParent seems wrong with relative positioning.  On the demo page: `highlight .relative --geometry`.  The arrow will draw from the body for instance, although it should be drawn from the element's original position.
* A margin causes the arrows to end earlier than seen by the highlighter.  On the demo page, run `highlight .fixed --geometry` and see the gap caused by the top margin.
* The top and left lines are drawn even on statically positioned elements if they have top/left defined in CSS (even though it doesn't do anything).  On the demo page: `highlight .static --geometry`
* Sticky positioning highlighting is pretty odd although it does show some interesting things (it's probably not used a ton and could be dropped for v1).  On the demo page: `highlight dt --geometry`.  Interestingly I think there may be a perf issue when scrolling with that command active, but not sure if it is specific to this highlighter.

Comment 31

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #30)
> Comment on attachment 8559766 [details] [diff] [review]
> bug1123851-1-element-geometry-highlighter v2.patch
> 
> Review of attachment 8559766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've played around with the patch and ran into some things I'd like to see
> addressed before proceeding.  I've made a demo page so that you can test
> these things at
> https://bgrins.github.io/devtools-demos/inspector/positions.html.
Thanks Brian, that helps a lot.
And sorry for not testing all these use cases more thoroughly.

There's a few of the problems that are actually caused by the way the gcli command uses the highlighter. Like for example, in my patch, it asks to hide the guides of the offsetParent by default. This is wrong. 

> * Drawing a line from the offsetParent seems wrong with relative
> positioning.  On the demo page: `highlight .relative --geometry`.  The arrow
> will draw from the body for instance, although it should be drawn from the
> element's original position.
You're right, I'll fix that.

> * A margin causes the arrows to end earlier than seen by the highlighter. 
> On the demo page, run `highlight .fixed --geometry` and see the gap caused
> by the top margin.
That's another case of the gcli command not configuring the highlighter correctly. The arrows end at the right place, if you have an element with a 10px left margin, absolutely positioned at 10px from the left, then the content box will be at 20px from the left, so the 10px arrow needs to stop at the margin box.
So what's wrong here is that the element highlighter should always cover the margin box.
In order to make the geometry highlighter fit within the `highlight` command, I made it accept the same parameters, but that makes it highlight the content box by default instead.
I'm starting to wonder if making another command instead wouldn't be a better idea. In fact, I made this command merely as a way to test the highlighter easier, but I'm not even convinced we need one.

> * The top and left lines are drawn even on statically positioned elements if
> they have top/left defined in CSS (even though it doesn't do anything).  On
> the demo page: `highlight .static --geometry`
Good catch, I thought I had added a check for this, but obviously not.

> * Sticky positioning highlighting is pretty odd although it does show some
> interesting things (it's probably not used a ton and could be dropped for
> v1).  On the demo page: `highlight dt --geometry`.  Interestingly I think
> there may be a perf issue when scrolling with that command active, but not
> sure if it is specific to this highlighter.
Yes this highlighter not great with performance, it access a lot of stuff from the CSSOM, DOM, computedstyles and does this repeatedly to move along when the highlighted element moves.
Having just one highlighter on is fine though, and that's what it was intended for (but of course the highlight command is useful for highlighting several elements at the same time).

Comment 32

[Patrick Brosset <:pbro>]

Attachment: bug1123851-1-element-geometry-highlighter v3.patch

This should take care of the remaining cases, and make the highlighter look a lot nicer too.
I'm going to upload a test-only patch just after, to make it a lot easier to review this new feature. It basically just replaces the boxmodelhighlighter with this new one, so that you can just open the inspector and hover over nodes to see it, instead of having to open a non-e10s window, then type a long gcli command.
This should help testing this.

Comment 33

[Patrick Brosset <:pbro>]

Attachment: bug1123851-for-testing-only.patch

This is the test-only patch I mentioned in the previous comment. Just apply it on top of part 1.

Comment 34

[Patrick Brosset <:pbro>]

Attachment: bug1123851-2-gcli-element-geometry-command v4.patch

And this is an update to the gcli patch, caused by the changes made to part 1.
I essentially removed all the configuration options I added to the geometry highlighter. I don't think they are needed for v1, and they were making the highlighter look weird/incomplete when using it from the command.
I'm not carrying R+ over on this one because I think we first need to figure out if adding this parameter to the gcli highlight command is the right thing to do.

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 8557926 [details] [diff] [review]
bug1123851-3-tests v1.patch

Clearing the review flag on the test patch for now, since I made quite many changes to part 1, I will have to revisit this patch anyway.

Comment 36

[Patrick Brosset <:pbro>]

(In reply to Olli Pettay [:smaug] from comment #28)
> I assume you add capturing event listener to get events before the web page,
> or
> system group event listener to get the event after web page has accessed it
> (stopPropagation in default group doesn't affect system group).
> 
> Don't you get access to the native anonymous elements using
> event.originalTarget?
> That is a bit scary if one tries to make any changes to the native anonymous
> content, so please
> use that only to detect that you're dealing with the elements you're
> interested in.
> We could also add some [ChromeOnly] property to Event if that helps your
> case -
> something which doesn't perhaps expose the actual original target object,
> but some id of it.

Hey Olli, I was experimenting with this a little bit just now, and was wondering about something you might be able to help with:

If we wanted to access the element inside the native anonymous container using event.originalTarget (notwithstanding the fact that this leaks the elements we were trying to hide), we'd have to remove the 'pointer-events:none;' css declaration added on the native anonymous container.
But, the problem I can see is that we use this for the devtools highlighter, and the way it works is that while you move your mouse over elements in the page, it highlights the element that it finds below the mouse. It does this with document.elementFromPoint.
This means we first need to know what 'document' is, and we can do this by accessing event.target, which in the absence of 'pointer-events:none;' is going to be the documentElement of the top window, not necessarily the one we're currently hovering over.
And then, elementFromPoint will return the closest non-anonymous ancestor, which in the case of the canvasframe anonymous elements will be the documentElement too.

So, I don't see a way for us to both get access to originalTarget (by removing pointer-events:none) and still be able to detect hovered elements in the content page.

Any ideas on how to achieve this?

Comment 37

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

pointer-events: none removes the element from hit testing, so event certainly can't be targeted to such
element.

Sounds like you might want to add a version of elementFromPoint which doesn't 
remove native anon content here.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=7a7e30dbc96c&mark=3505-3505#3469
(I don't know if nsLayoutUtils::GetFrameForPoint in that method bypasses pointer-event)

Comment 38

[Patrick Brosset <:pbro>]

Attachment: bug1123851-4-anon-content-event-handling.patch

Here's an attempt at adding event handling support for elements inserted into the canvasFrame native anonymous content.

Here's what the changes look like:

- I removed the css declarations that were making the whole container be 100%/100%. This prevents the native anonymous container from covering up the whole viewport at all times.

- I removed the pointer-events:none declaration too, so that it's possible to add event listeners that will detect the highlighter nodes using originalTarget.

- Since the container doesn't have a size anymore, I made the highlighters (usually svg nodes) calculate their sizes in pixels, based on the viewport size.

- I added pointer-events:none only to the box-model-highlighter, because that's the one used for picking elements in the page, and so it needs to let mousemoves and such go through to the page.

- Finally, I added 2 methods to the CanvasFrameAnonymousContentHelper class for adding and removing event listeners. Instead of adding the listeners to the inserted elements themselves (because we don't have access to them), it adds global listeners at browser level (like the picker does) and then uses the originalTarget to compare to the provided element ID.

I'd like Matteo and Brian's input on this before going forward with the code/test/reviews.

Comment 39

[Patrick Brosset <:pbro>]

Attachment: bug1123851-5-make-positioned-element-movable.patch

Using the event handler system in part 4, I was able to add drag/drop of positioned elements quite easily.
See it in action here: https://www.youtube.com/watch?v=zsNhGllr2-g

Comment 40

[Brian Grinstead [:bgrins]]

Comment on attachment 8562034 [details] [diff] [review]
bug1123851-1-element-geometry-highlighter v3.patch

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

Looking good, ready for tests.  A couple more issues I noticed when testing:

* I don't see the '40px' on the top line for the top-right fixed element at https://bgrins.github.io/devtools-demos/inspector/positions.html when I'm scrolled.
* The '#relative-inline' element has problems at https://bgrins.github.io/devtools-demos/inspector/positions.html (it seems like it is using the first clientRect only for highlighting the offsetParent and the element itself)
* You can have overlapping arrows, like on the #absolute-width-margin on the demo page.  I'm not really sure the best way to deal with this situation - the overlapping arrows probably aren't the biggest deal, but notice that the width arrow covers up the handle for the top property.  Maybe the width/height arrows should be off center?

::: toolkit/devtools/server/actors/highlighter.js
@@ +45,5 @@
>                               "  outline-offset: -2px!important;" +
>                               "}";
> +// Distance of the width or height handles from the node's edge.
> +const GEOMETRY_SIZE_ARROW_OFFSET = .25; // 25%
> +// How much are labels offset with respect to the corresponding arrows.

Looks like a hanging comment here - not sure what it is referring to

@@ +2026,5 @@
> +        if (value && value !== "auto") {
> +          // getCSSStyleRules returns rules ordered from least-specific to
> +          // most-specific, so just override any previous properties we have set.
> +          props.set(name, {
> +            cssRule: rule

The only way that any of these values are accessed later on (through sideProp and sizeProp) is:

prop.cssRule.style.getPropertyValue(name)

Which is doing duplicate work that you are already doing here.  You may as well store this value directly and it will simplify things quite a bit:

props.set(name, value)

@@ +2067,5 @@
> +    setIgnoreLayoutChanges(true);
> +
> +    // At each update, the position or/and size may have changed, so get the
> +    // list of defined properties, and re-position the arrows and highlighters.
> +    this.definedProperties.clear();

No need to clear() here - you are assigning it on the next line

@@ +2069,5 @@
> +    // At each update, the position or/and size may have changed, so get the
> +    // list of defined properties, and re-position the arrows and highlighters.
> +    this.definedProperties.clear();
> +    this.definedProperties = this.getDefinedGeometryProperties();
> +    if (!this.definedProperties.size) {

setIgnoreLayoutChanges is never reset in this early return case.  Could it wait to be set until after this condition?

@@ +2075,5 @@
> +      return false;
> +    }
> +
> +    // Get the offsetParent, if any.
> +    this.offsetParent = getOffsetParent(this.currentNode);

this.offsetParent and this.parentQuads could be set in the updateOffsetParent - your choice

Comment 41

[Brian Grinstead [:bgrins]]

The basics here are getting close, but I was thinking more about how exactly this will work in the UI.  We should be thinking about this and get UX help if needed, since it could affect implementation.

For the non-editable version: Is it on whenever you hover a top/left/bottom/right/width/height/position property?  Or could/should we somehow extend the box model highlighter to overlay this information at all times?

For the editable version: How do we 'lock' the highlighter so that it doesn't disappear?

Comment 42

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #41)
> The basics here are getting close, but I was thinking more about how exactly
> this will work in the UI.  We should be thinking about this and get UX help
> if needed, since it could affect implementation.
Thanks for the review Brian, I'll start correcting the patch in a bit.
As for the UX, I'm not sure yet to be honest, getting some UX help would be great in fact.

> For the non-editable version: Is it on whenever you hover a
> top/left/bottom/right/width/height/position property?  Or could/should we
> somehow extend the box model highlighter to overlay this information at all
> times?
I'm not a big fan of the show-on-hover thing (in fact, I'm in the process of prototyping something that removes it for selector highlight in bug 1106272). Displaying this new highlighter anytime you hover a top/left/... property could be frustrating. I'm not a big fan either of adding this to the standard BoxModelHighlighter, I think it needs to stay simple.
The solution I have in mind so far is having a button in the UI somewhere that toggles the new highlighter on the current node.
So, this could be in several places: in the layout-view, or also, as a small icon (like the color watches) next to positioning properties in the rule-view.

> For the editable version: How do we 'lock' the highlighter so that it
> doesn't disappear?
I don't know if we should have these 2 non-editable and editable modes. The non-editable part was merely done to keep the patch small and to give me some time to work on a decent event listener strategy for highlighters.
Anyway, if we go for the solution I proposed just before, locking the highlighter wouldn't be an issue because it would stay as long as you wouldn't click the button again.

How does that sound?
I'll NI? someone from UX.

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8563305 [details] [diff] [review]
bug1123851-4-anon-content-event-handling.patch

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

Seems fine to me

::: toolkit/devtools/server/actors/highlighter.css
@@ +25,5 @@
>  
> +/* The SVG container for this highlighter covers the viewport, so it needs to
> +   not respond to pointer-events, to avoid blocking the node hover mechanism */
> +:-moz-native-anonymous .box-model-elements {
> +  pointer-events: none;

Maybe just to be safe we should make all highlighters `pointer-events: none` by default and then we can selectively allow pointer events on ones that we do want to be able to interact with.

::: toolkit/devtools/server/actors/highlighter.js
@@ +557,5 @@
> +   * Unfortunately, the inserted nodes are still available via
> +   * event.originalTarget, and that's what the event handler here uses to check
> +   * that the event actually occured on the right element, but that also means
> +   * consumers of this code would be able to access the inserted elements.
> +   * MAKE SURE YOU DON'T DIRECTLY MODIFY ELEMENTS USING THE ORIGINALTARGET

Could we clone `event` and null out originalTarget before passing it along to the handler so we don't need to worry about consumers accessing it?

@@ +568,5 @@
> +  addEventListenerForElement: function(id, type, handler) {
> +    // If noone is listening for this type of event yet, add one listener.
> +    if (!this.listeners.has(type)) {
> +      let target = getPageListenerTarget(this.tabActor);
> +      target.addEventListener(type, this, true);

Do we need to removeEventListener for the target when all listeners are removed and/or the tabActor is being destroyed to clean up after ourselves?

This code in particular looks like it would add multiple listeners when calling 

addEventListener("click", foo);    // no listeners, add to target
removeEventListener("click", foo); // remove listener (don't do anything to target)
addEventListener("click", foo);    // no listeners, add to target

Comment 44

[Patrick Brosset <:pbro>]

Attachment: Adobe-edge-reflow-resize-move-tool.png

Stephen, what do you think the best user experience would be for this new inspector feature?
Basically, what this adds is a new "highlighter", or rather an "overlay". Something you can display on top of the currently selected node, in the page, and that shows all positioning/sizing properties (top, left, bottom, right, width, height), if they are set, and that allows the user to drag/drop/resize the element directly in the page.

We currently have nothing similar in the devtools. We do have a highlighter, but it's only displayed on hover of nodes in the inspector, so it never stays for long, and you can't interact with it.
This one is more like the Transform tool you have in Photoshop for example, it's something you need to enable, then use, and then disable.
Therefore, we don't really have a good way right now to do the enabling/desabling part.

I was suggesting having an icon next to positioning properties in the rule-view, but it's not great either, because you could potentially have it several times, or not at all, depending on what's in the rule-view, so not very predictable.

To be honest, I want to have a toolbar for this. We may have other such tools in the future, and a photoshop-like toolbar would work nicely for this (see this proposal: https://gist.github.com/captainbrosset/6535c92458a80d687487).

Fwiw, Adobe Edge Reflow is a nice tool that allows to create responsive web pages, and they have a very very similar tool. I've uploaded a screenshot.

Comment 45

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #40)
> * I don't see the '40px' on the top line for the top-right fixed element at
> https://bgrins.github.io/devtools-demos/inspector/positions.html when I'm
> scrolled.
That's right, I'll get this fixed.
> * The '#relative-inline' element has problems at
> https://bgrins.github.io/devtools-demos/inspector/positions.html (it seems
> like it is using the first clientRect only for highlighting the offsetParent
> and the element itself)
Yeah, I think I'd like bug 1023232 to be fixed first. This is a problem we have even with the normal box-model highlighter, we only really care about the first box.
> * You can have overlapping arrows, like on the #absolute-width-margin on the
> demo page.  I'm not really sure the best way to deal with this situation -
> the overlapping arrows probably aren't the biggest deal, but notice that the
> width arrow covers up the handle for the top property.  Maybe the
> width/height arrows should be off center?
What about I get rid of the width and height arrows, and instead just keep a label inside the box that gives the width and height values (when set)? I could then have a small handle at the bottom right corner of the box to allow resizing.
The top,bottom,left,right arrows are nice to keep I think, because it visually explains the relation with the offset parent, and they can't really overlap eachother, so we're fine here. I could however get rid of the 4 handles on these arrows, because I'm going to allow drag and drop of the box directly, so they really be a need for moving the box by grabbing one of the handles.

> @@ +2026,5 @@
> > +        if (value && value !== "auto") {
> > +          // getCSSStyleRules returns rules ordered from least-specific to
> > +          // most-specific, so just override any previous properties we have set.
> > +          props.set(name, {
> > +            cssRule: rule
> 
> The only way that any of these values are accessed later on (through
> sideProp and sizeProp) is:
> 
> prop.cssRule.style.getPropertyValue(name)
> 
> Which is doing duplicate work that you are already doing here.  You may as
> well store this value directly and it will simplify things quite a bit:
> 
> props.set(name, value)
Hmm, yeah but one of the next patches is to allow moving/resizing and for this to work, I need to store the rule here, so that I can later modify it. If I only stored the value here, I'd have to retrieve the CSS rule when the box is moved/resized to update it.

I've addressed all the other review comments.
I'll upload a new patch later today.

Comment 46

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #45)
> What about I get rid of the width and height arrows, and instead just keep a
> label inside the box that gives the width and height values (when set)? I
> could then have a small handle at the bottom right corner of the box to
> allow resizing.
This would have another consequence: we wouldn't need to have both the inner box (for width/height) and the outer box (for position) anymore. Just one box would be enough.

Comment 47

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #46)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #45)
> > What about I get rid of the width and height arrows, and instead just keep a
> > label inside the box that gives the width and height values (when set)? I
> > could then have a small handle at the bottom right corner of the box to
> > allow resizing.
> This would have another consequence: we wouldn't need to have both the inner
> box (for width/height) and the outer box (for position) anymore. Just one
> box would be enough.

I thought it was sort of interesting for relatively positioned items to see where they were taking up space in the DOM.  But I don't have any objections to getting rid of the arrows and instead just printing the width / height elsewhere.

Comment 48

[Patrick Brosset <:pbro>]

Attachment: bug1123851-1-element-geometry-highlighter v4.patch

This new part 1 should hopefully take care of the review comments.
The highlighter is simpler now and the labels try their best to remain visible in the page.

Comment 49

[Patrick Brosset <:pbro>]

Attachment: bug1123851-2-gcli-element-geometry-command v5.patch

Quick fix to the gcli patch after some of the changes made to the highlighter.

Comment 50

[Patrick Brosset <:pbro>]

Attachment: bug1123851-3-tests v2.patch

I updated the test patch, correcting failing tests after the changes I made to the highlighter, and I also added some new test cases and a new test.

Comment 51

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #43)
> Comment on attachment 8563305 [details] [diff] [review]
> bug1123851-4-anon-content-event-handling.patch
> 
> Review of attachment 8563305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems fine to me
> 
> ::: toolkit/devtools/server/actors/highlighter.css
> @@ +25,5 @@
> >  
> > +/* The SVG container for this highlighter covers the viewport, so it needs to
> > +   not respond to pointer-events, to avoid blocking the node hover mechanism */
> > +:-moz-native-anonymous .box-model-elements {
> > +  pointer-events: none;
> 
> Maybe just to be safe we should make all highlighters `pointer-events: none`
> by default and then we can selectively allow pointer events on ones that we
> do want to be able to interact with.
Good idea.

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +557,5 @@
> > +   * Unfortunately, the inserted nodes are still available via
> > +   * event.originalTarget, and that's what the event handler here uses to check
> > +   * that the event actually occured on the right element, but that also means
> > +   * consumers of this code would be able to access the inserted elements.
> > +   * MAKE SURE YOU DON'T DIRECTLY MODIFY ELEMENTS USING THE ORIGINALTARGET
> 
> Could we clone `event` and null out originalTarget before passing it along
> to the handler so we don't need to worry about consumers accessing it?
Not entirely sure how to go about this.
Do you think this would be ok?
  Object.defineProperty(event, "originalTarget", {
     get: () => null,
     configurable: false
   });

> @@ +568,5 @@
> > +  addEventListenerForElement: function(id, type, handler) {
> > +    // If noone is listening for this type of event yet, add one listener.
> > +    if (!this.listeners.has(type)) {
> > +      let target = getPageListenerTarget(this.tabActor);
> > +      target.addEventListener(type, this, true);
> 
> Do we need to removeEventListener for the target when all listeners are
> removed and/or the tabActor is being destroyed to clean up after ourselves?
> 
> This code in particular looks like it would add multiple listeners when
> calling 
> 
> addEventListener("click", foo);    // no listeners, add to target
> removeEventListener("click", foo); // remove listener (don't do anything to
> target)
> addEventListener("click", foo);    // no listeners, add to target
Right, looks like I forgot to clean after myself.

I've got a new patch ready I'll upload in a bit.

Comment 52

[Patrick Brosset <:pbro>]

Attachment: bug1123851-4-anon-content-event-handling v2.patch

This should take care of the review comments.
Brian, I've been asking a lot of reviews recently, feel free to redirect to someone else if you're busy at the moment. Especially that I've got 2 other patches coming...

Comment 53

[Patrick Brosset <:pbro>]

Attachment: bug1123851-5-make-positioned-element-movable v2.patch

Based on part 4, this patch makes positioned elements movable simply with drag/drop.
I've added tests.

Comment 54

[Patrick Brosset <:pbro>]

Attachment: bug1123851-6-make-elements-resizable v1.patch

Same as part 5, except this one takes care of the resizing part.
It adds a little handle at the bottom right corner of resizable elements that you can move around to resize the element.

Comment 55

[Brian Grinstead [:bgrins]]

Comment on attachment 8567968 [details] [diff] [review]
bug1123851-4-anon-content-event-handling v2.patch

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

It seems like we should have a test here directly for addEventListenerForElement/removeEventListenerForElement as opposed to relying on the tests for the drag/drop functionality to imply this works.  Since it would be a more direct and simple test, we could be more thorough with the cases for removing all listeners, page navigation, etc.  Thoughts?

::: toolkit/devtools/gcli/commands/highlight.js
@@ +41,4 @@
>    return {
> +    browser: browser,
> +    window: env.window,
> +    chromeEventHandler: browser.docShell.chromeEventHandler

I don't see this property used in this patch or any of the subsequent ones.  Is this change necessary?

::: toolkit/devtools/server/actors/highlighter.js
@@ +608,5 @@
> +    for (let [id, handler] of listeners) {
> +      if (event.originalTarget.id === id) {
> +        // Hiding the originalTarget property to avoid exposing references to
> +        // native anonymous elements. See addEventListenerForElement's comment.
> +        Object.defineProperty(event, "originalTarget", {

Could you just do event.originalTarget = null?

But either way, won't this be a problem when there is more than one listener and the first one matches.  In that case we set this to null and can't check the id property on the next iteration.

May be simpler to do:

let targetID = event.originalTarget.id;
event.originalTarget = null; // (or however is best to clear this out)
	
for (let [id, handler] of listeners) {
  if (targetID === id) {
    ....
  }
}

Comment 56

[Brian Grinstead [:bgrins]]

Comment on attachment 8566521 [details] [diff] [review]
bug1123851-1-element-geometry-highlighter v4.patch

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

At first I thought it didn't seem to react well to resizing (try opening the toolbox, then highlight --geometry .page-wrap on http://css-tricks.com/, then shrink the toolbox - I saw that the highlight was missing a chunk).  After applying only the first patches I realize that this might have been an issue introduced by one of the later ones, but leaving a comment here anyway so I don't forget, and in case you can reproduce.

::: toolkit/devtools/server/actors/highlighter.js
@@ +2108,5 @@
> +
> +  /**
> +   * Update the offset parent highlighter.
> +   * There are 3 different cases covered here:
> +   * - the node is absolutely/fixed positioned, and an offsetParent is defined

So we aren't going to show absolute/fixed with no offsetParent?  That's fine if intended, just double checking.  When I highlight the .absolute element on https://bgrins.github.io/devtools-demos/inspector/positions.html I don't see anything show up

@@ +2140,5 @@
> +      isHighlighted = true;
> +    } else if (isRelative) {
> +      let xDelta = parseFloat(this.computedStyle.left);
> +      let yDelta = parseFloat(this.computedStyle.top);
> +      if (xDelta || yDelta) {

Is the reason you don't check for 'bottom' / 'right' here that they get returned negative top/left in computed style?

I wasn't sure why it was working when testing this, but double checked computedStyle and realized that this was the case.

@@ +2258,5 @@
> +      }
> +
> +      // In case of relative positioning.
> +      if (this.computedStyle.position === "relative") {
> +        if (GeoProp.isInverted(side)) {

There are definitely some issues with bottom and right properties on relative elements.  Check out the 'relative-bottom-right' and 'relative-all' elements at: https://bgrins.github.io/devtools-demos/inspector/positions.html.

A simple case that appears to be incorrect is if you set `position: relative; top: 0; bottom: -50px`

@@ +2303,5 @@
> +    arrowEl.setAttribute(GeoProp.axis(side) + "2", mainEnd);
> +    arrowEl.setAttribute(GeoProp.crossAxis(side) + "2", crossPos);
> +    arrowEl.removeAttribute("hidden");
> +
> +    // Position the label <text> in the middle of the arrow (making sure it's

A case where the label gets covered up is on the relative-bottom-right element at https://bgrins.github.io/devtools-demos/inspector/positions.html.

A case where a label ends up below the fold: highlight --geometry .page-wrap on http://css-tricks.com.  I'm not sure if there is a good way to handle this, expect maybe ensuring the label is always at least visible on the page.

Comment 57

[Brian Grinstead [:bgrins]]

Comment on attachment 8566998 [details] [diff] [review]
bug1123851-3-tests v2.patch

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

The tests look fine.  I think there will need to be some more added based on Comments on 56 though, so please re-request review after that.

::: browser/devtools/commandline/test/browser_cmd_highlight_01.js
@@ +243,5 @@
>          output: '200 nodes highlighted'
>        }
>      },
>      {
> +      setup: 'highlight div --geometry',

The changes to commandline/test/* should be folded into the gcli command patch instead

::: browser/devtools/inspector/test/browser_inspector_highlighter-geometry_02.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// Test that the geometry highlighter labels content is correct.

'Test that the geometry highlighter labels are correct' or 'Test that the geometry highlighter label content is correct.'

@@ +30,5 @@
> +  yield highlighterFront.show(node);
> +
> +  let label = yield getHighlighterNodeTextContent(highlighterFront,
> +    ID + "label-left");
> +  is(label, "5rem", "Left label textcontent is correct");

Maybe also check that each label is visible.  Also, it'd be nice to remove one of the positions on the element and make sure that the corresponding label is hidden/has empty content (or just have another element that doesn't have one or more properties set and run a test with that one)

@@ +56,5 @@
> +  yield highlighterFront.show(node);
> +
> +  let label = yield getHighlighterNodeTextContent(highlighterFront,
> +    ID + "label-size");
> +  ok(label.contains("5em") && label.contains("50%"),

You could just copy in the characters that are being printed into the label and do a direct string comparison:

↔ 5em ↕ 50%

It'd also be nice here if you could remove width and/or height and make sure the label updates as expected

Comment 58

[Patrick Brosset <:pbro>]

Attachment: bug1123851-1-element-geometry-highlighter v5.patch

Thanks for the review. Here's a patch that should address the comments.

(In reply to Brian Grinstead [:bgrins] from comment #56)
> At first I thought it didn't seem to react well to resizing (try opening the
> toolbox, then highlight --geometry .page-wrap on http://css-tricks.com/,
> then shrink the toolbox - I saw that the highlight was missing a chunk). 
> After applying only the first patches I realize that this might have been an
> issue introduced by one of the later ones, but leaving a comment here anyway
> so I don't forget, and in case you can reproduce.
This is an issue introduced by part 4 where I compute the size of the container SVG element dynamically based on the window inner size, but I don't update it on window resize. I'll fix this in part 4.

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +2108,5 @@
> > +
> > +  /**
> > +   * Update the offset parent highlighter.
> > +   * There are 3 different cases covered here:
> > +   * - the node is absolutely/fixed positioned, and an offsetParent is defined
> 
> So we aren't going to show absolute/fixed with no offsetParent?  That's fine
> if intended, just double checking.
No we *are* highlighting positioned elements, even when they have no offset parents.
This comment is perhaps misleading. The word "highlighter" in this comment only applies to the offsetparent highlighter. I've fixed this in this new patch.
> When I highlight the .absolute element
> on https://bgrins.github.io/devtools-demos/inspector/positions.html I don't
> see anything show up
Really? That's weird, it does work for me, both with only this 1 patch applied, or all of them.

> @@ +2140,5 @@
> > +      isHighlighted = true;
> > +    } else if (isRelative) {
> > +      let xDelta = parseFloat(this.computedStyle.left);
> > +      let yDelta = parseFloat(this.computedStyle.top);
> > +      if (xDelta || yDelta) {
> 
> Is the reason you don't check for 'bottom' / 'right' here that they get
> returned negative top/left in computed style?
> 
> I wasn't sure why it was working when testing this, but double checked
> computedStyle and realized that this was the case.
Yeah exactly, I didn't know this before, but setting bottom/right on a relative positioned element causes the top/left computed style to be set the corresponding negative values.

> @@ +2258,5 @@
> > +      }
> > +
> > +      // In case of relative positioning.
> > +      if (this.computedStyle.position === "relative") {
> > +        if (GeoProp.isInverted(side)) {
> 
> There are definitely some issues with bottom and right properties on
> relative elements.  Check out the 'relative-bottom-right'
I'm not seeing anything really wrong with relative-bottom-right. It doesn't look great, but I think it's correct. It shows both the bottom and right arrows and labels. I could try and find another way to position the arrows, but assuming we wanted to keep the way arrows are placed now, this looks correct to me (let's file a follow-up bug for working on arrow placement if needed).
> and 'relative-all'
> elements at:
> https://bgrins.github.io/devtools-demos/inspector/positions.html.
This one is wrong indeed. With relative positioning, when you specify top, left, bottom and right, only top and left are really taken into account, and so the bottom and right arrows should not be displayed. I've fixed this.

> A simple case that appears to be incorrect is if you set `position:
> relative; top: 0; bottom: -50px`
Yep, this one is the same as the previous case, specifying both top and bottom results in only the top property being applied, so the highlighter shouldn't show the bottom arrow at all.

> @@ +2303,5 @@
> > +    arrowEl.setAttribute(GeoProp.axis(side) + "2", mainEnd);
> > +    arrowEl.setAttribute(GeoProp.crossAxis(side) + "2", crossPos);
> > +    arrowEl.removeAttribute("hidden");
> > +
> > +    // Position the label <text> in the middle of the arrow (making sure it's
> 
> A case where the label gets covered up is on the relative-bottom-right
> element at https://bgrins.github.io/devtools-demos/inspector/positions.html.
> 
> A case where a label ends up below the fold: highlight --geometry .page-wrap
> on http://css-tricks.com.  I'm not sure if there is a good way to handle
> this, expect maybe ensuring the label is always at least visible on the page.
Is it ok if I keep this for a follow-up bug? It's sufficiently decoupled I think that we can revisit this later. And I'd rather try and start landing stuff here.

Placing labels is really tricky here. They need to all be visible at the same time, and that's hard with negative dimensions sometimes, and also when elements are small.
One thing I did in this patch is completely hide the label when the corresponding arrow isn't visible in the viewport anymore. I'd like to keep it this way. I'm happy to be convinced otherwise, but I think keeping all the labels all the time even when "detached" from the corresponding arrows would look a but weird.

Comment 59

[Patrick Brosset <:pbro>]

Attachment: bug1123851-2-gcli-element-geometry-command v6.patch

Folded in the gcli tests that were previously part of patch 3.

Comment 60

[Patrick Brosset <:pbro>]

Attachment: bug1123851-3-tests v3.patch

Removed the gcli tests from this patch, and added more test cases for checking labels visibility and textContent.

Comment 61

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #55)
> Comment on attachment 8567968 [details] [diff] [review]
> bug1123851-4-anon-content-event-handling v2.patch
> 
> Review of attachment 8567968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems like we should have a test here directly for
> addEventListenerForElement/removeEventListenerForElement as opposed to
> relying on the tests for the drag/drop functionality to imply this works. 
> Since it would be a more direct and simple test, we could be more thorough
> with the cases for removing all listeners, page navigation, etc.  Thoughts?
You're right, I'm now writing a series of tests specifically for the CanvasFrameAnonymousContentHelper class.

> ::: toolkit/devtools/gcli/commands/highlight.js
> @@ +41,4 @@
> >    return {
> > +    browser: browser,
> > +    window: env.window,
> > +    chromeEventHandler: browser.docShell.chromeEventHandler
> 
> I don't see this property used in this patch or any of the subsequent ones. 
> Is this change necessary?
getPageListenerTarget in highlighter.js needs this property to exist on the tabActor, and because the gcli command somehow fakes a tabActor, it needs to define this property too. This was not a problem before this patch, because before that, getPageListenerTarget wasn't being called.

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +608,5 @@
> > +    for (let [id, handler] of listeners) {
> > +      if (event.originalTarget.id === id) {
> > +        // Hiding the originalTarget property to avoid exposing references to
> > +        // native anonymous elements. See addEventListenerForElement's comment.
> > +        Object.defineProperty(event, "originalTarget", {
> 
> Could you just do event.originalTarget = null?
> 
> But either way, won't this be a problem when there is more than one listener
> and the first one matches.  In that case we set this to null and can't check
> the id property on the next iteration.
> 
> May be simpler to do:
> 
> let targetID = event.originalTarget.id;
> event.originalTarget = null; // (or however is best to clear this out)
> 	
> for (let [id, handler] of listeners) {
>   if (targetID === id) {
>     ....
>   }
> }
I will fix this.

I'm also working on a patch that corrects the problem where the highlighter area is cut off to the viewport size and doesn't adapt when the window is resized.

Comment 62

[Patrick Brosset <:pbro>]

Filing follow up bug 1136594 to have a similar tool for editing padding, border and margin sizes. This should probably be handled by the same highlighter class, but maybe switching between the moving/resizing and box-model resizing modes, to avoid having too many handles and labels visible at the same time.

Comment 63

[Patrick Brosset <:pbro>]

Attachment: bug1123851-4-anon-content-event-handling v3.patch

I have added a few tests in this patch, specifically for testing the CanvasFrameAnonymousContentHelper class itself.

One thing I didn't manage to test was ensuring that event.originalTarget was null: in order to make it null, I had to use Object.defineProperty (assigning the property to null directly wouldn't work because it doesn't have a setter) to modify the property's behavior, but somehow, only in the scope of the test function, the property is back to normal.
For instance, if I log the value of e.originalTarget just before calling the handler (in highlighter.js), it correctly logs null, if I log it after calling the handler (still in highlighter.js), it's null too, but if I log it inside the handler, in the test file, then it's not null anymore.
This only happens in the test, and I have no idea why ...

I've also added some remarks in comment 61.

Comment 64

[Patrick Brosset <:pbro>]

Attachment: bug1123851-5-make-positioned-element-movable v3.patch

Rebased part 5

Comment 65

[Patrick Brosset <:pbro>]

Attachment: bug1123851-6-make-elements-resizable v2.patch

Rebased part 6.

Comment 66

[Patrick Brosset <:pbro>]

Brian and I had a discussion about UX/UI yesterday that led me to summarize my thoughts here: https://gist.github.com/captainbrosset/8a337290ba99b01f6425
Stephen: this document will help understand what are the main challenges to be solved in this bug.

Comment 67

[Patrick Brosset <:pbro>]

Oops, I messed with the needinfo flag somehow.

Comment 68

[Tim Nguyen :ntim]

I don't know if this has been done yet, but it'd be nice if holding the Ctrl/Cmd key would allow resizing/moving pixel by pixel (kinda like the responsive mode).

Comment 69

[Patrick Brosset <:pbro>]

Filed follow-up bug 1138330 for making label positioning better.

Comment 70

[Patrick Brosset <:pbro>]

Attachment: bug1123851-1-element-geometry-highlighter v6.patch

Fixed a typo ("bottom" instead of "top") in the code that prevented the bottom arrow in certain cases from being displayed.

Comment 71

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #68)
> I don't know if this has been done yet, but it'd be nice if holding the
> Ctrl/Cmd key would allow resizing/moving pixel by pixel (kinda like the
> responsive mode).
Good idea. No that hasn't been done. Good candidate for a follow-up.

Comment 72

[Brian Grinstead [:bgrins]]

Comment on attachment 8571250 [details] [diff] [review]
bug1123851-1-element-geometry-highlighter v6.patch

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

OK, let's organize things a bit here.  The main highlighter patch and test changes look good, I'd suggest we land those two patches in this bug and then file one follow up for part 4 and one follow up for parts 5 and 6.  This will allow us to unblock other work and focus the editing / ux discussions in another bug.

Comment 73

[Brian Grinstead [:bgrins]]

Comment on attachment 8569196 [details] [diff] [review]
bug1123851-4-anon-content-event-handling v3.patch

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

This looks pretty good, although there is a problem with all the patches applied having to do with originalTarget getter returning null (see comment below).

Also, it'd be easier to read the diff and eventual blame if the changes to highlighter.js were split into two patches - the first just doing the getElement refactor and the second making the relevant changes for event listeners.  Finally, can we move this patch into Bug 113918?

::: toolkit/devtools/server/actors/highlighter.js
@@ +610,5 @@
> +    if (!listeners) {
> +      return;
> +    }
> +
> +    let targetID = event.originalTarget.id;

I'm seeing errors about event.originalTarget is null with all the patches applied and running this code.  Instead of setting the getter on original target, how about this?

    // Hiding the originalTarget property to avoid exposing references to
    // native anonymous elements. See addEventListenerForElement's comment.
    if (event.originalTarget) {
      event._originalTarget = event.originalTarget;
      delete event.originalTarget;
    }

    let targetID = event._originalTarget.id;

@@ +978,5 @@
>        parent: rootWrapper,
>        attributes: {
>          "id": "elements",
> +        "class": "elements",
> +        "width": this.win.innerWidth,

Is there a benefit to doing the sizing here or can we move the sizing back to CSS for this since we made the pointer-events change opt-in?  Seems like this change plus the one below dealing with zoom is asking for some extra work for all custom highlighters.

@@ +1151,5 @@
>     */
>    _update: function() {
>      setIgnoreLayoutChanges(true);
>  
> +    let svg = this.getElement("elements");

Same comment as above about the sizing possibly moving to css

::: toolkit/devtools/server/tests/browser/browser_canvasframe_helper_03.js
@@ +36,5 @@
> +  function onMouseDown(e, id) {
> +    is(id, "child-element", "The mousedown event was triggered on the element");
> +    mouseDownHandled ++;
> +  }
> +  el.addEventListener("mousedown", onMouseDown);

Can you also add the same event listener on a parent element within the same helper to make sure it fires both times?  It would also be nice to check with a second CanvasFrameAnonymousContentHelper positioned in the same place to make sure they don't interfere with each other (I'm not sure what the intended behavior is here)

Comment 74

[Brian Grinstead [:bgrins]]

Comment on attachment 8569220 [details] [diff] [review]
bug1123851-5-make-positioned-element-movable v3.patch

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

We discussed a variety of cases that we still need to handle with the UX here.  I'd like to move that discussion (and further reviews) to Bug 1139187

Comment 75

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf317560fb9

Comment 76

[Patrick Brosset <:pbro>]

Pushed to fx-team:
remote:   https://hg.mozilla.org/integration/fx-team/rev/173ddf175fb8
remote:   https://hg.mozilla.org/integration/fx-team/rev/b2a7cf2a5aa1

(clearing UX flag too, will ping again in the follow-up bugs).

Comment 77

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/173ddf175fb8
https://hg.mozilla.org/mozilla-central/rev/b2a7cf2a5aa1