1173504 - Allow to remove elements while inspecting

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Sebastian Zartner [:sebo]]

While the inspector mode is enabled it should be possible to delete elements by hitting Delete without needing to lock the inspection to an element first.

Sebastian

Comment 1

[Patrick Brosset <:pbro>]

We could do that indeed, that's a pretty powerful way of cleaning up a page to investigate an issue or just remove unwanted things while browsing. I've seen this done in several addons.
Doing this is pretty easy, the main entry point is: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js?from=highlighter.js&case=true#299
(we already have a bunch of key listeners to navigate the DOM while the pick mode is enabled).

2 worries I have:
- these shortcuts are not discoverable, we at least need to remember to update the doc if we add this new one,
- should we worry about users accidentally deleting elements and not being able to undo this action?

Comment 2

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #1)
> - should we worry about users accidentally deleting elements and not being
> able to undo this action?

People can actually revert this action by hitting Ctrl+Z within the Inspector panel (which is also not very discoverable, btw.).
FWIW Firebug has this feature for a long time and nobody complained so far. It was actually claimed to be missing from the DevTools in the Firebug discussion group[1] lately, which is why I created this report.

Sebastian

[1] https://groups.google.com/d/msg/firebug/d-_T83v1IJs/9b3ox0BkKzkJ

Comment 3

[Patrick Brosset <:pbro>]

Ok, if firebug has it and that didn't cause any kind of frustration, let's go with it, I like the feature personally anyway.
As for the ctrl+Z, that won't work, the inspector panel only allows to undo certain actions that were initiated from the markup-view. That's something else we need to fix, but in another bug.

Anyway, I've set myself as mentor here because I don't have time to do this now, but I believe this is a simple enough bug, I'll add some flags.

Comment 4

[dave.gonzalez]

Hi there! Can I try to tackle this bug? I've spent a bit of time on it and think I may have a (possibly naive) solution.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to dave.gonzalez from comment #4)
> Hi there! Can I try to tackle this bug? I've spent a bit of time on it and
> think I may have a (possibly naive) solution.
Awesome. Feel free to attach a patch with your solution and ping me for feedback.
Also make sure you check out this wiki page first: https://wiki.mozilla.org/DevTools/Hacking
I'll assign the bug to you.

Comment 6

[dave.gonzalez]

Attachment: Patch for bug 1173504.

Patch cancel the highlighter, removes the node, then emits an event to unselect the highlighter.

Comment 7

[dave.gonzalez]

Thanks! Let me know if I should be doing anything differently - this will be my first time contributing.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8625564 [details] [diff] [review]
Patch for bug 1173504.

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

This looks great. Sweet and simple. I haven't had the time to try it locally yet, but in the meantime, do you think you could work on adding a new automated test to ensure this feature keeps on working in the future?
This test should be a devtools browser mochitest (some info here: https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests and here https://wiki.mozilla.org/DevTools/mochitests_coding_standards).
We already have tests for other highlighter key bindings, so the best way to start here would be to look at those other tests first:
browser/devtools/inspector/test/browser_inspector_highlighter-keybinding_*
Reading and executing those tests should give you a good idea of what's expected.
Let me know if you need any more help with this. I can even provide a new test file skeleton if you think this would help.

Comment 9

[dave.gonzalez]

Attachment: bug-1173504-tests.patch

I think I was able to follow the patterns that I saw in the other keybinding test files. I'm still figuring my way through Mercurial, so it looks like I accidentally created a new patch for the tests. Is there a simple way to cherry-pick these changes onto my original patch?

I set up three test cases - delete key usage, backspace key usage, and then moving up a node using the LEFT arrow option in order to delete the newly hovered node.

Thanks!

Comment 10

[Patrick Brosset <:pbro>]

(In reply to dave.gonzalez from comment #9)
> Created attachment 8626294 [details] [diff] [review]
> bug-1173504-tests.patch
> 
> I think I was able to follow the patterns that I saw in the other keybinding
> test files. I'm still figuring my way through Mercurial, so it looks like I
> accidentally created a new patch for the tests. Is there a simple way to
> cherry-pick these changes onto my original patch?
Uploading 2 separate patches for this isn't a problem, it actually helps reviewing. If you want to fold these 2 commits into one anyway, you can do this:
- if you're using MQ to create your commits, you should push up the 1st one only and then `hg qfold <the-second-one>'
- if you're not using MQ and are just creating your commits manually with 'hg commit', then you can rewrite the commit history using the 'hg histedit' extension which works very similarly to 'git rebase -i'.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8626294 [details] [diff] [review]
bug-1173504-tests.patch

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

Looks great, I only have a few comments about code simplification.
Also, you forgot to add this new test in the browser.ini file in the same directory.
Once done, try and run this new test locally with ./mach mochitest browser/devtools/inspector/test/browser_inspector_highlighter-keybinding_05.js

::: browser/devtools/inspector/test/browser_inspector_highlighter-keybinding_05.js
@@ +19,5 @@
> +
> +  // Testing delete-node shortcut using DELETE key
> +  info("Testing delete key as delete-node command");
> +  yield doKeyDelete({key: "VK_DELETE", options: {}});
> +  is(inspector.selection.nodeFront.id, null, "The #ahoy node was removed. Passed.");

I think we should more thoroughly test that the element has been removed from the DOM here by:
- trying to querySelector it from the DOM using the walker:
  let node = yield toolbox.walker.querySelector(toolbox.walker.rootNode, "#ahoy");
  ok(!node, "The node has been deleted");
- verifying that the expected element is now selected in the inspector (here you're just making sure that the newly selected element has no id, can we do something more to ensure it is really the element we expect?)

@@ +30,5 @@
> +
> +  // Testing delete-node shortcut using BACKSPACE key
> +  info("Testing backspace key as delete-node command");
> +  yield doKeyDelete({key: "VK_BACK_SPACE", options: {}});
> +  is(inspector.selection.nodeFront.id, null, "The #simple-div2 node was removed. Passed.");

Same comments here.
Also, the last 8 lines are almost an exact duplicate with the 8 first lines in the test, can you extract that into a reusable function:

function* testDelete(selector, key) {
  info("Starting element picker.");
  yield toolbox.highlighterUtils.startPicker();

  info("Selecting the " + selector + " element");
  yield moveMouseOver(selector);

  info("Testing the element delete key " + key);
  yield doKeyDelete({key, options: {}});
  
  ... then the assertions ...
}

And then use it like:

yield testDelete("#ahoy", "VK_DELETE");
yield testDelete("#simple-div2", "VK_BACK_SPACE");

@@ +63,5 @@
> +    }, null, false);
> +    return inspector.toolbox.once("picker-node-hovered");
> +  }
> +
> +  function doKeyHover(msg) {

Almost exactly like doKeyDelete, I think we should only have 1 of these functions with the right arguments so it can be used from all the same places.

Comment 12

[dave.gonzalez]

Thanks for the feedback! Sorry for the delay in responding - I'm in the middle of a cross country move, but I should be settled in by the end of the week. I'll hop back on and send in a new test patch + the browser.ini change then.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Dave, are you still planning to work on this?

Comment 14

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES.

Comment 15

[Rohit Kumar Jena]

Should I work on this bug?

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Rohit Kumar Jena from comment #15)
> Should I work on this bug?
If you want to, yes, I can assign it to you.
Feel free to get used to the dev environment first: https://developer.mozilla.org/en-US/docs/Tools/Contributing
And then get the code changes from attachment 8626294 [details] [diff] [review] as a starting point and go through my comment 11 for a list of things to finalize before this can land.
Thanks.

Comment 17

[Rohit Kumar Jena]

(In reply to Patrick Brosset <:pbro> from comment #16)
> (In reply to Rohit Kumar Jena from comment #15)
> > Should I work on this bug?
> If you want to, yes, I can assign it to you.
> Feel free to get used to the dev environment first:
> https://developer.mozilla.org/en-US/docs/Tools/Contributing
> And then get the code changes from attachment 8626294 [details] [diff] [review]
> [review] as a starting point and go through my comment 11 for a list of
> things to finalize before this can land.
> Thanks.

Let me get used to the code first. Mozilla code base is huge and I'm a beginner. Let me see if I can get ready. Will let you know in a couple of days.
Thanks.

Comment 18

[Rohit Kumar Jena]

I needed some help in here. First of all, I'm unable to replicate the problem. In the inspector mode, I simply click on the node I want to delete and press the Delete button, and it is deleted. Is it what is being referred to as 'locking' to an element? 
Secondly, I had a look at the patch given, is it coded in nodejs? 
Lastly, the link given in Comment 1, is broken. I wanted to know the code structure and a starting point. 
Thanks.

Comment 19

[Rohit Kumar Jena]

Ok, I figured out the bug. Sorry for the inconvenience caused.

Comment 20

[Sebastian Zartner [:sebo]]

Just for clarification:

(In reply to Rohit Kumar Jena from comment #18)
> I needed some help in here. First of all, I'm unable to replicate the
> problem. In the inspector mode, I simply click on the node I want to delete
> and press the Delete button, and it is deleted. Is it what is being referred
> to as 'locking' to an element?

The point is *not* having to click the node first. I.e. while hovering the element within the page you should be able to delete it.

> Secondly, I had a look at the patch given, is it coded in nodejs? 

Firefox' UI functionality is built in JavaScript. It's completely unrelated to Node.js.

> Lastly, the link given in Comment 1, is broken. I wanted to know the code
> structure and a starting point. 

The code from comment 1 has moved. I guess it should point to https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/devtools/server/actors/highlighters.js#298.

Sebastian

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner [:sebo] from comment #20)
> Just for clarification:
> 
> (In reply to Rohit Kumar Jena from comment #18)
> > I needed some help in here. First of all, I'm unable to replicate the
> > problem. In the inspector mode, I simply click on the node I want to delete
> > and press the Delete button, and it is deleted. Is it what is being referred
> > to as 'locking' to an element?
> 
> The point is *not* having to click the node first. I.e. while hovering the
> element within the page you should be able to delete it.

Just want to double check this is something we want.  This is an unfamiliar pattern to me and I could see easily accidentally deleting the node I wanted to if my mouse just happens to be over a different one.  Helen, any thoughts?

Comment 22

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

(In reply to Brian Grinstead [:bgrins] from comment #21)
> (In reply to Sebastian Zartner [:sebo] from comment #20)
> > Just for clarification:
> > 
> > (In reply to Rohit Kumar Jena from comment #18)
> > > I needed some help in here. First of all, I'm unable to replicate the
> > > problem. In the inspector mode, I simply click on the node I want to delete
> > > and press the Delete button, and it is deleted. Is it what is being referred
> > > to as 'locking' to an element?
> > 
> > The point is *not* having to click the node first. I.e. while hovering the
> > element within the page you should be able to delete it.
> 
> Just want to double check this is something we want.  This is an unfamiliar
> pattern to me and I could see easily accidentally deleting the node I wanted
> to if my mouse just happens to be over a different one.  Helen, any thoughts?

This pattern seems really really odd to me. I can't say it seems like a great idea for all users. That said, those are my two cents, and I'll leave the final decision up to Patrick.

Comment 23

[Sebastian Zartner [:sebo]]

Brian, Helen, please see comment 2 and comment 3. As Patrick previously expressed that he'd like to have this feature, I'm removing the ni.

Sebastian

Comment 24

[Brian Grinstead [:bgrins]]

It's ultimately his decision but I'm requesting that he re-evaluate given the concerns expressed in the last couple of comments.  This would be hard to discover, requires strong interaction between keyboard and mouse, and could cause people to accidentally delete nodes they didn't want to.  One other consideration: if delete works on hover, then what about all the other inspector key shortcuts?

Comment 25

[Sebastian Zartner [:sebo]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> It's ultimately his decision but I'm requesting that he re-evaluate given
> the concerns expressed in the last couple of comments.

Ok.

> This would be hard to discover

This could be handled by some UI like a tooltip, for example.

> requires strong interaction between keyboard and mouse

Which can be seen negative or positive.

> could cause people to accidentally delete nodes they didn't want to.

I suspect that accidentally pressing Delete while inspecting may be rare. And in case it happens though, you are able to revert the change via Ctrl+Z (but this functionality is still hard to discover).

> One other consideration: if delete works on hover, then what about all the other inspector key shortcuts?

They may work as well - at least some of them. Reading a comment in the highlighter code[1] it sounds like this should already work, though it currently doesn't.

Btw. I rather see this feature as gap between Firebug and the DevTools and carried it over from the Firebug discussion group. Personally I don't have a strong need for it myself, but I believe it does improve UX.

Sebastian

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.js#291-297

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner [:sebo] from comment #25)
> I suspect that accidentally pressing Delete while inspecting may be rare.
> And in case it happens though, you are able to revert the change via Ctrl+Z
> (but this functionality is still hard to discover).

The scenario I'm thinking of is you purposefully press delete thinking it will delete the node that is selected in the markup view, but your mouse just happens to be over some other node.  It would be a confusing / hard to reproduce failure of the tool

Comment 27

[Patrick Brosset <:pbro>]

As I said in comment 1 and comment 3, I have a few concerns about this feature:

- Discoverability: this is a global devtools problem, not just for the highlighter, and not just for the inspector. There are many shortcuts and key bindings people don't know about. However I don't think this should block this bug, we need bug 892188 for this.

- Accidental element deletions, which is Brian's point. Personally, I think these cases would be super rare as Sebastian said, and I was comforted by the fact that Firebug had this feature. That's why I initially decided to go with it. 

Why would someone press the DELETE key in element pick mode?
  - maybe you might have forgotten, or not realized that you were in pick mode, and you might believe the focus to be in the markup-view (see more about focus below). If that is the case, then you'd expect DELETE to remove the selected element, not the one currently hovered in the page.
  --> we should make it visually easy to know that you're in pick mode and that the page content is focused.
  - you might accidentally press DELETE while wanting to press ENTER (the 2 keys are close to each others on a keyboard), thinking that ENTER would select the hovered element (which it does).

- My 3rd concern, which in fact would solve the accidental deletion, is Undo. Sebastian mentioned this as a good solution, which I think it is, but as said in comment 3, this won't work at the moment. Our Undo/Redo mechanism in the inspector is client-side only. Meaning that the markup-view remembers what actions have been done on the DOM, and allows users to undo or redo them. But when you're in pick mode, deleting elements from the page, the markup-view has no knowledge about that. We should really be handling undo/redo on the server-side as a single stack of actions no matter where these actions come from. But that is not the case right now.

About focus: right now, when you engage the pick mode, the toolbox keeps the focus. So none of the pick mode key bindings work by default. I can't remember if this was always like this or if it's a regression, but the fact is that pressing DELETE right after having clicked on the element picker wouldn't do anything anyway. You'd first have to focus the content (by, e.g., clicking in the URL bar and pressing TAB once).

So, it looks to me like we need 3 things:
- bug 892188 (although this shouldn't be a blocker),
- server-side undo/redo so that ctrl+Z works for nodes deleted in pick mode too,
- better focus handling so that users know when content is focused.

Now, I still think this isn't such an odd pattern to have. Many (all?) graphical/prototyping tools allow to select elements from the artboard/page and delete them with the keyboard. Now, I know devtools isn't one of those tools, but it seemed similar enough that this pattern would be recognized by users. This, plus the fact that Firebug had it made me think we should implement it.

However, I've come to realize lately that adding features in devtools is a really big deal, and we should be really cautious about which ones we add and how they behave.

So, based on Brian's and Helen's feedback, I think we can safely assume that this feature is a controversial one. So let's hold off until we have the 3 things described above. Then we can re-assess if we want it. But shipping it before those things will likely create more problems than it will solve.

Comment 28

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

(In reply to Patrick Brosset <:pbro> from comment #27)
> Now, I still think this isn't such an odd pattern to have. Many (all?)
> graphical/prototyping tools allow to select elements from the artboard/page
> and delete them with the keyboard. Now, I know devtools isn't one of those
> tools, but it seemed similar enough that this pattern would be recognized by
> users. This, plus the fact that Firebug had it made me think we should
> implement it.

I think it was mentioned elsewhere in this bug that there are a few GUIs that use this pattern, so I went through and checked Sketch, Photoshop, and Illustrator, and none of them had it, at least. Perhaps I'm understanding this feature fundamentally?

If an element is selected in the markup view like so: http://cl.ly/3X3h0v1E472g hitting the "Delete" key should remove that node. That makes sense to me.

> (In reply to Sebastian Zartner [:sebo] from comment #20)
> > The point is *not* having to click the node first. I.e. while hovering the
> > element within the page you should be able to delete it.

I was under the impression that this bug was about deleting nodes that were not selected, like so: http://cl.ly/0C3u0i0r0n3e So, with an element not selected but hovered, hitting "Delete" would remove the /hovered/ node, not the selected one. This, I assume, was also Brian's concern—this is an odd pattern that I have not seen in any GUIs.

After reading your comment, Patrick, I played around with the Inspector more and it appears that one cannot delete a node without selecting it with the Inspector pick first: http://cl.ly/0V3a2w1i0k2Z I can understand why a bug would be filed against that, as it makes sense that one should be able to remove a line if it is targeted like this screenshot: http://cl.ly/3X3h0v1E472g 

Is this bug to solve that last use case, or to solve the issues brought up in comment #20?

Comment 29

[Sebastian Zartner [:sebo]]

Thank you for the detailed analysis, Patrick!

(In reply to Patrick Brosset <:pbro> from comment #27)
> - Accidental element deletions, which is Brian's point. Personally, I think
> these cases would be super rare as Sebastian said, and I was comforted by
> the fact that Firebug had this feature. That's why I initially decided to go
> with it. 
> 
> Why would someone press the DELETE key in element pick mode?
>   - maybe you might have forgotten, or not realized that you were in pick
> mode, and you might believe the focus to be in the markup-view (see more
> about focus below). If that is the case, then you'd expect DELETE to remove
> the selected element, not the one currently hovered in the page.
>   --> we should make it visually easy to know that you're in pick mode and
> that the page content is focused.
>   - you might accidentally press DELETE while wanting to press ENTER (the 2
> keys are close to each others on a keyboard), thinking that ENTER would
> select the hovered element (which it does).

Just to be clear, the question is 'Why would someone *accidentally* press the Delete key in element pick mode?', but yes, I guess that are the main use cases where this can happen.

> So, it looks to me like we need 3 things:
> - bug 892188 (although this shouldn't be a blocker),
> - server-side undo/redo so that ctrl+Z works for nodes deleted in pick mode
> too,
> - better focus handling so that users know when content is focused.

+1 to all three of them.

> Now, I still think this isn't such an odd pattern to have. Many (all?)
> graphical/prototyping tools allow to select elements from the artboard/page
> and delete them with the keyboard. Now, I know devtools isn't one of those
> tools, but it seemed similar enough that this pattern would be recognized by
> users. This, plus the fact that Firebug had it made me think we should
> implement it.
> 
> However, I've come to realize lately that adding features in devtools is a
> really big deal, and we should be really cautious about which ones we add
> and how they behave.
> 
> So, based on Brian's and Helen's feedback, I think we can safely assume that
> this feature is a controversial one. So let's hold off until we have the 3
> things described above. Then we can re-assess if we want it. But shipping it
> before those things will likely create more problems than it will solve.

Also +1 to this.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #28)
> (In reply to Patrick Brosset <:pbro> from comment #27)
> > Now, I still think this isn't such an odd pattern to have. Many (all?)
> > graphical/prototyping tools allow to select elements from the artboard/page
> > and delete them with the keyboard. Now, I know devtools isn't one of those
> > tools, but it seemed similar enough that this pattern would be recognized by
> > users. This, plus the fact that Firebug had it made me think we should
> > implement it.
> 
> I think it was mentioned elsewhere in this bug that there are a few GUIs
> that use this pattern, so I went through and checked Sketch, Photoshop, and
> Illustrator, and none of them had it, at least. Perhaps I'm understanding
> this feature fundamentally?
> 
> If an element is selected in the markup view like so:
> http://cl.ly/3X3h0v1E472g hitting the "Delete" key should remove that node.
> That makes sense to me.
> 
> > (In reply to Sebastian Zartner [:sebo] from comment #20)
> > > The point is *not* having to click the node first. I.e. while hovering the
> > > element within the page you should be able to delete it.
> 
> I was under the impression that this bug was about deleting nodes that were
> not selected, like so: http://cl.ly/0C3u0i0r0n3e So, with an element not
> selected but hovered, hitting "Delete" would remove the /hovered/ node, not
> the selected one.

Correct.

> This, I assume, was also Brian's concern—this is an odd
> pattern that I have not seen in any GUIs.

The Windows Explorer allows to enable selection on hovering, see http://i.imgur.com/5d5W8IZ.png. (Though it's not exactly the same for the DevTools, because the hovered element would not be auto-selected.)
Common GUIs usually only allow to select elements on click but don't highlight them on hover. (I don't have Sketch, Photoshop, nor Illustrator, could be that they are different.) So, the pick mode in the DevTools is different in this regard.

> After reading your comment, Patrick, I played around with the Inspector more
> and it appears that one cannot delete a node without selecting it with the
> Inspector pick first: http://cl.ly/0V3a2w1i0k2Z I can understand why a bug
> would be filed against that, as it makes sense that one should be able to
> remove a line if it is targeted like this screenshot:
> http://cl.ly/3X3h0v1E472g 
> 
> Is this bug to solve that last use case, or to solve the issues brought up
> in comment #20?

It is about deleting hovered elements.
But again, bug 892188, server-side undo/redo (bug 773510?) and improved focus handling (no bug filed yet) should be tackled first.

Sebastian

Comment 30

[Patrick Brosset <:pbro>]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #28)
> If an element is selected in the markup view like so:
> http://cl.ly/3X3h0v1E472g hitting the "Delete" key should remove that node.
> That makes sense to me.
Yes, that's a feature we already have and are not planning to remove.

> I was under the impression that this bug was about deleting nodes that were
> not selected, like so: http://cl.ly/0C3u0i0r0n3e So, with an element not
> selected but hovered, hitting "Delete" would remove the /hovered/ node, not
> the selected one. This, I assume, was also Brian's concern—this is an odd
> pattern that I have not seen in any GUIs.
I think there's a misunderstanding here: this bug is about deleting nodes that are hovered *in the page* when you move your mouse over nodes and while the pick mode is ON.
1 - click on the element picker icon
2 - start moving your mouse over elements in the page
3 - as you do this, the highlighter shows the box-model over hovered elements
4 - find an element you want to remove, move your mouse over it, and press DELETE.

We're not saying you should be able to delete elements by hovering over them *in the markup-view*, that would indeed be odd.

Comment 31

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset <:pbro> from comment #30)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #28)
> > I was under the impression that this bug was about deleting nodes that were
> > not selected, like so: http://cl.ly/0C3u0i0r0n3e So, with an element not
> > selected but hovered, hitting "Delete" would remove the /hovered/ node, not
> > the selected one. This, I assume, was also Brian's concern—this is an odd
> > pattern that I have not seen in any GUIs.
> I think there's a misunderstanding here: this bug is about deleting nodes
> that are hovered *in the page* when you move your mouse over nodes and while
> the pick mode is ON.
> 1 - click on the element picker icon
> 2 - start moving your mouse over elements in the page
> 3 - as you do this, the highlighter shows the box-model over hovered elements
> 4 - find an element you want to remove, move your mouse over it, and press
> DELETE.
> 
> We're not saying you should be able to delete elements by hovering over them
> *in the markup-view*, that would indeed be odd.

Oh yes, I didn't realize Helen could have meant something different. As Patrick said, we're talking about deleting elements while inspecting them within the page. Thank you for clarifying that, Patrick!

Sebastian

Comment 32

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

All right, thanks everyone! This bug has been pretty confusing, thanks for being patient.

The original intent of this bug was to be able to delete nodes when one is hovering over nodes in-page with the "Inspect Element" picker on. I've checked release Chrome, Canary, webkit Nightly, and release Safari, and no one else seems to have this pattern by default. I think for something as prevalent as the Inspect-element we should follow the crowd. I don't want users having to remember "That thing Firefox does differently with their inspect thing" while they're cross-browser testing. While a potentially useful tool to a small set, I think this would be interpreted as a cross-browser papercut by many.

(In reply to Sebastian Zartner [:sebo] from comment #29)
> The Windows Explorer allows to enable selection on hovering, see
> http://i.imgur.com/5d5W8IZ.png. (Though it's not exactly the same for the
> DevTools, because the hovered element would not be auto-selected.)
> Common GUIs usually only allow to select elements on click but don't
> highlight them on hover. (I don't have Sketch, Photoshop, nor Illustrator,
> could be that they are different.) So, the pick mode in the DevTools is
> different in this regard.
I think this conversation could be very different if it was being introduced under an option, like this screenshot is suggesting it is for Windows Explorer. However, changing the default behavior of the Inspect picker instead of it being an option brings me back to papercuts between browsers, etc. If this bug could handle adding this behavior as an option that users have to go to Settings and turn on, my concerns would be placated.

Comment 33

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 34

[Kimberly Pennington [:kimberlythegeek]]

Should this bug be marked as closed? It appears this feature has been added.

Comment 35

[Sebastian Zartner [:sebo]]

(In reply to Kimberly Pennington [:kimberlythegeek] from comment #34)
> Should this bug be marked as closed? It appears this feature has been added.

No, it isn't implemented yet. Note that deletion should work while you are inspecting, i.e. the element is only hovered within the website but not clicked.

Sebastian

Comment 36

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner [:sebo] from comment #35)
> (In reply to Kimberly Pennington [:kimberlythegeek] from comment #34)
> > Should this bug be marked as closed? It appears this feature has been added.
> 
> No, it isn't implemented yet. Note that deletion should work while you are
> inspecting, i.e. the element is only hovered within the website but not
> clicked.

I don't think we will be making this the default behavior based on the discussion so far here and comment 32.  It could be added as an option if we wanted to go that route but I assume it won't get much usage if it's off by default.

Comment 37

[Sebastian Zartner [:sebo]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #32)
> (In reply to Sebastian Zartner [:sebo] from comment #29)
> > The Windows Explorer allows to enable selection on hovering, see
> > http://i.imgur.com/5d5W8IZ.png. (Though it's not exactly the same for the
> > DevTools, because the hovered element would not be auto-selected.)
> > Common GUIs usually only allow to select elements on click but don't
> > highlight them on hover. (I don't have Sketch, Photoshop, nor Illustrator,
> > could be that they are different.) So, the pick mode in the DevTools is
> > different in this regard.
> I think this conversation could be very different if it was being introduced
> under an option, like this screenshot is suggesting it is for Windows
> Explorer.

As far as I remember, this option is the default in Windows 7. (I might be wrong though. A long time passed since I installed Win7 the last time.)

> However, changing the default behavior of the Inspect picker
> instead of it being an option brings me back to papercuts between browsers,
> etc. If this bug could handle adding this behavior as an option that users
> have to go to Settings and turn on, my concerns would be placated.

(In reply to (Unavailable until Jan 4) Brian Grinstead [:bgrins] from comment #36)
> (In reply to Sebastian Zartner [:sebo] from comment #35)
> > (In reply to Kimberly Pennington [:kimberlythegeek] from comment #34)
> > > Should this bug be marked as closed? It appears this feature has been added.
> > 
> > No, it isn't implemented yet. Note that deletion should work while you are
> > inspecting, i.e. the element is only hovered within the website but not
> > clicked.
> 
> I don't think we will be making this the default behavior based on the
> discussion so far here and comment 32.  It could be added as an option if we
> wanted to go that route but I assume it won't get much usage if it's off by
> default.

The main counter-point in earlier comments was that it's highly unlikely that you unintentionally press Delete while you're in pick mode. The papercut Helen is talking about is currently experienced by Firebug users, as they are used to it there.
And I agree that having this behind an option will block its usage.

Patrick did a very good analysis of this feature in comment 27 and its pros and cons. As he said, let's first solve the blocking issues and then reevaluate this.

Sebastian

Comment 38

[Patrick Brosset <:pbro>]

See comment 27's conclusion, this isn't a bug we should fix now, and therefore does not qualify as a good-first-bug that people can pick up. Let's remove the flag for now.