Closed Bug 1195120 Opened 9 years ago Closed 9 years ago

Drag and drop mode in inspector shows indicator before opening tag when mouse is placed to the left from closing tag

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 affected, firefox46 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox46 --- fixed

People

(Reporter: arni2033, Assigned: nchevobbe, Mentored)

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(3 files, 3 obsolete files)

Drag and drop mode in inspector should respect mouse position.webm
276.04 KB, video/webm
Details
screenshot 1 (explanation) - dragging element is dropped _before_ hovered element.png
20.38 KB, image/png
Details
Bug_1195120.patch
1.54 KB, patch
pbro
: review+
Details | Diff | Splinter Review
STR: (Nightly 43.0a1 (2015-08-15)) 1. Open page data:text/html,<div><b> 2. Open Inspector, start dragging <b> element 3. Move mouse to the left border of markup-view 4. Move mouse between lines </div> and </body> (don't forget to keep mouse close to the left border of markup-view) 5. Release mouse Result: [watch video] <b> element was moved before <div> element within the <body> Expectations: <b> element should've been moved after <div> element within the <body>
Mentor: pbrosset
Depends on: 1197276
An easier way to reproduce this bug is to dock the devTools window to the right first. Then open-up the inspector panel and start dragging a node. While dragging, move the mouse to the 'content' window, in order to get the cursor out of the inspector panel. Indeed, the dragged node get stuck where the cursor went out. The problem seems to be in devtools/client/markupview/markup-view.js . A 'mousemove' listener is created on the document body of the MarkupContainer, in the initialize function. That's why the function is not triggered when the cursor goes out of the markup view. I don't really know if this should be fixed, as it is quite a common pattern in User Interfaces. For example, in the Firefox bookmark window, if you drag a bookmark and move the cursor out of the bookmark list, the "move" indicator does not follow the cursor position until it's back to the bookmark list ( similar behavior here : http://rubaxa.github.io/Sortable/ ).
Flags: needinfo?(pbrosset)
> move the mouse to the 'content' window, in order to get the cursor out of the inspector panel Nicolas, that's definitely not the case. I just had no time to describe this better. Improved STR: 1. Open page data:text/html,<div><b></b><b></b></div><span><a></a></span> 2. Open Inspector, start dragging second <b> element 3. Move mouse to a line with "</div>" / "</span>" / "</body>" 4. Move mouse before string "</div>" / "</span>" / "</body>" in that line Result: Markup-view indicates that <b> element will be dropped before the element But I'm not sure about Expectations. Now this looks like a useful but "not obvious at all" feature. Patrick, please tell whether this is desired behavior or not (since NI is already set)
(In reply to arni2033 from comment #2) > Created attachment 8696573 [details] > screenshot 1 (explanation) - dragging element is dropped _before_ hovered > element.png > > > move the mouse to the 'content' window, in order to get the cursor out of the inspector panel > Nicolas, that's definitely not the case. I just had no time to describe this > better. Improved STR: > 1. Open page data:text/html,<div><b></b><b></b></div><span><a></a></span> > 2. Open Inspector, start dragging second <b> element > 3. Move mouse to a line with "</div>" / "</span>" / "</body>" > 4. Move mouse before string "</div>" / "</span>" / "</body>" in that > line > > Result: Markup-view indicates that <b> element will be dropped before > the element > > But I'm not sure about Expectations. Now this looks like a useful but "not > obvious at all" feature. > Patrick, please tell whether this is desired behavior or not (since NI is > already set) My bad, now I get it better. The problem is due to the closing-tag div not taking the full width of the document, unlike children elements. In devtools/client/markupview/markup-view.js , in the _onMouseMove function of MarkupContainer, the underlying element of the cursor position is requested ( with this.markup.doc.elementFromPoint ). If the cursor is left from the closing-tag div, it's then its parent div that is returned, and thus the drop indicator gets misplaced. A solution might be to ensure that all the 'lines' in the markup view takes the full width of the document, but I don't really know if it could break something elsewhere. I'll work on it to find a solution, can somebody with bug-editing privilege assign it to me please ?
(In reply to arni2033 from comment #2) > Created attachment 8696573 [details] > screenshot 1 (explanation) - dragging element is dropped _before_ hovered > element.png > > > move the mouse to the 'content' window, in order to get the cursor out of the inspector panel > Nicolas, that's definitely not the case. I just had no time to describe this > better. Improved STR: > 1. Open page data:text/html,<div><b></b><b></b></div><span><a></a></span> > 2. Open Inspector, start dragging second <b> element > 3. Move mouse to a line with "</div>" / "</span>" / "</body>" > 4. Move mouse before string "</div>" / "</span>" / "</body>" in that > line > > Result: Markup-view indicates that <b> element will be dropped before > the element > > But I'm not sure about Expectations. Now this looks like a useful but "not > obvious at all" feature. > Patrick, please tell whether this is desired behavior or not (since NI is > already set) This isn't a desired behavior, this is a bug. We want the drop target to be the same whether you hover near the left edge or not.
Flags: needinfo?(pbrosset)
Attached patch patch.diff (obsolete) — — Splinter Review
First attempt at fixing this bug. I made the tag-line element full width so the elementFromPoint function called in the _onMouseMove will always return a tag-line, and thus, correctly place the drop indicator. In order to make it full width, I added a very wide negative left margin and a very wide positive left padding. I set a custom-property to be consistent in the margin and padding, and also to use it in an other CSS file, which added a left padding on the tag-line class. I ran the tests in the markupview folder, and a bunch of them failed. For example : ./mach test devtools/client/markupview/test/browser_markupview_dragdrop_reorder.js fails : 49 INFO TEST-UNEXPECTED-FAIL | devtools/client/markupview/test/browser_markupview_dragdrop_reorder.js | Test timed out - I took a look at the test but don't really know why it times out. Is there any way I can debug/set breakpoints on a mocha test ? I also removed an obsolete ( I think ) rule, which was made to 'indent' the drop indicator, but it does not work because of the 1000px offset on it. But I don't know if I should remove it, maybe it's a regression ?
Flags: needinfo?(pbrosset)
Attachment #8697313 - Flags: feedback?(pbrosset)
Attached patch patch.diff (obsolete) — — Splinter Review
So I managed to guess what was provoking the timeouts I mentioned in comment 5 ( using console.log() and info() and running the test, maybe there is a better way ?). The test timed out because markupmutation was never fired, because the test takes the upper left corner of source element to drag it. And because of my modifications, the upper left corner is off-screen, and the drag-and-drop is not done. Thus I went with an other way to fix this, but I'm not totally convinced by it. In the _onMouseMove, if the elementFromPoint returns an li element with the "child" class on it, I query the the last tag-line element on it, and if its top and bottom contains the y position of the cursor, I use it to show the drop indicator. All the tests from devtools/inspector/test passes, and it did fix the bug.
Attachment #8697313 - Attachment is obsolete: true
Attachment #8697313 - Flags: feedback?(pbrosset)
Attachment #8697876 - Flags: feedback?(pbrosset)
Sorry for the delay Nicolas, last week all of Mozilla was at a company event, so it couldn't review the patch here. I'll get to it tomorrow first thing.
Flags: needinfo?(pbrosset)
Comment on attachment 8697876 [details] [diff] [review] patch.diff Review of attachment 8697876 [details] [diff] [review]: ----------------------------------------------------------------- I agree that _onMouseMove seems like the right place to fix the bug because its something to do with getting the right hovered element with elementFromPoint. But I, like you, am not convinced by this approach. It complexifies the logic to get the hovered element quite a bit. I investigated the issue a little bit and I think we might have another solution. The problem we're having here is that when the mouse moves to the left, at some stage it goes out of the .tag-line element and onto the parent .child element. So, in fact your first approach looked better to me, somehow we need to have elements that expand to the full width. But it turns out we already do! These elements are .tag-state. We have one per element in the tree, and they're used to show elements as hovered or selected. The problem is that they're set to z-index:-1 right now, making them not get any pointer event. I've tested this and it seems to fix the problem: .tag-line .tag-state { z-index: 0; } /* Bring the tag-state up so it receives mouse events */ .expander { position: relative; z-index: 1; } /* Make sure the expander still appears above the tag-state */ .editor { position: relative; z-index: 1; } /* And that the editor does too */ This way, the mouseover events will always reach the tag-state element, not some parent .child container. Can you try this out?
Attachment #8697876 - Flags: feedback?(pbrosset)
Patrick, the code in Comment 8 fix the bug in my opinion. I edited the existing rules with your fix and everything works fine, all the tests of the markupview folder passed.
Attachment #8697876 - Attachment is obsolete: true
Attachment #8700194 - Flags: review?(pbrosset)
Comment on attachment 8700194 [details] [diff] [review] proposed patch for bug resolution Review of attachment 8700194 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Please address the minor comments and upload a new patch with R+. I've pushed this one to TRY in the meantime, so we catch any potential regressions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66f830339857 ::: devtools/client/markupview/markup-view.css @@ +134,5 @@ > .expander { > display: inline-block; > margin-left: -14px; > vertical-align: middle; > + position: relative; nit: please remove the trailing whitespace @@ +135,5 @@ > display: inline-block; > margin-left: -14px; > vertical-align: middle; > + position: relative; > + /* Make sure the expander still appears above the tag-state */ Please move this comment above position:relative, because relative positioning is necessary for the z-index to take effect. @@ +136,5 @@ > margin-left: -14px; > vertical-align: middle; > + position: relative; > + /* Make sure the expander still appears above the tag-state */ > + z-index: 1; nit: please remove the trailing whitespace @@ +192,5 @@ > display: none; > cursor: pointer; > } > > +.editor { nit: please remove the trailing whitespace @@ +193,5 @@ > cursor: pointer; > } > > +.editor { > + position: relative; nit: please remove the trailing whitespace @@ +194,5 @@ > } > > +.editor { > + position: relative; > + /* Make sure the editor still appears above the tag-state */ Please move this comment above position:relative, because relative positioning is necessary for the z-index to take effect. @@ +195,5 @@ > > +.editor { > + position: relative; > + /* Make sure the editor still appears above the tag-state */ > + z-index: 1; nit: please remove the trailing whitespace @@ +196,5 @@ > +.editor { > + position: relative; > + /* Make sure the editor still appears above the tag-state */ > + z-index: 1; > +} nit: please remove the trailing whitespace
Attachment #8700194 - Flags: review?(pbrosset) → review+
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
Try was green, so I ended up fixing those nits myself and pushed to fx-team. Thanks Nicolas!
Attachment #8700578 - Flags: review+
Attachment #8700194 - Attachment is obsolete: true
Thank you Patrick. Can you suggest me an other bug to work on ?
(In reply to Nicolas Chevobbe from comment #13) > Thank you Patrick. > Can you suggest me an other bug to work on ? I'm working mostly on animation inspection at the moment, so I'm going to suggest bugs for the animation panel: - bug 1228978 : minor visual change for a better UX - bug 1228080 : splitting files to make it easier to work on them - bug 1227477 : polishing the way the time graduations are calculated I can provide different ones if you want, also http://firefox-dev.tools may be helpful
Thanks, I'll take a look at one of these this evening
https://hg.mozilla.org/mozilla-central/rev/fd7f6a9dcfad
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
I have reproduced this bug on Nightly 43.0a1 (2015-08-16) on ubuntu 14.04 LTS, 32 bit! The bug's fix is now verified on Latest Nightly 46.0a1! Build ID: 20151227030239 User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0
QA Whiteboard: [bugday-20151223]
Status: RESOLVED → VERIFIED
Summary: Drag and drop mode in inspector should respect mouse position → Drag and drop mode in inspector shows indicator before opening tag when mouse is placed to the left from closing tag
[bugday-20160323] Status: RESOLVED,FIXED -> VERIFIED Comments: Test successful Component: Name Firefox Version 46.0b4 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: <b></b> tag is able to move before <div> Actual Results: As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: