Allow drag and drop reordering of elements in markup view

RESOLVED FIXED in Firefox 39

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: msucan, Assigned: mahdi, Mentored)

Tracking

({dev-doc-complete})

Trunk
Firefox 39
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed, relnote-firefox 39+)

Details

Attachments

(1 attachment, 40 obsolete attachments)

37.85 KB, patch
mahdi
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
I was just asked by a web designer who needed to reorder DOM elements in his page if he can simply drag and drop them in the markup view. He wanted to reorder table columns and rows.

I believe it makes sense to allow tree reorder via drag and drop. Thoughts?
Agreed, it doesn't affect the UI in ways other than that which people would expect, and it doesn't feel like it's going to involve lots of extra code.
Duplicate of this bug: 891423
Priority: -- → P3

Comment 3

5 years ago
Please allow selection of multiple nodes. I would like to hold down shift key and select multiple nodes and then drag them into their new location.
Duplicate of this bug: 785148
Priority: P3 → P2

Comment 5

5 years ago
Coming from Chrome devtools I was surprised this wasn't already in Firefox. Would suggest to start with being able to just move one node first as in chrome and then move to supporting multiple with shift and ctrl.

Comment 6

5 years ago
I guess this could be a [good next bug] for new contributors. What do you think ?
Flags: needinfo?(pbrosset)
(In reply to msnazi from comment #3)
> Please allow selection of multiple nodes. I would like to hold down shift
> key and select multiple nodes and then drag them into their new location.

The multiple node is a bit tricky because right now everything in the inspector is based on a single node being selected at any given point in time. If we want to allow multi-selection, we first need to decide what the inspector should display (mostly in the sidebar) when more than one node is selected.

(In reply to Martin Lundberg from comment #5)
> Coming from Chrome devtools I was surprised this wasn't already in Firefox.
> Would suggest to start with being able to just move one node first as in
> chrome and then move to supporting multiple with shift and ctrl.

I agree, let's use this bug to move one node, we can file a follow-up to deal with multi-selection when we've answered the first question.
Flags: needinfo?(pbrosset)
(In reply to Tim Nguyen [:ntim] from comment #6)
> I guess this could be a [good next bug] for new contributors. What do you
> think ?

Yes this can be a good bug for contributors to work on. I can guide whoever is interested to solve this bug.

There are several parts to the bug. First we need to support drag/drop in the markup-view. This means:
- being able to move a dragged representation of the node anywhere in the markup-view,
- providing a visual clue of where the node would be inserted if it was dropped

Once dropped, I believe we have the right methods on the server side to re-order the markup. Namely, |walkerActor.insertBefore| can be used to insert the node at the right position.
Once done, there's no need to refresh the markup-view, this will be done automatically by mutation events.
Mentor: pbrosset
Hi, I'm interested in this bug, I've taken first steps into this by making elements movable, making a placeholder, getting familiar, etc.

I guess I have to take a look at the methods |walkerActor| provides. I tried finding it's source but had no luck, can you guide me?

Thanks!
Assignee: nobody → mdibaiee
Will let Patrick do the guiding :)
Status: NEW → ASSIGNED
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #9)
> Hi, I'm interested in this bug, I've taken first steps into this by making
> elements movable, making a placeholder, getting familiar, etc.
> 
> I guess I have to take a look at the methods |walkerActor| provides. I tried
> finding it's source but had no luck, can you guide me?
> 
> Thanks!

Hi Mahdi, I think Patrick will be able to give more details if you need them, but here is where the WalkerActor is declared: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#1129.
(In reply to Brian Grinstead [:bgrins] from comment #11)

> Hi Mahdi, I think Patrick will be able to give more details if you need
> them, but here is where the WalkerActor is declared:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> inspector.js#1129.

Thanks, it's really helpful to read the source, I'm finding clues of how to implement it.

Updated

5 years ago
Flags: needinfo?(pbrosset)
I think the |insertBefore| method could be useful here, but feel free to add new walkerActor methods if needed for this bug.

As you might have noticed already, the walkerActor is an object that runs server-side (in the debuggee) and that is responsible for walking/altering the DOM of whatever page/app is being inspected and sending information about it to the client-side inspector (the toolbox).
Most of the time, the information sent is a representation of DOM nodes. Because the debuggee can be on a remote device, we don't pass actual references to DOM node objects. We instead create NodeActor objects for them. Each node actor has a unique ID and a set of properties (like tagname, etc...) and a set of methods that can be called on them.

Node actors are usually what we send back and forth between the inspector client and server sides.
They're called Actors on the server, and Fronts on the client. In between, we have the devtools protocol that transform one in the other.

For this bug, once the user has moved the node in the markup-view and dropped it somewhere, you'll need to send its Front object to the WalkerActor, along with a "reference" Front so it knows where to move it to.
In MarkupView, you can access the walker with |this.walker|. So if you were to implement a new method like |MarkupView.prototype.onDrop(container)| where container would be the MarkupContainer of the node that's being moved around, then you'd need in this method to detect which other MarkupContainer it is being dropped next to, and then you could do something like: |this.walker.insertBefore(container, nextContainer.parentNode(), nextContainer)|.

As I said in a previous comment, the walkerActor also detects mutations of the DOM, so once the node is inserted at the new place, the walker will automatically send an event to the client-side which will update the markup-view's display.

Can I suggest that you work on several different patches? The drag/drop feature in the markup-view could be done in one, calling the walker actor could be done in another one, maybe a third patch could be done for adding undo capabilities to this new feature, and a fourth one for tests.
Flags: needinfo?(pbrosset)
Thanks for your explanations!

As far as I've understood, NodeFronts can be accessed inside a MarkupContainer by `this.node`, so this is what I send to walkerActor, right? 

This patch enables moving of an element inside markupview + an indicator to show it's destination.

Thanks
Attachment #8523419 - Flags: review?(pbrosset)

Comment 15

5 years ago
Comment on attachment 8523419 [details] [diff] [review]
Allow moving of elements in markupview and indicate their destination; r=patrick

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

::: browser/devtools/markupview/markup-view.css
@@ +75,5 @@
> +
> +#drop-placeholder {
> +  position: absolute;
> +  left: 1em;
> +  border-top: 2px dashed white;

nit : Please use a color from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors
> nit : Please use a color from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

Thanks, I tried different colors, most of them are hard to identify (at least on dark theme) and I found the lightest one to be the most appropriate.

Also fixed another bug when moving element bigger than normal around.
Attachment #8523419 - Attachment is obsolete: true
Attachment #8523419 - Flags: review?(pbrosset)
Attachment #8523503 - Flags: review?(pbrosset)

Comment 17

5 years ago
Comment on attachment 8523503 [details] [diff] [review]
Allow moving of elements in markupview and indicate their destination; r=patrick

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

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #16)
> > nit : Please use a color from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors
> 
> Thanks, I tried different colors, most of them are hard to identify (at
> least on dark theme) and I found the lightest one to be the most appropriate.
I would use Content (Text) - High Contrast here :

.theme-light #drop-placeholder {
  border: 1px solid #292e33;
}

.theme-dark #drop-placeholder {
  border: 1px solid #a9bacb;
}

Also, this patch needs rebasing off latest tip.

::: browser/devtools/markupview/markup-view.css
@@ +82,5 @@
> +  z-index: 6;
> +  pointer-events: none;
> +}
> +
> +.hidden {

I'm sure the hidden attribute will do.

::: browser/devtools/markupview/markup-view.js
@@ +1614,5 @@
> +
> +    var diff = event.pageY - this._startingMouseY;
> +    this.elt.style.top = diff + "px";
> +
> +    var el = this.elt,

nit : let instead of var
(same for other occurrences as well)

@@ +1654,5 @@
> +    // Moving up, our top offset will be element's top offset, we move from there
> +    // going down, we move from element's bottom, i.e. top + height
> +    var top = dir ? this._startingElementY : this._startingElementY + this._offsetHeight;
> +
> +    this.dropPlaceholder.style.top = (dir ? -1 : 1)*distance + top + "px";

nit : space before and after *
Thanks Tim! I applied your tips,

> Also, this patch needs rebasing off latest tip.

I didn't get what you mean :-s (couldn't find you at IRC)

Thanks,
Attachment #8523503 - Attachment is obsolete: true
Attachment #8523503 - Flags: review?(pbrosset)
Attachment #8523519 - Flags: review?(pbrosset)

Comment 19

5 years ago
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #18)
> Created attachment 8523519 [details] [diff] [review]
> Allow moving of elements in markupview and indicate their destination;
> r=patrick
> 
> Thanks Tim! I applied your tips,
> 
> > Also, this patch needs rebasing off latest tip.
> 
> I didn't get what you mean :-s (couldn't find you at IRC)
This means your patch is outdated, and will cause conflicts with the newer source.
To rebase your patch, you can do this :
hg qpop -a
hg pull -u (this will pull the latest changes)
hg qpush yourpatch.diff
Then copy back all rejected changes back in their file. Then do :
hg qref

Btw, I'm usually not IRC on weekends, but you can ask the #introduction channel.
Yep, as Tim said, you need to rebase your patch against the latest source code (for info, in the devtools team, we usually develop on the fx-team repository, but inbound repos are merged often, so it's not a big deal if you're not using it). See https://wiki.mozilla.org/DevTools/GetInvolved#Hacking.
I'm afraid the merge isn't going to be super trivial, looking at your patch, the code you started from dates from a rather long time (several weeks at least), and things change very quickly. I usually update every day, but that's because I work on mozilla source code every day, so I want my patches to always apply cleanly. I suggest you update once a week or so.
Comment on attachment 8523519 [details] [diff] [review]
Allow moving of elements in markupview and indicate their destination; r=patrick

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

Thanks for the patch!
Since it doesn't apply yet this is just a review of the code. I haven't been able to actually build locally with it and test the feature for myself.
I have made a few comments that you'll see below, but overall, my biggest problem is with the mouseMove method. It took me a long time to figure out what was going on in the while loop, and I'm still unsure about a few things :) Mind you, I haven't applied the patch, it usually help understand better.
The thing is it looks complex to me, and I'm not sure this complexity is required. Shouldn't you be able to detect which node is being hovered over, using the coordinates of the mouse (maybe using the elementFromPoint API). Also, it looks to me like actually appending the placeholder element where the mouse is, as part of the actual tree instead of being absolutely positioned on top would solve a lot of problems (for one, it would be indented nicely like the rest).
Let me know if this makes sense.

::: browser/devtools/markupview/markup-view.css
@@ +64,5 @@
>  }
>  
> +.dragging {
> +  position: relative;
> +  z-index: 5;

The markup-view doesn't have much absolutely positioned elements.
We only have the html-editor at z-index:2.
I think you could use 2 here and for #drop-placeholder.

@@ +65,5 @@
>  
> +.dragging {
> +  position: relative;
> +  z-index: 5;
> +  cursor: grabbing;

Shouldn't we also add -moz-user-select: none; here?

@@ +69,5 @@
> +  cursor: grabbing;
> +}
> +
> +.dragging * {
> +  cursor: grabbing;

Why is this needed? FWIW the * selector isn't very performant, please try and see exactly why you need this, and if really needed, use the actual classnames of the elements you need it for.

@@ +75,5 @@
> +
> +#drop-placeholder {
> +  position: absolute;
> +  left: 1em;
> +  width: 80%;

Why 80%? I can understand the reason behind left:1em, but I would do right:1em instead of specifying a width here.

@@ +76,5 @@
> +#drop-placeholder {
> +  position: absolute;
> +  left: 1em;
> +  width: 80%;
> +  height: 1px;

I think just removing the height altogether should be enough.

::: browser/devtools/markupview/markup-view.js
@@ +1361,5 @@
>    this.elt.addEventListener("mousedown", this._onMouseDown, false);
>  
> +  // Allow reordering using drag and drop
> +  this.elt.draggable = true;
> +  this.dropPlaceholder = this.markup._frame.contentWindow["drop-placeholder"];

We usually use getElementById instead.
Also, you can use this.markup.doc:
this.markup.doc.getElementById("drop-placeholder");

@@ +1366,5 @@
> +  this._onMouseUp = this._onMouseUp.bind(this);
> +  this._onMouseMove = this._onMouseMove.bind(this);
> +
> +  this.doc.body.addEventListener("mouseup", this._onMouseUp, true);
> +  this.doc.body.addEventListener("mousemove", this._onMouseMove, true);

Please remove these event listeners in the destroy method.

@@ +1579,5 @@
>      }
>      // Or it may be the "show more nodes" button (which already has its onclick)
>      // Else, it's the container itself
>      else if (target.nodeName !== "button") {
> +      this._mouseDown = true;

I think this property deserves a better name, to make the whole mechanism easier to understand. At least something like this._isMouseDown

@@ +1581,5 @@
>      // Else, it's the container itself
>      else if (target.nodeName !== "button") {
> +      this._mouseDown = true;
> +
> +      this.markup._frame.contentWindow.setTimeout(function() {

I see a lot of occurrences of this._frame.contentWindow and some occurrences of this.markup._frame.contentWindow.
Could you create a shortcut property in the MarkupView constructor?

this.win = this._frame.contentWindow;

And then replace all occurrences of |_frame.contentWindow| with |win|.

Also in the MarkupView.prototype.destroy method, do this.win = null;

If you replace |setTimeout(function() {| with |setTimeout(() => {|, you'll be able to get rid of the |.bind(this)| at the end of the function. Arrow functions are bound already.

@@ +1592,5 @@
> +        let left = this.elt.querySelector(".expander").getBoundingClientRect().left;
> +        this.dropPlaceholder.style.left = Math.floor(left) + "px";
> +        this.dropPlaceholder.style.top = this._startingElementY + this._offsetHeight + "px";
> +        this.dropPlaceholder.hidden = false;
> +      }.bind(this), 200);

200 is a magic number here, please add a const at the top of the file with a self-explanatory name (and optional comment if needed). See NEW_SELECTION_HIGHLIGHTER_TIMER for instance.

Also, can you elaborate why you're using a timeout here? This at least requires a comment in the code.

@@ +1609,5 @@
> +    this.elt.style.top = "0px";
> +  },
> +
> +  _onMouseMove: function(event) {
> +    if(!this.elt.classList.contains("dragging")) return;

Could we use a property instead here: this._isDragging ?
I find it more robust than checking for a class which, in time, could be used for other things.

@@ +1623,5 @@
> +
> +    // Tag lines are not necessarily 16px, they can change,
> +    // we loop over elements to calculate the distance traveled
> +    do {
> +      // We don't want to skip the first child, see 1646:9

What does 1646:9 mean here? Is that a line:column reference? If it is, you shouldn't use those in comments, the code changes all the time. Better use a method name for instance.

@@ +1635,5 @@
> +
> +      // ul.children
> +      if(el.tagName == "ul") {
> +        // the opening/closing .tag-line
> +        distance += el[dir ? "previousElementSibling" : "nextElementSibling"].offsetHeight + 0.4;

Using 0.4 here and further down below seems very fragile. First of all, for someone who doesn't know the code, it's impossible to understand why this is done (you could used a named const variable instead to help), secondly, this value might not actually be correct everywhere (different OSs, different themes, different zoom levels).

@@ +1657,5 @@
> +    let top = dir ? this._startingElementY : this._startingElementY + this._offsetHeight;
> +
> +    this.dropPlaceholder.style.top = (dir ? -1 : 1) * distance + top + "px";
> +    let left = el.querySelector(".expander").getBoundingClientRect().left;
> +    this.dropPlaceholder.style.left = Math.floor(left) + "px";

As I said, I haven't applied the patch locally, so I can't see what this looks like, but why are you updating the left coordinate? Is it so that the placeholder would appear indented like the other elements?
Attachment #8523519 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21)
> Comment on attachment 8523519 [details] [diff] [review]
> Allow moving of elements in markupview and indicate their destination;
> r=patrick
> 
> Review of attachment 8523519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch!
> Since it doesn't apply yet this is just a review of the code. I haven't been
> able to actually build locally with it and test the feature for myself.
> I have made a few comments that you'll see below, but overall, my biggest
> problem is with the mouseMove method. It took me a long time to figure out
> what was going on in the while loop, and I'm still unsure about a few things
> :) Mind you, I haven't applied the patch, it usually help understand better.
> The thing is it looks complex to me, and I'm not sure this complexity is
> required. Shouldn't you be able to detect which node is being hovered over,
> using the coordinates of the mouse (maybe using the elementFromPoint API).
> Also, it looks to me like actually appending the placeholder element where
> the mouse is, as part of the actual tree instead of being absolutely
> positioned on top would solve a lot of problems (for one, it would be
> indented nicely like the rest).
> Let me know if this makes sense.

Thanks, I thought about actually inserting the placeholder instead of moving it around, I don't remember why but something was in the way, I'll try it again with help of elementFromPoint, I didn't know about this API.

> ::: browser/devtools/markupview/markup-view.css
> @@ +64,5 @@
> >  }
> >  
> > +.dragging {
> > +  position: relative;
> > +  z-index: 5;
> 
> The markup-view doesn't have much absolutely positioned elements.
> We only have the html-editor at z-index:2.
> I think you could use 2 here and for #drop-placeholder.
> 

Sure, I'll change that.

> @@ +65,5 @@
> >  
> > +.dragging {
> > +  position: relative;
> > +  z-index: 5;
> > +  cursor: grabbing;
> 
> Shouldn't we also add -moz-user-select: none; here?

Adding `this.elt.dragging = true` seems to do that, but I'll add it just in case.

> 
> @@ +69,5 @@
> > +  cursor: grabbing;
> > +}
> > +
> > +.dragging * {
> > +  cursor: grabbing;
> 
> Why is this needed? FWIW the * selector isn't very performant, please try
> and see exactly why you need this, and if really needed, use the actual
> classnames of the elements you need it for.
> 

Something was replacing this style and the cursor would be `default` in some cases, I'll change it to  class names.

> @@ +75,5 @@
> > +
> > +#drop-placeholder {
> > +  position: absolute;
> > +  left: 1em;
> > +  width: 80%;
> 
> Why 80%? I can understand the reason behind left:1em, but I would do
> right:1em instead of specifying a width here.
> 

Most of these were there just so I could test it, I forgot to modify these, sorry. :D

> @@ +76,5 @@
> > +#drop-placeholder {
> > +  position: absolute;
> > +  left: 1em;
> > +  width: 80%;
> > +  height: 1px;
> 
> I think just removing the height altogether should be enough.
> 

Right

> ::: browser/devtools/markupview/markup-view.js
> @@ +1361,5 @@
> >    this.elt.addEventListener("mousedown", this._onMouseDown, false);
> >  
> > +  // Allow reordering using drag and drop
> > +  this.elt.draggable = true;
> > +  this.dropPlaceholder = this.markup._frame.contentWindow["drop-placeholder"];
> 
> We usually use getElementById instead.
> Also, you can use this.markup.doc:
> this.markup.doc.getElementById("drop-placeholder");
> 

I was using `getElementById` at first, I thought this would be just a little faster as it's already there, probably.

> @@ +1366,5 @@
> > +  this._onMouseUp = this._onMouseUp.bind(this);
> > +  this._onMouseMove = this._onMouseMove.bind(this);
> > +
> > +  this.doc.body.addEventListener("mouseup", this._onMouseUp, true);
> > +  this.doc.body.addEventListener("mousemove", this._onMouseMove, true);
> 
> Please remove these event listeners in the destroy method.
> 

I hadn't noticed the destroy method, I'll try to clean up everything.

> @@ +1579,5 @@
> >      }
> >      // Or it may be the "show more nodes" button (which already has its onclick)
> >      // Else, it's the container itself
> >      else if (target.nodeName !== "button") {
> > +      this._mouseDown = true;
> 
> I think this property deserves a better name, to make the whole mechanism
> easier to understand. At least something like this._isMouseDown
> 

Right

> @@ +1581,5 @@
> >      // Else, it's the container itself
> >      else if (target.nodeName !== "button") {
> > +      this._mouseDown = true;
> > +
> > +      this.markup._frame.contentWindow.setTimeout(function() {
> 
> I see a lot of occurrences of this._frame.contentWindow and some occurrences
> of this.markup._frame.contentWindow.
> Could you create a shortcut property in the MarkupView constructor?
> 
> this.win = this._frame.contentWindow;
> 
> And then replace all occurrences of |_frame.contentWindow| with |win|.
> 
> Also in the MarkupView.prototype.destroy method, do this.win = null;
> 

You're right

> If you replace |setTimeout(function() {| with |setTimeout(() => {|, you'll
> be able to get rid of the |.bind(this)| at the end of the function. Arrow
> functions are bound already.
> 

Oh, didn't know about that, thanks.

> @@ +1592,5 @@
> > +        let left = this.elt.querySelector(".expander").getBoundingClientRect().left;
> > +        this.dropPlaceholder.style.left = Math.floor(left) + "px";
> > +        this.dropPlaceholder.style.top = this._startingElementY + this._offsetHeight + "px";
> > +        this.dropPlaceholder.hidden = false;
> > +      }.bind(this), 200);
> 
> 200 is a magic number here, please add a const at the top of the file with a
> self-explanatory name (and optional comment if needed). See
> NEW_SELECTION_HIGHLIGHTER_TIMER for instance.
> 
> Also, can you elaborate why you're using a timeout here? This at least
> requires a comment in the code.
> 

It was so annoying that if I was going to click an element just to see properties, and suddenly a dashed line would appear out of nowhere, or trying to double click, it would flash.

+ I would like to know what's your idea for the delay, how much is prefered?

> @@ +1609,5 @@
> > +    this.elt.style.top = "0px";
> > +  },
> > +
> > +  _onMouseMove: function(event) {
> > +    if(!this.elt.classList.contains("dragging")) return;
> 
> Could we use a property instead here: this._isDragging ?
> I find it more robust than checking for a class which, in time, could be
> used for other things.
> 

I thought the same thing, but forgot to apply it. 

> @@ +1623,5 @@
> > +
> > +    // Tag lines are not necessarily 16px, they can change,
> > +    // we loop over elements to calculate the distance traveled
> > +    do {
> > +      // We don't want to skip the first child, see 1646:9
> 
> What does 1646:9 mean here? Is that a line:column reference? If it is, you
> shouldn't use those in comments, the code changes all the time. Better use a
> method name for instance.
> 

Yes, it's a line:column. You're right, I'll change it.

> @@ +1635,5 @@
> > +
> > +      // ul.children
> > +      if(el.tagName == "ul") {
> > +        // the opening/closing .tag-line
> > +        distance += el[dir ? "previousElementSibling" : "nextElementSibling"].offsetHeight + 0.4;
> 
> Using 0.4 here and further down below seems very fragile. First of all, for
> someone who doesn't know the code, it's impossible to understand why this is
> done (you could used a named const variable instead to help), secondly, this
> value might not actually be correct everywhere (different OSs, different
> themes, different zoom levels).
> 

I really don't know why is this 0.4 required, I see no extra margin/padding, but it was somehow required. I guess you're right it will probably break on other themes / OSes, I'll try to find out where does this come from.

> @@ +1657,5 @@
> > +    let top = dir ? this._startingElementY : this._startingElementY + this._offsetHeight;
> > +
> > +    this.dropPlaceholder.style.top = (dir ? -1 : 1) * distance + top + "px";
> > +    let left = el.querySelector(".expander").getBoundingClientRect().left;
> > +    this.dropPlaceholder.style.left = Math.floor(left) + "px";
> 
> As I said, I haven't applied the patch locally, so I can't see what this
> looks like, but why are you updating the left coordinate? Is it so that the
> placeholder would appear indented like the other elements?

Yes.

> I usually update every day, but that's because I work on
> mozilla source code every day, so I want my patches to always 
> apply cleanly. I suggest you update once a week or so.

The problem is it takes me a lot of time to build Firefox, I guess I have to set it to build at nights.


Thanks Patrick.
The code is cut down to just a few lines, phew.

I think the drag/drop functionality is ready, I'll wait for your review and start the actual reordering afterwards.

Thanks Patrick
Attachment #8523519 - Attachment is obsolete: true
Attachment #8526845 - Flags: review?(pbrosset)

Comment 24

5 years ago
Comment on attachment 8526845 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

::: browser/devtools/markupview/markup-view.js
@@ +1545,5 @@
> +    this._isMouseDown = false;
> +    this._isDragging = false;
> +    this.elt.classList.remove("dragging");
> +    this.dropPlaceholder.hidden = true;
> +    this.elt.style.top = "0px";

nit : If you want to remove the top css property, this would be better :

this.elt.style.removeProperty("top")
Thanks Tim,

I had also forgot to use double-quote instead of single-quotes, fixed.
Attachment #8526845 - Attachment is obsolete: true
Attachment #8526845 - Flags: review?(pbrosset)
Attachment #8526920 - Flags: review?(pbrosset)
Comment on attachment 8526920 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

Thanks for the updated patch.

A few remarks after playing with it locally:
- I think we need to temporarily disable the _showContainerAsHovered and the _showBoxModel while dragging, it's very distracting. I think you can do both at once in MarkupView._onMouseMove and MarkupView._onMouseLeave (just add an isDragging flag on MarkupView).
- The 2px placeholder is a little bit annoying because appending it in between nodes makes the nodes below move by 2px down. It's not much, but still noticeable. Sorry I did not think of this before, but we have an element in the markup-view which would be perfect for this use case, instead of adding this new placeholder element: the tag-state element. This is an element that's inside each and every tag-line element (so in each and every MarkupContainer) and that we use for 2 things right now: showing when a node is selected or hovered, and when a node is being mutated.
We could very easily also use it for this new use case.
Adding this to the css:

.tag-state.drop-target {
  border-top: 2px dashed;
}

.theme-light .tag-state.drop-target {
  border-color: #292e33;
}

.theme-dark .tag-state.drop-target {
  border-color: #a9bacb;
}

Then removing the placeholder node you added. And instead of moving a node, you would have to detect the position of the drop (as you're doing now) and add the class "drop-target" to the tag-state at this position.

The tag-state node is out of the flow (absolutely positioned below the container) so adding a border to it won't move anything around it.
Of course, you'll have to store which tag-state has had the class added previously, so that you can then remove the border when moving the mouse somewhere else.

I believe this would again simplify _moveDropPlaceholder further.

Other than this and the few comments in the code below, this is getting close.

::: browser/devtools/markupview/markup-view.css
@@ +89,5 @@
> +  border-color: #a9bacb;
> +}
> +
> +
> +

nit: Just one empty line in between rules is enough.

::: browser/devtools/markupview/markup-view.js
@@ +1502,5 @@
>    parentContainer: function() {
>      return this.elt.parentNode ? this.elt.parentNode.container : null;
>    },
>  
> +

nit: just one empty line is enough.

@@ +1505,5 @@
>  
> +
> +  _isMouseDown: false,
> +  _isDragging: false,
> +  _mustAppend: false,

The value of mustAppend is only set, never read. Did you forget to remove it?

@@ +1561,5 @@
> +  },
> +
> +  _moveDropPlaceholder: function(x, y) {
> +    // At mouseDown, we don't need x,y to know the elements, |this.elt| is our target
> +    let target = arguments.length ? this.markup.doc.elementFromPoint(x, y) : this.elt.querySelector(".tag-line");

strict mode doesn't allow using the |arguments| keyword. This file isn't marked "use strict"; but it should, so let's assume that it is, and not use |arguments|.
You can test the presence of x and y instead here.

In fact, after thinking about it a bit more, I would make this method accept a single argument which would be a node. And instead, make the 2 callers of this method provide the node.
So, in _onMouseDown, we would call it with this.elt.querySelector(".tag-line"), and in _onMouseMove, we would call it with this.markup.doc.elementFromPoint(x, y).

This should make this function simpler and more reusable. And then you could rename it something like _showDropPlaceholderAt.

@@ +1564,5 @@
> +    // At mouseDown, we don't need x,y to know the elements, |this.elt| is our target
> +    let target = arguments.length ? this.markup.doc.elementFromPoint(x, y) : this.elt.querySelector(".tag-line");
> +    if(!target) return;
> +
> +    target = target.closest(".tag-line");

Nice use of this API, I didn't even know it, it sounds super useful!
Attachment #8526920 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26)
> We could very easily also use it for this new use case.
> Adding this to the css:
> 
> .tag-state.drop-target {
>   border-top: 2px dashed;
> }
> 
> .theme-light .tag-state.drop-target {
>   border-color: #292e33;
> }
> 
> .theme-dark .tag-state.drop-target {
>   border-color: #a9bacb;
> }
> 

Please use this instead (we now have the ability to use CSS variables to make this simpler):

  .tag-state.drop-target {
    border-top: 2px dashed var(--theme-content-color1);
  }
(In reply to Brian Grinstead [:bgrins] from comment #27)
> Please use this instead (we now have the ability to use CSS variables to
> make this simpler):
> 
>   .tag-state.drop-target {
>     border-top: 2px dashed var(--theme-content-color1);
>   }
That's right! Very cool. Thanks Brian.
Thanks Patrick!

`.tag-state` would add complexity as it was not available where it was necessary. An example would be inserting after the last child, there was no `.tag-state` to rely on (closing `.tag-lines` don't seem to have a `.tag-state`). Also with `.tag-state` the indentation level was not visible, instead I used the `.tag-line` and it's working fine. But the 2px push-down is still there. 

`.tag-line` doesn't have a fixed height, thus `box-sizing` won't work (tested). A solution I thought of was using psuedo-elements instead of border, what do you think?

I renamed `moveDropPlaceholder` to `indicateDropTarget` and the function now accepts an element as it's only argument.

I also increased the GRAB_DELAY to 400 as it was breaking doubleclicks.

Code is a lot clearer now, thanks!

+ Thank you Brian!
Attachment #8526920 - Attachment is obsolete: true
Attachment #8530086 - Flags: review?(pbrosset)

Comment 30

5 years ago
Comment on attachment 8530086 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

::: browser/devtools/markupview/markup-view.xhtml
@@ +100,5 @@
>    <div id="previewbar" class="disabled">
>       <div id="preview"/>
>       <div id="viewbox"/>
>    </div>
> +  <div id="drop-placeholder" hidden=""></div>

I think you forgot to remove this.
Oh, right :)) Thanks Tim
Attachment #8530086 - Attachment is obsolete: true
Attachment #8530086 - Flags: review?(pbrosset)
Attachment #8530331 - Flags: review?(pbrosset)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #29)
> `.tag-state` would add complexity as it was not available where it was
> necessary. An example would be inserting after the last child, there was no
> `.tag-state` to rely on (closing `.tag-lines` don't seem to have a
> `.tag-state`). Also with `.tag-state` the indentation level was not visible,
> instead I used the `.tag-line` and it's working fine. But the 2px push-down
> is still there.
I see, let's go for .tag-line then.

> `.tag-line` doesn't have a fixed height, thus `box-sizing` won't work
> (tested). A solution I thought of was using psuedo-elements instead of
> border, what do you think?
Good idea. tag-line is already a positioning context (position:relative), so it would be easy to absolulty position the ::before or ::after pseudo-element at the right place, without it causing a push-down on the other elements.

I'll review the new patch probably next week.
Sorry, I've been really busy in the few past days. 

Here is the patch using psuedo-elements for border
Attachment #8530331 - Attachment is obsolete: true
Attachment #8530331 - Flags: review?(pbrosset)
Attachment #8531999 - Flags: review?(pbrosset)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #33)
> Created attachment 8531999 [details] [diff] [review]
> Bug 858038 - Allow moving of elements in markupview and indicate their
> destination; r=patrick
> 
> Sorry, I've been really busy in the few past days. 
Don't worry about it, *I* have been very busy too last week, I didn't get to do any reviews at all.
I'll get to your review tomorrow morning. Thanks for updating that patch.
Comment on attachment 8531999 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

This looks good.

A few things I've seen when playing with it:
- I think it works better when the dragged node is visually taken out of its original place (by adding height:0; to .child.dragging as commented below).
- Dragging an expanded container around makes it impossible to insert in some places. I can't be really much more specific, but try expanding a few nodes, then grabbing the parent, and try to insert it. Some insertion points seem to simply not be possible. However, adding height:0; as said above seems to fix this.
- Dragging an element all the way to the bottom or top, when the view has a scrollbar, doesn't scroll the iframe up or down automatically, so it makes it impossible to insert an element somewhere below the fold. I'm happy for this last point to be handled in a separate patch after this one is out of the way, to make sure we keep patches small and easy to review.

Other than this, it seems to work pretty well. I'm happy with the pseudo-element approach.

Oh, one other thing, you haven't disabled the mousemove thing in MarkupView while dragging. So it's quite distracting to see the nodes being highlighted while moving.
Here's how you can fix it: in MarkupView _onMouseMove and _onMouseLeave, just add these lines to the top of both functions:

    if (this.isDragging) {
      return;
    }

Then, in MarkupContainer, where you do this._isDragging = true; and this._isDragging = false; you should also do this.markup.isDragging = true; and this.markup.isDragging = false;

::: browser/devtools/markupview/markup-view.css
@@ +14,5 @@
>    min-width: 100%;
>  }
>  
> +body.dragging .tag-line {
> +  cursor: grabbing !important;

!important is unfortunate here. Can you explain why it is needed? I think it should be possible to avoid it just by making the selector more specific.

@@ +69,5 @@
>  
> +.child.dragging {
> +  position: relative;
> +  pointer-events: none;
> +  opacity: 0.4;

I think 0.4 is too low, it's hard to see the node being moved.

Also, I think you should add height:0; here.
This would avoid the blank that appears in place of the dragged node. I've tested it and I makes the whole thing better ux-wise I think.

It's especially important when dragging an expanded container, as the blank space that is left behind is just too big otherwise and takes most of the room in the markup-view for nothing.

@@ +73,5 @@
> +  opacity: 0.4;
> +  -moz-user-select: none;
> +}
> +
> +.tag-line.drop-target:before {

:before should in fact be ::before

::: browser/devtools/markupview/markup-view.js
@@ +1613,5 @@
> +      this.win.setTimeout(() => {
> +        if(!this._isMouseDown) return;
> +        this.elt.classList.add("dragging");
> +        this.markup.doc.body.classList.add("dragging");
> +        this._isDragging = true;

The 2 last lines would look better in a setter, something like this:

set isDragging(isDragging) {
  if (isDragging) {
    this.elt.classList.add("dragging");
    this.markup.doc.body.classList.add("dragging");
    this._isDragging = true;
    this.markup.isDragging = true;
  } else {
    this._isDragging = false;
    this.markup.isDragging = false;
    this.elt.classList.remove("dragging");
    this.markup.doc.body.classList.remove("dragging");
  }
}

get isDragging() {
  return this._isDragging;
}

This way, you can simply do this.isDragging = true; in _onMouseDown and this.isDragging = false; in _onMouseUp, and it's more maintainable.

@@ +1633,5 @@
> +    this.elt.classList.remove("dragging");
> +    this.markup.doc.body.classList.remove("dragging");
> +    this.elt.style.removeProperty("top");
> +    if(this._lastDropTarget)
> +      this._lastDropTarget.classList.remove("drop-target");

Please parens around if blocks.

@@ +1646,5 @@
> +    let el;
> +
> +    // as our dragging element starts moving, we'll have an empty
> +    // space between it's neighbours which will then refer to the parent
> +    // if we use elementFromPoint, let's have an exception

The height:0 change should remove the need for this exception, right?

@@ +1657,5 @@
> +    }
> +    this._indicateDropTarget(el);
> +  },
> +
> +  _indicateDropTarget: function(element) {

I don't think this needs to be marked as a private method (no leading _), also this requires a comment.

And in fact, element is any element in the markup-view, so it's not really under the responsibility of the MarkupContainer to do this. I think this method should be in MarkupView instead.

Now that I think of it, in _onMouseMove, if we don't need to have the special blank space case, we're left with just needing to find the 2 x and y coordinates, and then we could just call a new method on MarkupView that deals with calling elementFromPoint and then indicateDropTarget.
It's a better separation of concerns. The MarkupContainer class should only really have to deal with its own elements, not start making changes to other containers around it.
Attachment #8531999 - Flags: review?(pbrosset)
Thanks for your review Patrick!

I've applied your tips.

I thought it'd be better if indicateDropTarget accepted both x,y and element, as we're using it both ways. In some events event.target is our target so we don't have to use |elementFromPoint| while in some conditions it's required to use |elementFromPoint|. What do you think?


Also for the next patch (probably) I thought we would need another indicator to show element's initial place as the developer might get confused about where he picked the element up.

Thanks!
Attachment #8531999 - Attachment is obsolete: true
Attachment #8535644 - Flags: review?(pbrosset)

Updated

5 years ago
Flags: needinfo?(pbrosset)

Comment 37

5 years ago
Comment on attachment 8535644 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

::: browser/devtools/markupview/markup-view.css
@@ +69,5 @@
>  
> +.child.dragging {
> +  position: relative;
> +  pointer-events: none;
> +  opacity: 0.7;

Since this is invisible (height: 0), this rule is useless.

::: browser/devtools/markupview/markup-view.js
@@ +158,5 @@
>          this._isImagePreviewTarget.bind(this));
>      }
>    },
>  
> +  isDragging: false,

Not sure if this is needed with the new getters and setters.
Thanks Tim for your tips!

(In reply to Tim Nguyen [:ntim] from comment #37)
> Comment on attachment 8535644 [details] [diff] [review]
> Bug 858038 - Allow moving of elements in markupview and indicate their
> destination; r=patrick
> 
> Review of attachment 8535644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/markup-view.css
> @@ +69,5 @@
> >  
> > +.child.dragging {
> > +  position: relative;
> > +  pointer-events: none;
> > +  opacity: 0.7;
> 
> Since this is invisible (height: 0), this rule is useless.
> 

The element doesn't become invisible, it just doesn't take up space in the markup.

> ::: browser/devtools/markupview/markup-view.js
> @@ +158,5 @@
> >          this._isImagePreviewTarget.bind(this));
> >      }
> >    },
> >  
> > +  isDragging: false,
> 
> Not sure if this is needed with the new getters and setters.

Does it introduce any overheads? because I think it's better to have it as it proves existence of such property.

Comment 39

5 years ago
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #38)
> Thanks Tim for your tips!
> 
> (In reply to Tim Nguyen [:ntim] from comment #37)
> > Comment on attachment 8535644 [details] [diff] [review]
> > Bug 858038 - Allow moving of elements in markupview and indicate their
> > destination; r=patrick
> > 
> > Review of attachment 8535644 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/markupview/markup-view.css
> > @@ +69,5 @@
> > >  
> > > +.child.dragging {
> > > +  position: relative;
> > > +  pointer-events: none;
> > > +  opacity: 0.7;
> > 
> > Since this is invisible (height: 0), this rule is useless.
> > 
> 
> The element doesn't become invisible, it just doesn't take up space in the
> markup.
If it doesn't take up space in the markup view, the opacity effect won't be visible anyway right ?

> > ::: browser/devtools/markupview/markup-view.js
> > @@ +158,5 @@
> > >          this._isImagePreviewTarget.bind(this));
> > >      }
> > >    },
> > >  
> > > +  isDragging: false,
> > 
> > Not sure if this is needed with the new getters and setters.
> 
> Does it introduce any overheads? because I think it's better to have it as
> it proves existence of such property.
I'm not sure, I'll leave the answer to pbrosset.
(In reply to Tim Nguyen [:ntim] from comment #39)
> (In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #38)
> > Thanks Tim for your tips!
> > 
> > (In reply to Tim Nguyen [:ntim] from comment #37)
> > > Comment on attachment 8535644 [details] [diff] [review]
> > > Bug 858038 - Allow moving of elements in markupview and indicate their
> > > destination; r=patrick
> > > 
> > > Review of attachment 8535644 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: browser/devtools/markupview/markup-view.css
> > > @@ +69,5 @@
> > > >  
> > > > +.child.dragging {
> > > > +  position: relative;
> > > > +  pointer-events: none;
> > > > +  opacity: 0.7;
> > > 
> > > Since this is invisible (height: 0), this rule is useless.
> > > 
> > 
> > The element doesn't become invisible, it just doesn't take up space in the
> > markup.
> If it doesn't take up space in the markup view, the opacity effect won't be
> visible anyway right ?

The element becomes relatively positioned and follows the mouse, everything works as expected, the problem was, without height: 0, there would be an empty space at element's initial place, by adding height: 0, the empty space is gone but the .tag-line remains visible (the wrapper element .child gets the height: 0, but without overflow: hidden, it's children are visible).
Comment on attachment 8535644 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

So, this looks good to me.
Just 2 things I'd like to see revisited:
- the indicateDropTarget signature
- the usage of _lastDropTarget
Apart from these, I only have a few minor comments.
Once this is done, I'm happy to R+ the patch.

The next steps would be:

- make the markup-view auto-scroll when dragging an element towards the edge of the frame (this can be done in a separate patch)
- adding an indicator for the original position of the node (that's indeed a good idea, thanks for mentioning it) (can also be done in a separate patch)
- calling the right method on the walker to append the node on drop (other patch too)
- adding automated tests to avoid future regressions (other patch too).

How do you feel about this plan? I know this patch has been going on for a while, tell me if you need/want help on some of the next steps.

::: browser/devtools/markupview/markup-view.css
@@ +71,5 @@
> +  position: relative;
> +  pointer-events: none;
> +  opacity: 0.7;
> +  -moz-user-select: none;
> +  height: 0;

Can you add a comment here that explains why we're making the dragged element 0px high?

@@ +74,5 @@
> +  -moz-user-select: none;
> +  height: 0;
> +}
> +
> +.tag-line.drop-target::before {

And also a comment above this selected explaining what it is used for.

::: browser/devtools/markupview/markup-view.js
@@ +1435,5 @@
>        this._previewBar.classList.remove("hide");
>      }, 1000);
> +  },
> +
> +  _lastDropTarget: null,

nit: I don't think we usually add these flag properties. Booleans like isDragging are fine, because their initial values carry meaning. Here, lastDropTarget simply doesn't exist before we've dragged any node, so we usually don't define at all.
I know we have _hoveredNode here which is pretty similar, but to me, it should be removed (that's for another bug though).

@@ +1441,5 @@
> +  /**
> +   * Takes x,y coords or an element as it's only argument and
> +   * marks the element as drop target (adding dashed border)
> +   */
> +  indicateDropTarget: function(x, y) {

I still don't think this is the right way to declare this method. Right now, it can either accept a DOM element or a set of x,y coordinates. 
- the comment doesn't say so
- functions that can take a varying number of arguments and argument types are harder to understand and maintain
See my comment below near the callers of this function.

@@ +1444,5 @@
> +   */
> +  indicateDropTarget: function(x, y) {
> +    let el = typeof y !== "undefined" ? this.doc.elementFromPoint(x, y) : x;
> +
> +    let target = el.classList.contains("tag-line") ? el : el.querySelector(".tag-line") || el.closest(".tag-line");

nit: please find a good place to cut this line in several lines so it fits in 80 cols.
Maybe: 
    let target = el.classList.contains("tag-line") ?
      el : el.querySelector(".tag-line") || el.closest(".tag-line");

@@ +1449,5 @@
> +    if(!target) return;
> +
> +    target = target.closest(".tag-line");
> +
> +    this._lastDropTarget.classList.remove("drop-target");

Just enclose this in an if (this._lastDropTarget) {...} and you're good.
Then you don't have to access this.markup._lastDropTarget in the MarkupContainer below (see my comment below).

@@ +1633,5 @@
>    },
>  
> +  _isMouseDown: false,
> +  _isDragging: false,
> +  _startingMouseY: 0,

nit: rename this to _dragStartY

@@ +1673,5 @@
> +        if(!this._isMouseDown) return;
> +        this.isDragging = true;
> +
> +        this._startingMouseY = event.pageY;
> +        this.markup._lastDropTarget = this.elt;

You've made _lastDropTarget a private property of MarkupView (which I think is reasonable), so you shouldn't be accessing it from here.
Why not handle this entirely inside indicateDropTarget? This would make sense.
Whenever indicateDropTarget is called, it removes the class to this._lastDropTarget if it exists, then adds the class to the new target instead, and sets the value to _lastDropTarget

@@ +1696,5 @@
> +    let diff = event.pageY - this._startingMouseY;
> +    this.elt.style.top = diff + "px";
> +
> +    // let el = this.markup.doc.elementFromPoint(event.pageX - this.win.scrollX, event.pageY - this.win.scrollY);
> +    this.markup.indicateDropTarget(event.pageX - this.win.scrollX, event.pageY - this.win.scrollY);

So, as said above, why not pass el here, instead of passing the 2 coordinates. This would make indicateDropTarget simpler.
Attachment #8535644 - Flags: review?(pbrosset)
Clearing the needinfo flag.
Flags: needinfo?(pbrosset)
Sorry, I've been really busy in the past few days.

I applied your tips, you were right about indicateDropTarget, it just got better.

There's something I'm not sure why it happens but it seems to be related to this patch. Try double clicking on an html attribute, then try moving text cursor by clicking on the text, it won't work. I tried it after popping the patch and it works. Any idea?

Thanks!
Attachment #8535644 - Attachment is obsolete: true
Attachment #8541681 - Flags: review?(pbrosset)
Comment on attachment 8541681 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=patrick

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

I have found the reason for the reason why clicking in the text wouldn't work anymore, and also fixed another edge case (dragging an element without waiting for the delay), and made a few more comments below.
I actually ended up testing my fixes locally, so I have a patch that I will upload for you (it also includes a bit of cleanup of the code which I think we can make while working on this bug).

::: browser/devtools/markupview/markup-view.css
@@ +72,5 @@
> +.child.dragging {
> +  position: relative;
> +  pointer-events: none;
> +  opacity: 0.7;
> +  /* this rule prevents the texts to be selected while dragging */

Sorry I wasn't clear in my previous review. I wasn't asking to add a comment here (I think -moz-user-select doesn't require a comment), but below, just before the ".tag-line.drop-target::before" selector. The comment should explain that this rule is used to display a line where the element being dragged will be inserted.

::: browser/devtools/markupview/markup-view.js
@@ +1450,5 @@
> +    if(this._lastDropTarget) {
> +      this._lastDropTarget.classList.remove("drop-target");
> +    }
> +
> +    // target = target.closest(".tag-line");

This commented out line should be removed.

@@ +1505,5 @@
>  
>      this._onToggle = this._onToggle.bind(this);
>  
> +    // Allow reordering using drag and drop
> +    this.elt.draggable = true;

Setting this property isn't needed, and it seems to be what causes the inplace-editor not to work correctly.

@@ +1507,5 @@
>  
> +    // Allow reordering using drag and drop
> +    this.elt.draggable = true;
> +    this._onMouseUp = this._onMouseUp.bind(this);
> +    this._onMouseMove = this._onMouseMove.bind(this);

nit: I think we should re-organize this function to do all of its function bindings in one place first, and then add all its event listeners.

@@ +1671,5 @@
>      else if (target.nodeName !== "button") {
> +      this._isMouseDown = true;
> +
> +      this.win.setTimeout(() => {
> +        if(!this._isMouseDown) return;

I found one more edge-case: try to mouse-down and drag an element in the markup-view without waiting for the 400ms. It still works.
So, this condition should also check that the currently hovered node is still the original target.

@@ +1696,5 @@
> +
> +    let diff = event.pageY - this._dragStartY;
> +    this.elt.style.top = diff + "px";
> +
> +    let el = this.markup.doc.elementFromPoint(event.pageX - this.win.scrollX, event.pageY - this.win.scrollY);

nit: please cut this line to 80 cols.
Attachment #8541681 - Flags: review?(pbrosset)
Please take a look at the corrections made in this patch.
I think the only remaining problem now is auto-scrolling of the markup-view when the mouse reaches the top of bottom edge of the frame.
Attachment #8541681 - Attachment is obsolete: true
Flags: needinfo?(mdibaiee)
Hi,

I'm sorry for having too many small mistakes in the code, and for being late at answering. Thank you Patrick!

I coded the auto-scroll, I think there's something left here, adding a first-place indicator for the dragged element, I didn't implement anything to ask you about how you think it should be implemented?

Should it be an element appended after the element with some visuals, or do you have another idea?

Also, about the auto-scroll, I'm not sure if there is a specific way of implementing it (the auto-scroll differs from the top edge auto-scroll done by the browser itself).

Thank you Patrick!
Flags: needinfo?(mdibaiee)
Attachment #8547223 - Flags: review?(pbrosset)
Comment on attachment 8547223 [details] [diff] [review]
Bug 858038 - Auto-scroll while dragging elements up / down, add a first place indicator for dragged element

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

::: browser/devtools/markupview/markup-view.js
@@ +61,5 @@
>  function MarkupView(aInspector, aFrame, aControllerWindow) {
>    this._inspector = aInspector;
>    this.walker = this._inspector.walker;
>    this._frame = aFrame;
> +  this.win = this._frame.contentWindow;

We need to set this to null in the destroy function of this class. And in fact this.doc too, it isn't nullified for now:

this.win = this.doc = null;

@@ +162,5 @@
>  
>    isDragging: false,
>  
>    _onMouseMove: function(event) {
> +    if(this.isDragging) {

nit: please 1 space between if and (

@@ +164,5 @@
>  
>    _onMouseMove: function(event) {
> +    if(this.isDragging) {
> +      event.preventDefault();
> +      var docEl = this.doc.documentElement;

let instead of var

@@ +167,5 @@
> +      event.preventDefault();
> +      var docEl = this.doc.documentElement;
> +
> +      // Auto-scroll when the mouse approaches bottom edge
> +      var distance = docEl.clientHeight - event.pageY + this.win.scrollY;

let instead of var.

This is the right way to handle this, but this only works for the bottom edge.
We also need:
let distanceFromTop = event.pageY - this.win.scrollY;

(and so, you should rename distance to distanceFromBottom)

@@ +169,5 @@
> +
> +      // Auto-scroll when the mouse approaches bottom edge
> +      var distance = docEl.clientHeight - event.pageY + this.win.scrollY;
> +
> +      if(distance <= 50) {

50 is what we call a magic number here. Instead, define a const at the top of the file (next to |const GRAB_DELAY = 400;|) like this:

const DRAGDROP_AUTOSCROLL_EDGE_DISTANCE = 50;

and rewrite the condition to:

if (distanceFromBottom <= DRAGDROP_AUTOSCROLL_EDGE_DISTANCE) {
  ...
} else if (distanceFromTop <= DRAGDROP_AUTOSCROLL_EDGE_DISTANCE) {
  ...
}

This way, it's obvious to future readers of the code what this is.

@@ +171,5 @@
> +      var distance = docEl.clientHeight - event.pageY + this.win.scrollY;
> +
> +      if(distance <= 50) {
> +        // Scale our distance to 5-15 range
> +        var speed = distance / 50 * (15 - 5) + 5;

let instead of var.

More magic numbers here, in fact this whole calculation looks really mysterious, why (15-5), and why scaling from 5 to 15? People going through this code later will have a hard time understanding and fixing bugs in there.

I suggest adding this helper function at the bottom of the file somewhere (after parseAttributeValues):

/**
 * Map a number from one range to another.
 */
function map(value, istart, istop, ostart, ostop) {
  let ratio = istop - istart;
  if (ratio == 0) {
    return value;
  }
  return ostart + (ostop - ostart) * ((value - istart) / ratio);
}

Then you can use it like this:

// Map our distance from 0-50 to 5-15 instead.
let speed = map(distance, 0, DRAGDROP_AUTOSCROLL_EDGE_DISTANCE, 5, 15);

Also please make this comment better so readers know *why* you want to map to the 5-15 range.
And please define 5 and 15 as const at the top of the file, with self-explanatory names too.

@@ +176,5 @@
> +
> +        // Here, we use minus because the value of speed - 15 is always negative
> +        // and it makes the speed relative to the distance between mouse and edge
> +        // the closer to the edge, the faster
> +        docEl.scrollTop -= speed - 15;

magic number here too.
Attachment #8547223 - Flags: review?(pbrosset)
Thanks Patrick. 

I applied your tips.

If you think this feature is complete, what's your idea about the first place indicator?
Attachment #8547223 - Attachment is obsolete: true
Attachment #8547527 - Flags: review?(pbrosset)
Comment on attachment 8547527 [details] [diff] [review]
Bug 858038 - Auto-scroll while dragging elements up / down, add a first place indicator for dragged element

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

These changes look good. Thanks!

I think there's still one problem though, since the view is only scrolled on mousemove this means users have to keep on moving the mouse as long as they want the view to scroll.
So if you start dragging, then move towards the bottom edge and then stop moving down, you'd still expect the view to scroll down continuously because you are hovering near the edge, but with the current solution it won't.

A solution could be starting an interval after we started to scroll, using the same speed, and stopping that interval when a new mousemove occurs. Kind of like this demo I just did: http://jsbin.com/beyuxaxexu/3/

> If you think this feature is complete, what's your idea about the first
> place indicator?
By first-place indicator, do you mean the position where the node was originally appended? If yes, then what about adding a new css class that does the same as the drop target? But with a different color maybe. Then we'd just have to add this class to the first hovered element. Does that make sense?
Attachment #8547527 - Flags: review?(pbrosset)
Thanks for your tips.

I fixed the auto-scroll bug.

About the initial place indicator, adding CSS class to the elements neighbours could be inaccurate, for example if an element which is the only child of it's parent was dragged away, the hovered element would be it's parent, which would be inaccurate to show (indentation level, etc). I just added an element when the dragging began, and removed it after dragging was done.

+ Are you ok with the color used for the initial place indicator?
Attachment #8547527 - Attachment is obsolete: true
Attachment #8548894 - Flags: review?(pbrosset)
There were some |if()| hanging around without an space, fixed them.
Attachment #8548895 - Flags: review?(pbrosset)
Comment on attachment 8548895 [details] [diff] [review]
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=pbrosset

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

Ok cool, I think we're ok with this part 1.
I'll take a look at the auto-scroll and first place indicator next.
Attachment #8548895 - Flags: review?(pbrosset) → review+
Comment on attachment 8548894 [details] [diff] [review]
Bug 858038 - Auto-scroll while dragging elements up / down, add a first place indicator for dragged element

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

The auto-scroll looks good to me! I've played with it, it looks like it's working fine.
About the initial target, I have some comments about a better solution. Let me know what you think.

::: browser/devtools/markupview/markup-view.js
@@ +173,5 @@
> +      if (this._scrollInterval) {
> +        this.win.clearInterval(this._scrollInterval);
> +      }
> +
> +      // Auto-scroll when the mouse approaches bottom edge

typo: "when the mouse approaches bottom and top edges"

@@ +197,5 @@
> +                        DRAG_DROP_MIN_AUTOSCROLL_SPEED, DRAG_DROP_MAX_AUTOSCROLL_SPEED);
> +
> +        this._scrollInterval = this.win.setInterval(() => {
> +          docEl.scrollTop += speed - DRAG_DROP_MAX_AUTOSCROLL_SPEED;
> +

nit: empty line to be removed.

@@ +285,5 @@
>      if (this._hoveredNode) {
>        this.getContainer(this._hoveredNode).hovered = false;
>      }
>      this._hoveredNode = null;
> +

nit: remove this empty line.

@@ +1757,5 @@
>        this.isDragging = true;
>  
>        this._dragStartY = event.pageY;
>        this.markup.indicateDropTarget(this.elt);
> +      var indicator = this.initialPlaceIndicator = this.markup.doc.createElement('div');

let instead of var.

@@ +1760,5 @@
>        this.markup.indicateDropTarget(this.elt);
> +      var indicator = this.initialPlaceIndicator = this.markup.doc.createElement('div');
> +      indicator.classList.add('initial-place-indicator');
> +
> +      this.elt.parentNode.insertBefore(indicator, this.elt);

I don't understand what you said about being inaccurate if you didn't inserted an element here.

I see 2 use cases (correct me if I'm wrong):
- either the dragged node has no next siblings: in this case, the close tag of its parent should be highlighted as being the initial indicator,
- or it has a next sibling and in that case, the next sibling should be highlighted.

I don't think there is any other case, and if so, it should be relatively easy to find the either the close tag-line of the parent container or the open tag-line of the nextsibling, and highlight these ones as initial target.

In fact, you could create a new function in MarkupView called indicateDragStartTarget, somewhat similar to indicateDropTarget, which would take an element as input and would add the class "drag-target" to this element. If the provided element is null, it would remove this class from the last element it added it to (so that you can call this.markup.inidicateDragStartTarget(null) in MarkupContainer._onMouseUp).

Then the CSS could look like:

.tag-line.drop-target::before,
.tag-line.drag-target::before {
  content: '';
  position: absolute;
  width: 100%;
  border-top: 2px dashed var(--theme-content-color1);
}

.tag-line.drag-target::before {
  border-top: 2px dashed var(--theme-highlight-red);
}

This way, if the dragged element is moved over its initial drag start target, there wouldn't 2 lines on top of eachother like your current solution.

Let me know your thoughts. I believe this would make the code simpler and the solution a bit better overall.
Attachment #8548894 - Flags: review?(pbrosset)
Thanks for your tips Patrick.

Yeah you were right, it was simpler than I thought, I actually thought of it another way, thanks for your explaination.

I added a case for when we're using the closing <div.tag-line> as indicator, to push the indicator to right to simulate the indentation level.

I also applied the same way of using to indicateDropTarget, so you can pass it null to simply remove the indicator.
Attachment #8548894 - Attachment is obsolete: true
Attachment #8550305 - Flags: review?(pbrosset)
Comment on attachment 8543918 [details] [diff] [review]
bug858038_move_indicate_elements_markupview.diff

Marking my old correction patch as obsolete now.
Attachment #8543918 - Attachment is obsolete: true
Comment on attachment 8550305 [details] [diff] [review]
Bug 858038 - Auto-scroll while dragging elements up / down, add a first place indicator for dragged element; r=pbrosset

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

Alright, that looks great. Just one more review I think.
My 2 main comments here:
- use the markup-view API where possible
- remove the --push rule, I don't think we need this extra indentation.

Thanks!

::: browser/devtools/markupview/markup-view.css
@@ +92,5 @@
> +.tag-line.drag-target::before {
> +  border-top: 2px dashed var(--theme-contrast-background);
> +}
> +
> +.tag-line.drag-target--push::before {

This rule can go away with my comment in markup-view.js

::: browser/devtools/markupview/markup-view.js
@@ +1538,5 @@
> +
> +  /**
> +   * Takes an element to mark it as indicator of dragging target's initial place,
> +   * the element might be a <li.child> or a closing <div.tag-line>, if it's the latter
> +   * it adds a class to push the indicator to right, to simulate indentation level of its children

I don't think we need to do this in fact.
When you move the dragged element over an element that has no child for example, the drop target indicator isn't indented:

<div>
-----------------
</div>

and I don't think that's a problem, it looks fine to me. 
So I would say, let's do the same for initial drag targets too: if there are no sibblings left in the parent of the dragged element, the indicator will look like the above diagram, aligned with the parent's open/close tags.

@@ +1548,5 @@
> +
> +    if (!el) return;
> +
> +    let closingTagLine = el.classList.contains("tag-line");
> +    let target = el.classList.contains("tag-line") ?

You initialized closingTagLine just above, why not use its value here instead of re-writing the same el.classList.contains("tag-line") ?

@@ +1553,5 @@
> +                 el : el.querySelector(".tag-line") || el.closest(".tag-line");
> +
> +    if (!target) return;
> +
> +    if (closingTagLine) target.classList.add("drag-target--push");

So this line can go away.

@@ +1786,5 @@
>        this._dragStartY = event.pageY;
>        this.markup.indicateDropTarget(this.elt);
> +
> +      // If this is the last child, use the closing <div.tag-line> of parent as indicator
> +      this.markup.indicateDragTarget(this.elt.nextElementSibling || this.elt.parentNode.nextElementSibling);

Rather than navigating the DOM of the markup-view, you should try and use the MarkupView and MarkupContainer object APIs instead.

For instance: |this.markup| will give you the MarkupView object. From there, you can access the convenient |getContainer(nodeFront)| method, which accepts a NodeFront as its argument.
Here, you want to access the parent container of the current container. So you can get the corresponding NodeFront like so: |let parent = this.node.parentNode()|.
This gives you the parent NodeFront, and then you can do: |this.markup.getContainer(parent)|

So, this gives you a MarkupContainer object, and you want to access its |closeTagLine| property.

So all in all:

this.elt.parentNode.nextElementSibling

can be written:

this.markup.getContainer(this.node.parentNode()).closeTagLine

This is longer, yes, but easier to understand for people working with the markup-view objects/APIs, and also, more robust because changing the DOM of the markup-view isn't going to break this in the future.

I wish we could do the same for |this.elt.nextElementSibling|, but there isn't an easy way other than actually sending another request to the server to ask for the nextSibling from there. So I'm ok keeping |this.elt.nextElementSibling| as is for now. Anyway, I intend to rewrite the markup-view model to make traversing much easier.
Attachment #8550305 - Flags: review?(pbrosset)
As discussed on IRC, I'm ok to leave the margin for the initial drag target in case it was an only child. But let's find another selector for this.
Also as discussed, you've duplicated the drop-target class in one of the selectors. This should be nicer:
http://pastebin.mozilla.org/8250408

And finally, ::before and ::after pseudo-elements displayed in the markup-view should not be draggab,e. You can check if this is the case in _onMouseDown by testing this.node.isPseudoElement.
/* After talking with Patrick on IRC we decided to find a new way to visualize the indentation on closing tags and kept it.
Also, Pseudo elements are now undraggable. */

Thanks Patrick, applied.

I added |event.stopPropagation()| because if the event propagated, it's parent would be dragged instead which caused confusion.
Attachment #8550305 - Attachment is obsolete: true
Attachment #8551222 - Flags: review?(pbrosset)
Comment on attachment 8551222 [details] [diff] [review]
Bug 858038 - Auto-scroll while dragging elements up / down, add a first place indicator for dragged element; r=pbrosset

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

Looks good to me now. Thanks.
Just one minor remark about adding a comment to the CSS file. No need to ask me for review again after this change though, you can mark the next patch as R+ yourself.

So, now that we have the basics in place, the next steps are:

- on drop, send the necessary request to append the node at the right place. I see 2 use cases: either appending the element at the end of the target's parent (if there are no other siblings or if the target is the last child), or insertBefore.
In both cases you can call |this.walker.insertBefore(aNode, parent, nextSibling);| knowing that nextSibling can be null, in which case the element is appended to the end.
Of course, you should first check if the drop target isn't the same as the initial drag target, if it is, no need to do anything.

- adding tests for the front-end code. Tests should verify that:
  - pseudo-elements aren't draggable
  - a delay is needed to start dragging elements
  - initial drag target and drop targets are indicated
  - the view scrolls automatically when nearing the edges
These tests may take some time to get right, I wouldn't say writing ui tests is the easiest thing in the world. But I will help you through them. The first to do is having a look at the tests that already exist in /browser/devtools/markupview/test/. That should give you an idea of how tests are written.

::: browser/devtools/markupview/markup-view.css
@@ +96,4 @@
>    border-top: 2px dashed var(--theme-content-color1);
>  }
>  
> +ul.children + .tag-line::before {

Could you just add a comment above this rule saying why it is necessary? It's not immediately obvious to the reader.
Attachment #8551222 - Flags: review?(pbrosset) → review+
Hey Mahdi, this is just a quick ping asking if you're still interested in continuing working on this bug. There's no rush (except that I'd hate the patches to bitrot), but do let me know.
Thanks.
Flags: needinfo?(mdibaiee)
Yes, I'm just a little busy these days, I just came here to review what I should do next and saw your comment. I'm going to finish this bug.

Thanks.
Flags: needinfo?(mdibaiee)
Hi Patrick,

The request is sent and the element is moved, I tried testing different edge cases to make sure it doesn't have bugs, I hope there's no mistake.

One thing: I was not sure if the |this._dragStartEl = event.target| line I removed was actually doing something, but it was causing a bug, I removed it but didn't notice any bug rising, do you know something about it? :sweat_drops:
Attachment #8557487 - Flags: review?(pbrosset)
Comment on attachment 8557487 [details] [diff] [review]
Bug 858038 - Request server to move element to desired position; r=pbrosset

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

Thanks for the patch! It works nicely, but there are some errors being thrown when using it. I'm not sure exactly what causes them, but before going into the details, I have one remark about this patch:
why not moving the logic to call insertBefore into the MarkupContainer's _onMouseUp function instead?
The MarkupView class has a _onMouseUp handler too because it's the one who knows which nodes have been indicated as drop or drag targets, so it's the one who removes the indicators when the mouse goes up.
However, the MarkupContainer class knows which underlying server-side DOM node is being moved, so I think it would take less code and be more logical if placed in this class instead.

See this version of MarkupContainer._onMouseUp:

  _onMouseUp: function(event) {
    if (!this.isDragging) {
      return;
    }

    this._isMouseDown = false;
    this.isDragging = false;
    this.elt.style.removeProperty("top");

    let target = this.markup._lastDropTarget;
    let nextSibling = null;
    let parent = null;
    if (target.previousElementSibling &&
        target.previousElementSibling.nodeName.toLowerCase() === "ul") {
      // Appending at the end of the child list in parent
      parent = target.parentNode.container.node;
    } else {
      // Appending before target.parentNode
      nextSibling = target.parentNode.container.node;
      parent = nextSibling.parentNode();
    }

    this.markup.walker.insertBefore(this.node, parent, nextSibling).then(null, console.error);
  },

I've added a check for this.isDragging to early return, otherwise all MarkupContainer will react to the event.
I think there are 2 cases: either the node is inserted as part of a child node list, or before the closing tag of a parent.
If the drop target is a closing tag, it means it may have a previousElementSibling which is the <ul> list containing all childNodes of the parent.

I've thrown this together quickly, so there might be cases I haven't thought about, but moving the code there makes it simpler I think.
Also, remember that the elements in the markup panel that correspond to a MarkupContainer instance have the .container property set, so you can access it.
For example, consider 'el' being an element in the markup-view that corresponds to a MarkupContainer:
- el.container -> gives you the MarkupContainer isntance
- el.container.node -> gives you the NodeFront instance for the MarkupContainer (the thing that gets sent to the server)
- el.container.node.parentNode() -> gives you the NodeFront that is the parentNode of el.container.node

In fact, if we go this way, I would like to suggest something else that would simplify onMouseUp even further: let's add a getter on MarkupView that looks at this._lastDropTarget and based on it, returns the corresponding MarkupContainer:

get dropTargetNodes() {
  if (!this._lastDropTarget) {
    return null;
  }

  let dropTarget = {};
  
  if (this._lastDropTarget.previousElementSibling &&
      this._lastDropTarget.previousElementSibling.nodeName.toLowerCase() === "ul") {
    dropTarget.parent = target.parentNode.container.node;
  } else {
    dropTarget.parent = target.parentNode.container.node.parentNode();
    dropTarget.nextSibling = target.parentNode.container.node;
  }

  return dropTarget;
}

Or something like this. With this, the MarkupContainer's _onMouseUp handler would be simplified to:

  let {parent, nextSibling} = this.markup.dropTargetNodes;
  this.markup.walker.insertBefore(this.node, parent, nextSibling).then(null, console.error);

::: browser/devtools/markupview/markup-view.js
@@ -1811,5 @@
>     * On mouse move, move the dragged element if any and indicate the drop target.
>     */
>    _onMouseMove: function(event) {
>      if (!this.isDragging) {
> -      this._dragStartEl = event.target;

Agreed, this line was useless.
Attachment #8557487 - Flags: review?(pbrosset)
Hi, Thanks Patrick!

You were right, the code became simpler as I didn't have to store |_dragStartEl| in the MarkupView and I could easily use |this.elt|, too.

We talked on IRC and agreed to check for conditions which "HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy" would happen and we avoided it by checking the drop target and not allowing it to be documentElement or anything outside it.
Attachment #8557487 - Attachment is obsolete: true
Attachment #8557927 - Flags: review?(pbrosset)
This patch uses NodeFront's nodeType instead of comparing ownerDocument and parentNode.
Attachment #8557927 - Attachment is obsolete: true
Attachment #8557934 - Flags: review?(pbrosset)
Attachment #8557927 - Flags: review?(pbrosset)
Comment on attachment 8557934 [details] [diff] [review]
Bug 858038 - Request server to move element to desired position; r=pbrosset

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

::: browser/devtools/markupview/markup-view.js
@@ +1559,5 @@
>      this._lastDragTarget = target;
> +  },
> +
> +  /**
> +   * Used to get the nodes required to modify the markup after dragging the element (parent/nextSibling)

Please cut lines after 80 chars.

@@ +1827,5 @@
>     */
>    _onMouseUp: function(event) {
>      this._isMouseDown = false;
> +
> +    let lastDropTarget = this.markup._lastDropTarget;

You're accessing a private property of the markupView here. One of the reasons why I suggested adding the dropTargetNodes getter on MarkupView was to avoid this.
You should use the getter here and use the return value in the if below.

@@ +1838,5 @@
> +    let dropTarget = lastDropTarget.parentNode;
> +    let draggedNode = this.elt;
> +
> +    // make sure the drop target is not documentElement or outside of it
> +    if (dropTarget.container.node.parentNode().nodeType === Ci.nsIDOMNode.DOCUMENT_NODE) return;

I'd suggest moving this check in the getter too.

@@ +1841,5 @@
> +    // make sure the drop target is not documentElement or outside of it
> +    if (dropTarget.container.node.parentNode().nodeType === Ci.nsIDOMNode.DOCUMENT_NODE) return;
> +
> +    // Used for conditions in which drop target is a closing .tag-line
> +    let closingTagLine = lastDropTarget.previousElementSibling;

I still don't understand the logic here. The if below is pretty complex, lot's of conditions and ! makes it really hard to read.
I tried locally removing it and didn't notice anything wrong.
I think the if in the getter takes care of all possible cases, no?

I'll attach a patch in a second that simplifies this so you can test locally and let me know what I missed.
Attachment #8557934 - Flags: review?(pbrosset)
Please apply this one on top of your 3 patches and let me know if you see anything missing.
Hi, you said we have to check whether the element has moved since dragging or not, that's what this if is doing, the patch you sent is running insertBefore in all cases, no matter if the element was moved or not.

I'm not sure how does insertBefore handle this situation, does it really re-insert, does it consume any resource or not, if you think it's not required, the code can be simplified of course.
Flags: needinfo?(pbrosset)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #68)
> Hi, you said we have to check whether the element has moved since dragging
> or not, that's what this if is doing, the patch you sent is running
> insertBefore in all cases, no matter if the element was moved or not.
Ok, that wasn't immediately obvious to me. If we want to keep this check, we need to make it more easily readable in any case but...
> I'm not sure how does insertBefore handle this situation, does it really
> re-insert, does it consume any resource or not, if you think it's not
> required, the code can be simplified of course.
It looks like using insertBefore to re-insert a node at the same position does cause 2 markup mutations: the node if first removed from the parent and then re-inserted at the same position.
So it does cause a tiny bit of traffic, but considering the amount of code this removes from the client-side, I'd rather not check for this case.
Flags: needinfo?(pbrosset)
Sorry for the late response.

If you think it's not necessary to check for changes, then we can merge the patches. Is there a way to merge hg patches? I searched and found hg push -m, just want to make sure it doesn't break anything.

Thanks.
Flags: needinfo?(pbrosset)

Comment 71

4 years ago
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #70)
> Sorry for the late response.
> 
> If you think it's not necessary to check for changes, then we can merge the
> patches. Is there a way to merge hg patches? I searched and found hg push
> -m, just want to make sure it doesn't break anything.
> 
> Thanks.

http://codefirefox.com/video/move-fold-patch
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #70)
> If you think it's not necessary to check for changes, then we can merge the
> patches. Is there a way to merge hg patches? I searched and found hg push
> -m, just want to make sure it doesn't break anything.
As Tim said, you can fold the 2 patches together.
Let's say your patch queue is like so:

0 A Allow moving of elements in markupview and indicate their destination
1 A Auto-scroll while dragging elements up / down, add a first place indicator for dragged element
2 A Request server to move element to desired position
3   bug858038-simplify-insertBefore-logic.patch

(your 3 first patches applied, mine next, but unapplied).
Then run: 
hg qfold 3
Flags: needinfo?(pbrosset)
Thanks for the video Tim, that helped a lot.

I folded the patches together, if you think the patch is ready, can we move onto the ui tests? I'll take a look at other tests when as soon as I get time to.

Thanks
Attachment #8557934 - Attachment is obsolete: true
Attachment #8562068 - Flags: review?(pbrosset)
Comment on attachment 8562068 [details] [diff] [review]
bug858038_request_element_change.diff

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

Yep, that looks good to me. Let's move on to ui testing!

::: browser/devtools/markupview/markup-view.js
@@ +1843,5 @@
> +    if (!dropTargetNodes) {
> +      return;
> +    }
> +
> +    //   let {parent, nextSibling} = this.markup.dropTargetNodes;

Please remove this commented line.
Attachment #8562068 - Flags: review?(pbrosset) → review+

Updated

4 years ago
Attachment #8558411 - Attachment is obsolete: true
While writing tests I found out trying to insert before pseudo-elements causes the element to be destroyed, now inserting before ::before, actually prepends to element and inserting before ::after appends to the element.
Attachment #8562068 - Attachment is obsolete: true
Attachment #8562826 - Flags: review?(pbrosset)
UI Tests

1) Test the delay before element is in dragging mode (GRAB_DELAY = 400)
2) Test pseudo-elements draggability, they should not ever become draggable
3) Test different re-orderings, to last child, after parent, append, etc.

I will add any test you think that's missing.

Thanks.
Attachment #8562829 - Flags: review?(pbrosset)
While testing the patches, I found out another use case we will need to prevent: anonymous elements.
It turns out that in the browser toolbox, we show native anonymous elements too (things inside of <input /> for instance), and these shouldn't be movable outside of the non native anonymous parent.
So, an anonymous div inside an input shouldn't be dragged outside of the input, while it should be possible to re-arrange things inside the input.
Handling this is complex and definitely could be done as a v2 of the feature, so in the meantime, can you add a check for |this.node.isAnonymous| in |MarkupContainer._onMouseDown| (just like the other |this.node.isPseudoElement| check you have).
Right, thanks for pointing it out.

Did you run / review tests yet? I'm expecting some tests are missing, but can't figure out what's missing.

Thanks Patrick.
Attachment #8562826 - Attachment is obsolete: true
Attachment #8562826 - Flags: review?(pbrosset)
Flags: needinfo?(pbrosset)
Attachment #8564935 - Flags: review+
I found 2 regressions caused by the new drag/drop feature:

- the markup-view text selection feature does not work anymore. It is not possible anymore to select text (for copy/pasting) because when you start dragging your cursor to select text, that starts moving the node after the delay. One simple test you can make to detect whether the user has started selecting text in the setTimeout: |this.markup.win.getSelection().isCollapsed|. If this is true, then there is no active selections.

- it's not possible anymore to click to select ::before and ::after nodes. The place where you've added the check for |this.node.isPseudoElement| is too soon in the |_onMouseDown| callback.
This version of the callback should fix this + add the check for isAnonymous described in my previous comment and make the text selection work again:

>   _onMouseDown: function(event) {
>     let target = event.target;
> 
>     // The "show more nodes" button (already has its onclick).
>     if (target.nodeName === "button") {
>       return;
>     }
> 
>     // output-parser generated links handling.
>     if (target.nodeName === "a") {
>       event.stopPropagation();
>       event.preventDefault();
>       let browserWin = this.markup._inspector.target
>                            .tab.ownerDocument.defaultView;
>       browserWin.openUILinkIn(target.href, "tab");
>       return;
>     }
> 
> 
>     // target is the MarkupContainer itself.
>     this._isMouseDown = true;
>     this.hovered = false;
>     this.markup.navigate(this);
>     event.stopPropagation();
> 
>     // Start dragging the container after a delay.
>     this._dragStartEl = target;
>     this.win.setTimeout(() => {
>       // Make sure the mouse is still down and on target.
>       if (this.node.isPseudoElement ||
>           this.node.isAnonymous ||
>           !this._isMouseDown ||
>           this._dragStartEl !== target ||
>           !this.markup.win.getSelection().isCollapsed) {
>         return;
>       }
>       this.isDragging = true;
> 
>       this._dragStartY = event.pageY;
>       this.markup.indicateDropTarget(this.elt);
> 
>       // If this is the last child, use the closing <div.tag-line> of parent as indicator
>       this.markup.indicateDragTarget(this.elt.nextElementSibling ||
>                                      this.markup.getContainer(this.node.parentNode()).closeTagLine);
>     }, GRAB_DELAY);
>   },
Flags: needinfo?(pbrosset)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #78)
> Created attachment 8564935 [details] [diff] [review]
> Bug 858038 - Request server to move element to desired position; r=pbrosset
> 
> Right, thanks for pointing it out.
> 
> Did you run / review tests yet? I'm expecting some tests are missing, but
> can't figure out what's missing.
> 
> Thanks Patrick.
No I haven't done the review yet.
Comment on attachment 8564935 [details] [diff] [review]
Bug 858038 - Request server to move element to desired position; r=pbrosset

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

I have some minor remarks that I'd like to see addressed. Apart from those, this looks good.

::: browser/devtools/markupview/markup-view.js
@@ +1559,5 @@
>      this._lastDragTarget = target;
> +  },
> +
> +  /**
> +   * Used to get the nodes required to modify the markup after dragging the element (parent/nextSibling)

nit: please wrap lines after 80 characters.
Also, rephrase to:
Used to get the drop target parent and sibling nodes for the currently dragged node.

@@ +1568,5 @@
> +    if (!target) {
> +      return null;
> +    }
> +
> +    let dropTarget = {parent: null, nextSibling: null};

Let's change this to:

let parent;
let nextSibling;

This will make it possible in the next few lines to remove |dropTarget.|, and therefore make the code easier to read with shorter lines.

@@ +1591,5 @@
> +    if (dropTarget.parent.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE) {
> +      return null;
> +    }
> +
> +    return dropTarget;

And because we got rid of the dropTarget object, here you'll need to return:

return {parent, nextSibling};

@@ +1803,5 @@
>        browserWin.openUILinkIn(target.href, "tab");
>        return;
>      }
>  
> +    if (this.node.isPseudoElement || this.node.isAnonymous) {

It's a little weird that you've fixed this in this patch. This should be part of the first patch, and this patch should only be kept for sending the request to the server.
It's not a big deal, don't change anything now, I suggest we just fold all the patches into one at the end.
I found another problem, with the 2nd patch this time:
https://bugzilla.mozilla.org/attachment.cgi?id=8551222&action=diff#a/browser/devtools/markupview/markup-view.js_sec7
Right here, you nullify this.doc and this.win, which is good, but that breaks the MarkupContainer destroy sequence because it relies on it to exist. You have this in MarkupContainer.destroy:

    this.markup.doc.body.removeEventListener("mouseup", this._onMouseUp, true);
    this.markup.doc.body.removeEventListener("mousemove", this._onMouseMove, true);

but by the time this runs, this.markup.doc is null.
Can you move the 2 nullifying lines in MarkupView destroy from where they are to after the |container.destroy()| for loop?
Comment on attachment 8562829 [details] [diff] [review]
Bug 858038 - UI Tests; r=pbrosset

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

I like these tests. I have made a few comments, mostly minor stuff.
I think you need to add a few tests.

::: browser/devtools/markupview/test/browser.ini
@@ +38,5 @@
>  [browser_markupview_anonymous_04.js]
>  [browser_markupview_copy_image_data.js]
>  [browser_markupview_css_completion_style_attribute.js]
> +[browser_markupview_dragdrop_isDragging.js]
> +[browser_markupview_dragdrop_pseudoElements.js]

We need a test for anonymous nodes too, but since is so similar to how we deal with pseudoElements, just rename this test to something like:
browser_markupview_dragdrop_invalidNodes.js
and make it test both cases.
To make a test with anonymous nodes, you need your test to first switch the pref "devtools.inspector.showAllAnonymousContent" to true, and then add an <input /> to your test page, then just expand to a node inside the input and try to drag it.

Services.prefs.setBoolPref("devtools.inspector.showAllAnonymousContent", true);

@@ +39,5 @@
>  [browser_markupview_copy_image_data.js]
>  [browser_markupview_css_completion_style_attribute.js]
> +[browser_markupview_dragdrop_isDragging.js]
> +[browser_markupview_dragdrop_pseudoElements.js]
> +[browser_markupview_dragdrop_reorder.js]

We also need a new test for text selection. This test should simulate mousedown, move and up and verify that a text selection exists in the window and that no nodes were moved instead.
Similarly, we should have the opposite test too: try to mousedown, wait, mousemove, mouseup and verify that no text selection exists and that a node was moved.

And I think we also need a test for auto-scrolling. This might be a bit harder to put in place though. I suggest adding a test with a very small toolbox height, then loading a page with lot's of elements, so that you're sure that there will be a scrollbar, on all test machines. Then make sure scrollTop is 0, then drag a node and simulate a mouseMove near the bottom edge, wait for a small delay, and simply check if scrollTop isn't 0 anymore after the delay.

::: browser/devtools/markupview/test/browser_markupview_dragdrop_isDragging.js
@@ +18,5 @@
> +
> +  let el = yield getContainerForSelector("#test", inspector);
> +  let rect = el.tagLine.getBoundingClientRect();
> +
> +  el._onMouseDown({

Can you add an |info("Simulating a mousedown on the markup container")| above this line?
As a general rule, try to annotate all the major steps of your test cases with info() statements, this helps a lot when reading test run logs.

@@ +41,5 @@
> +      pageY: rect.y
> +    });
> +
> +    is(el.isDragging, false, "isDragging false after mouseUp");
> +  }, GRAB_DELAY);

GRAB_DELAY doesn't work for me locally, the test fails.
It does work with GRAB_DELAY + 1 though.

I'm a little hesitant with this setTimeout usage, it's usually a good source for unstable tests, but since everything is happening in the same process here (the markup-view setTimeout and these 2 tests setTimeouts), I guess this should be fine. We'll make sure to test this many times on TRY to see if that works as expected.

@@ +43,5 @@
> +
> +    is(el.isDragging, false, "isDragging false after mouseUp");
> +  }, GRAB_DELAY);
> +
> +  yield onMutated;

Can you also add |yield inspector.once("inspector-updated");| after this line? This will make sure to wait until after the markup-view has updated its UI with the new nodes before finishing the test.

::: browser/devtools/markupview/test/browser_markupview_dragdrop_pseudoElements.js
@@ +22,5 @@
> +
> +  yield inspector.markup.expandNode(parentFront);
> +  yield waitForMultipleChildrenUpdates(inspector);
> +
> +  inspector.markup.win.scrollTo(0, 500);

I don't think this is required. And when the test run on different systems, the font-size or window width may be different, which may mean that the node won't be scrolled to with 500px anyway. So let's get rid of it.

@@ +35,5 @@
> +    setTimeout(() => {
> +      is(beforePseudo.isDragging, false, "isDragging is false after GRAB_DELAY has passed");
> +      resolve();
> +    }, GRAB_DELAY);
> +  })

You can replace the whole promise block with:

yield wait(GRAB_DELAY + 1);
is(beforePseudo.isDragging, false, "isDragging is false after GRAB_DELAY has passed");

@@ +40,5 @@
> +});
> +
> +// The expand all operation of the markup-view calls itself recursively and
> +// there's not one event we can wait for to know when it's done
> +function* waitForMultipleChildrenUpdates(inspector) {

It looks like this function is now duplicated in 4 different test files.
Can you please move it to the head.js file in the same folder, and remove it everywhere else it's defined.

::: browser/devtools/markupview/test/browser_markupview_dragdrop_reorder.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test different kinds of re-ordering

nit: rephrase: Test different kinds of drag and drop node re-ordering.

@@ +8,5 @@
> +
> +const TEST_URL = TEST_URL_ROOT + "doc_markup_dragdrop.html";
> +const GRAB_DELAY = 400;
> +
> +let dragContainer = Task.async(function*(selector, targetOffset, inspector) {

This function is used from a generator function already, with yield, so you don't need to wrap it into a Task.async again.
function* dragContainer(selector, targetOffset, inspector) {...}
would work just as well.

@@ +11,5 @@
> +
> +let dragContainer = Task.async(function*(selector, targetOffset, inspector) {
> +  info("Dragging the markup-container for node " + selector);
> +
> +  // let nodeFront = yield getNodeFront(selector, inspector);

Remove this unused line.

@@ +20,5 @@
> +  let rect = {
> +    x: container.tagLine.offsetLeft,
> +    y: container.tagLine.offsetTop
> +  };
> +  info("rect.x: " + rect.x + " rect.y: " + rect.y)

Remove this info or provide some text in it to help when reading test logs.

@@ +48,5 @@
> +
> +  return updated;
> +});
> +
> +let moveElementDown = Task.async(function*(selector, next, inspector) {

This function is used from a generator function already, with yield, so you don't need to wrap it into a Task.async again.
function* moveElementDown(selector, next, inspector) {...}
would work just as well.

@@ +61,5 @@
> +
> +  yield onMutated;
> +});
> +
> +add_task(function*() {

Please have the add_task always at the top of the file after the test description comment and the const, this makes it easier to read the test.

@@ +71,5 @@
> +
> +  yield inspector.markup.expandNode(parentFront);
> +  yield waitForMultipleChildrenUpdates(inspector);
> +
> +  inspector.markup.win.scrollTo(0, 500);

This isn't needed.

@@ +100,5 @@
> +});
> +
> +// The expand all operation of the markup-view calls itself recursively and
> +// there's not one event we can wait for to know when it's done
> +function* waitForMultipleChildrenUpdates(inspector) {

Remove this, since it should be moved to head.js
Attachment #8562829 - Flags: review?(pbrosset)
I've moved the right codes to their right patch. I hope I haven't messed up with anything.
Attachment #8548895 - Attachment is obsolete: true
Attachment #8565139 - Flags: review+
I applied your tips to the tests and made a test for autoscroll, but I didn't make textSelection test yet.

I wanted to ask how should I implement it? What API fits this well?

Thanks Patrick.
Attachment #8562829 - Attachment is obsolete: true
Attachment #8565144 - Flags: review?(pbrosset)

Comment 88

4 years ago
I originally requested this feature. It's great to see you are working on it. I wonder if it's possible to enable multiple elements selection with this? It would be nice if I could hold Shift down and select a bunch of elements and then move them around. 

Chrome DevTools doesn't have multiple selection either. I talked to a Chrome DevTools engineer about it and he said it's very tricky because they weren't sure what should happen to $0 in console when you select multiple elements.

What's the possibility of multiple selection in Firefox?
Implementing the drag & drop feature for multiple elements shouldn't be really hard, but I'm not sure how it would work with other tools.
Flags: needinfo?(pbrosset)
Enabling multi-selection within the Inspector is a separate feature and should be tracked independently of this bug. So I suggest to create a new report for that.

Sebastian
(In reply to msnazi from comment #88)
> I originally requested this feature. It's great to see you are working on
> it. I wonder if it's possible to enable multiple elements selection with
> this? It would be nice if I could hold Shift down and select a bunch of
> elements and then move them around. 
> 
> Chrome DevTools doesn't have multiple selection either. I talked to a Chrome
> DevTools engineer about it and he said it's very tricky because they weren't
> sure what should happen to $0 in console when you select multiple elements.
> 
> What's the possibility of multiple selection in Firefox?
I agree with Sebastian, this is better tracked in a separate bug, because this has consequences on other tools in the inspector:
- How do you highlight the selected nodes? All of them at the same time?
- What do you show in the rule-view, computed-view, font-view, layout-view and animation inspector?
- What about the breadcrumbs?
- $0?
Basically, everything in the devtools that interacts in any way with the inspector assumes that only one element is selected.
So, it could be that some webdev problems are better solved when more than 1 element is selected, I'm all for it, but I'm a little concerned about the user experience in the tools when that happens.
Should we disable all tools that need the single-selection assumption to hold true as soon as more than one node is selected?
Another option is to still consider just one node as selected and have a list of additional selected nodes that some features/tools would use.
Anyway, please file a new bug (I was sure we had one already, but can't find it) with a specific use case that this would solve.
Flags: needinfo?(pbrosset)
Comment on attachment 8565144 [details] [diff] [review]
Bug 858038 - UI Tests; r=pbrosset

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

Looking good. Just a few more changes and it should be ok to R+.
About the text selection test: you'll need to create a text selection programmatically, and then try to start dragging a node. To create a text selection, something like this should work:
let r = document.createRange();
r.selectNode(<the node to be selected>);
document.getSelection().addRange(r);

::: browser/devtools/markupview/test/browser_markupview_dragdrop_autoscroll.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test different kinds of drag and drop node re-ordering

This comment is incorrect: the test is about auto-scrolling, not re-ordering.

@@ +25,5 @@
> +    stopPropagation: function() {},
> +    preventDefault: function() {}
> +  });
> +
> +  yield wait(GRAB_DELAY + 1);

Testing this on TRY will tell us whether it's ok, but I'm a bit scared about using so many yield wait in these tests.
If we see that it's a source of unstability, we can easily switch to events instead (by making MarkupContainer emit events when it starts/stops dragging).
So let's keep it for now, we'll see later.

@@ +39,5 @@
> +  };
> +
> +  markup._onMouseMove(ev);
> +
> +  yield wait(500);

It's recommended to use events rather than timeouts everywhere we can. Here is a place we can: you can start to listen to scroll events on the markup-view before simulating the mousemove, and after just check if a scroll event was emitted. No need to check for scrollY either, the fact that the event happened is probably enough.

::: browser/devtools/markupview/test/browser_markupview_dragdrop_invalidNodes.js
@@ +56,5 @@
> +  yield wait(GRAB_DELAY + 1);
> +  is(anonymousDiv.isDragging, false, "[anonymous element] isDragging is false after GRAB_DELAY has passed");
> +});
> +
> +

nit: just one empty line at the end of a file is enough.

::: browser/devtools/markupview/test/doc_markup_dragdrop_autoscroll.html
@@ +13,5 @@
> +  <p id="display">Test</p>
> +  <div id="content" style="display: none">
> +
> +  </div>
> +

Can you add a comment node here to explain why there are so many divs here:
<!-- Make sure the markup-view has enough nodes shown by default that it has a scrollbar -->

::: browser/devtools/markupview/test/head.js
@@ +562,5 @@
>      editMode ? attrName + " is in edit mode" : attrName + " is not in edit mode");
>  }
> +
> +// The expand all operation of the markup-view calls itself recursively and
> +// there's not one event we can wait for to know when it's done

This comment looks like it's not complete.
I think you should continue the sentence with:
... so use this helper function to wait until all recursive children updates are done.
Attachment #8565144 - Flags: review?(pbrosset)
Thanks Patrick.

I applied your tips, made the textSelection test, it basically selects a text then tries triggering drag and drop and checks whether isDragging is true or not after the GRAB_DELAY.

I thought of making MarkupContainer events return promises to make sure the delays do not break, I'll fix it if it's required.
Attachment #8565144 - Attachment is obsolete: true
Attachment #8565965 - Flags: review?(pbrosset)
Comment on attachment 8565965 [details] [diff] [review]
Bug 858038 - UI Tests; r=pbrosset

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

All right, thanks for making these changes.
Can you make sure the tests still pass, at least locally?
I suggest running the following ones:
./mach mochitest-devtools browser/devtools/inspector
./mach mochitest-devtools browser/devtools/markupview
and maybe these ones just in case (they are not immediately related, but it's better to know if something fails locally before pushing to try, and these tests aren't long to run):
./mach mochitest-devtools browser/devtools/styleinspector

Once done, let me know, I'll push the patches to our TRY server, so that all the tests are run on a variety of OSs and builds.
Attachment #8565965 - Flags: review?(pbrosset) → review+
I ran the tests, there's one failure and that's this:

> 3031 INFO TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_destroy-before-ready.js | A promise chain failed to handle a rejection:  - at resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/markupview/markup-view.js:1739 - TypeError: this.markup.doc is null
> Stack trace:
>    MarkupContainer.prototype.expanded@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/markupview/markup-view.js:1739:15
>    MarkupView.prototype._expandContainer/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/markupview/markup-view.js:859:7
>    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
>    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
>    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
>    testScope/test_finish/<@chrome://mochikit/content/browser-test.js:979:11
>     testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:881:9

It seems to be related with putting |this.doc = null| in MarkupView's destroy method.

I added a condition which checks markup.doc's availibility before continuing.

Apart from this, all the tests passed.

Thanks Patrick.
Attachment #8565965 - Attachment is obsolete: true
Attachment #8566504 - Flags: review?(pbrosset)
This minor change to part 1 avoids the test failure in browser_inspector_destroy-before-ready.js
You were nullifying doc and win in destroy, which is good, but that caused some of the MarkupContainer async operations to fail.
To avoid this, I'm just checking, after the async call returns, if the markup-view still exists or not. If it's been destroyed in the meantime, I just log a warning and early return.
Attachment #8565139 - Attachment is obsolete: true
Attachment #8567836 - Flags: review+
Comment on attachment 8566504 [details] [diff] [review]
Bug 858038 - UI Tests; r=pbrosset

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

::: browser/devtools/markupview/markup-view.js
@@ +1725,5 @@
>      return !this.elt.classList.contains("collapsed");
>    },
>  
>    set expanded(aValue) {
> +    if (!this.expander || !this.markup.doc) {

This becomes useless with the new part 1 patch I just attached.
It's better to avoid trying to expand the node in the first place, if the markup-view has been destroyed, rather than adding these kinds of checks all over the place.

Can you remove this and run the tests once more to make sure my fix does work?
Attachment #8566504 - Flags: review?(pbrosset)
Thanks Patrick, your patch worked, all the tests passed.
Attachment #8565140 - Attachment is obsolete: true
Attachment #8567870 - Flags: review+
Attachment #8566504 - Attachment is obsolete: true
Okay, now what's left? Should I merge the patches together and add an attachment?
Flags: needinfo?(pbrosset)
Comment on attachment 8567871 [details] [diff] [review]
Bug 858038 - UI Tests; r=pbrosset

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

LGTM now.
Attachment #8567871 - Flags: review+
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #100)
> Okay, now what's left? Should I merge the patches together and add an
> attachment?
Yes please do fold all the patches together and upload the new one, marking all others obsolete, and flagging it R+.
I'll push to try to test that all tests pass fine on all builds/platforms.
Flags: needinfo?(pbrosset)
Hmmm, the auto-scroll patch seems to be missing now. We have twice the same patch now instead.
Can you clean this up a bit?
Release Note Request (optional, but appreciated)
[Why is this notable]: Cool new feature in the inspector frequently requested
[Suggested wording]: Inspector : Nodes in markup view can now be dragged and dropped
[Links (documentation, blog post, etc)]: Should appear on hacks.mo.org or on MDN once it reaches aurora
relnote-firefox: --- → ?
Attaching latest patches so we have an archive of them. (accidentaly had obsoleted the auto-scroll patch instead)
Attachment #8565141 - Attachment is obsolete: true
Attachment #8567870 - Attachment is obsolete: true
Attachment #8569086 - Flags: review+
Attachment #8567871 - Attachment is obsolete: true
Attachment #8569089 - Flags: review+
Folded patches together.

Patrick, may you please mark your patch as obsolete as well? Thanks.
Attachment #8569086 - Attachment is obsolete: true
Attachment #8569087 - Attachment is obsolete: true
Attachment #8569088 - Attachment is obsolete: true
Attachment #8569089 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8569093 - Flags: review+
Attachment #8567836 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Just one last detail, can you clean up the commit message?
It's currently:
Bug 858038 - Allow moving of elements in markupview and indicate their destination; r=pbrosset
* * *
Bug 858038 - Auto-scroll while dragging elements up / down, add a first place indicator for dragged element; r=pbrosset
* * *
Bug 858038 - Request server to move element to desired position; r=pbrosset
* * *
Bug 858038 - UI Tests; r=pbrosset

You can change this with something like:
hg qref -m "Bug 858038 - Allow moving elements in the markupview by drag/drop; r=pbrosset"

Here's a pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e01925c0ab
done, thanks Patrick.
Attachment #8569093 - Attachment is obsolete: true
Attachment #8569100 - Flags: review+
Looks like browser_markupview_dragdrop_isDragging.js is constantly failing on TRY. The good news is, no other markup-view tests seem to be failing :)
Can you take a look at the failure logs at https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e01925c0ab and propose a fix? Let me know if you need help with that.
There was a check in the middle of a setTimeout, which I talked about with Patrick and we thought it wasn't necessary and got rid of it (they usually fail on different machines).
Attachment #8569100 - Attachment is obsolete: true
Attachment #8569137 - Flags: review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #114)
> New try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8ee5f04158
So, apart from the seemingly unrelated xpcshell test failures, the dt tests all seem to be green so far, enough to try and land this in fx-team.
Good job Mahdi!
Keywords: checkin-needed
Yup! :satisfied:

Thank you Patrick for mentoring me on this bug, you really helped a lot, I really liked working on this bug, it was a fun experience and I learned a lot.

I'll probably pick up another bug soon.

I'm making my way to a Mozilla Internship, I guess.

Should I mark the bug as RESOLVED?
Flags: needinfo?(pbrosset)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #116)
> Should I mark the bug as RESOLVED?

Sheriffs will do this automatically once it reaches mozilla-central. Then the next day, it'll appear on Nightly. Thanks for working on this :)
Flags: needinfo?(pbrosset)
(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #116)
> Thank you Patrick for mentoring me on this bug, you really helped a lot, I
> really liked working on this bug, it was a fun experience and I learned a
> lot.
No problem, glad I could help you learn.
> I'll probably pick up another bug soon.
That'd be awesome, what about the following bugs:
- bug 984442
- bug 1128974
- bug 1055181
You can find bugs here too: http://www.joshmatthews.net/bugsahoy/
> That'd be awesome, what about the following bugs:
> - bug 984442
> - bug 1128974
> - bug 1055181
> You can find bugs here too: http://www.joshmatthews.net/bugsahoy/

Oh, those are some interesting bugs, I'll start working on them.

Bugsahoy is awesome, I used it before and I like it.
https://hg.mozilla.org/mozilla-central/rev/08e62ceb672b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Tim, do you have a post or something on MDN to link to for the release notes?
Flags: needinfo?(ntim.bugs)
Relnoted for 39 as: Drag and drop enabled for nodes in Inspector markup view
(In reply to Liz Henry (:lizzard) from comment #122)
> Tim, do you have a post or something on MDN to link to for the release notes?

I'm not sure. Patrick might know.
Flags: needinfo?(ntim.bugs) → needinfo?(pbrosset)
(In reply to Liz Henry (:lizzard) from comment #122)
> Tim, do you have a post or something on MDN to link to for the release notes?
I just updated this page on MDN and added a bit of documentation on this new feature: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Editing_HTML_2
Flags: needinfo?(pbrosset)
Depends on: 1198039
Depends on: 1198040
Depends on: 1198042
Depends on: 1176785

Updated

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