Closed Bug 1261781 Opened 10 years ago Closed 10 years ago

Add an inspector toolbar button to insert new elements in the DOM

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [btpp-backlog])

Attachments

(1 file, 2 obsolete files)

MozReview Request: Bug 1261781 - Inspector toolbar button for inserting new elements; r=jdescottes
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
Similarly to how there is a + button in the rules-view to insert new rules, there should be a + button in the markup-view to insert new elements. Why should the markup-view be primarily an inspection tool? I think it has a role to play in authoring too. My gut feeling is that while people are used to modifying styles in the rule-view by adding/removing/changing properties, they're much less used to modifying things in the markup-view, more often going back to their editors and refreshing the page to see the changes. A simple + sign in the toolbar would make it consistent with the rule-view and allow people to add new elements very quickly. I imagine this would insert a new element as the last child of the currently selected element. Technically adding this is very simple. There are some edge cases to take into account and questions to answer however: - should a <div> be inserted? - should there be a modal displayed to enter attributes before inserting the element? - if the document is XUL, should a <box> be inserted instead? - if the document is SVG, should a <rect> be inserted instead? - if the currently selected element is <head>, or <html>, <script>, <style>, etc... should we still insert an element or grey-out the button? - if a pseudo-element is selected, the button should probably not work either.
This is a draft patch, with none of the questions above answered. It will simply unconditionally insert an element at the end of whatever element is selected. And there are no tests.
(In reply to Patrick Brosset [:pbro] from comment #0) My two cents: > Technically adding this is very simple. > There are some edge cases to take into account and questions to answer > however: > - should a <div> be inserted? In HTML document it's probably OK, but we should focus the new element immediately, so that the user can change if needed, with few steps. > - should there be a modal displayed to enter attributes before inserting the > element? I don't think is needed in the first iteration; the users should be able to add attributes from the markup view as usual. > - if the document is XUL, should a <box> be inserted instead? I'm not sure about that, but I don't think also that matters too much. > - if the document is SVG, should a <rect> be inserted instead? In that case it's probably different from HTML and XUL, since every tag has a specific purpose; a rect is probably one of the less flexible; so we should really understand the user case for the SVG I think. Personally, I don't mind to enable such feature only for HTML at the beginning, and investigate more for other markup languages. > - if the currently selected element is <head>, or <html>, <script>, <style>, > etc... should we still insert an element or grey-out the button? I would probably disable the button, if the context is not suitable to its action. > - if a pseudo-element is selected, the button should probably not work > either. Agreed. (In reply to Patrick Brosset [:pbro] from comment #1) > This is a draft patch, with none of the questions above answered. It will > simply unconditionally insert an element at the end of whatever element is > selected. I played a bit with the patch; I think we should definitely expand the current element when we add the new child, and highlight the new element added; otherwise – especially if the parent has already children – it seems the button didn't have any effect at all.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
This should be complete now. Just need some tests. I'll work on them tomorrow and ask review then.
Attachment #8737728 - Attachment is obsolete: true
Comment on attachment 8737824 [details] [diff] [review] Bug_1261781_-_Inspector_toolbar_button_for_inserti.diff Going to push a finalized patch to mozreview in a bit.
Attachment #8737824 - Attachment is obsolete: true
Attachment #8738071 - Flags: review?(jdescottes) → review+
Comment on attachment 8738071 [details] MozReview Request: Bug 1261781 - Inspector toolbar button for inserting new elements; r=jdescottes https://reviewboard.mozilla.org/r/44279/#review40961 Thanks for the new feature! Implementation is easy to read, and great tests, nice and short! Just a few comments and nits, otherwise looks good to me. ::: devtools/client/inspector/inspector-panel.js:458 (Diff revision 1) > + canAddHTMLChild: function() { > + return this.selection.isHTMLNode() && > + this.selection.isElementNode() && > + !this.selection.isPseudoElementNode() && > + !this.selection.isAnonymousNode(); > + }, Some edge cases to review : - feature enabled when <html> is selected, but does nothing - feature enabled when <iframe> is selected, but does nothing - can add <div>s in <head> - can add <div>s in <textarea>, <script> (but they are added as text) The add button and menu item should be disabled for these elements. (I'm ok with handling this in a follow-up as well, none of those are breaking the devtools) ::: devtools/client/inspector/test/browser_inspector_addNode_02.js:46 (Diff revision 1) > + let btn = doc.querySelector("#inspector-element-add-button"); > + let item = doc.querySelector("#node-menu-add"); > + > + // Force an update of the node-menu first, since it normally only updates on > + // popupshown. > + inspector._setupNodeMenu({target: {}}); This makes the test simpler but is a bit hacky. I guess the only alternative is to actually open the context menu to trigger an update? I think it's best to avoid using "private" methods, even in tests. Given that the alternative is painful to implement, up to you :) ::: devtools/client/inspector/test/browser_inspector_addNode_03.js:7 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test that the adding nodes does work as expected: the parent gets expanded nit: "Test that the adding" -> "Test that adding"? ::: devtools/client/inspector/test/browser_inspector_addNode_03.js:32 (Diff revision 1) > + parentNode = yield getNodeFront("#bar", inspector); > + yield selectNode(parentNode, inspector); > + collapseNode(parentNode, inspector); > + yield testAddNode(parentNode, inspector); > + > + info("Adding in alement with children that is expanded"); nit: s/alement/element ::: devtools/client/inspector/test/browser_inspector_addNode_03.js:47 (Diff revision 1) > + let onMutation = inspector.once("markupmutation"); > + btn.click(); > + let mutations = yield onMutation; > + > + // Expect an inspector-updated event right after the mutation event. > + yield inspector.once("inspector-updated"); Any risk we could miss the "inspector-updated" event here? Why not add the listener before the click, do we expect several inspector-updated events? ::: devtools/client/locales/en-US/inspector.dtd:162 (Diff revision 1) > <!ENTITY inspectorDuplicateNode.label "Duplicate Node"> > + > +<!-- LOCALIZATION NOTE (inspectorAddNode.label): This is the label shown in > + the inspector toolbar for the button that lets users add elements to the > + DOM (as children of the currently selected element). --> > +<!ENTITY inspectorAddNode.label "Create new node"> Capitalize: most menu items are capitalized right now (all except "Use in Console"). "Create New Node" ? ::: devtools/client/themes/inspector.css:44 (Diff revision 1) > } > > +/* Add element toolbar button */ > + > +#inspector-element-add-button { > + list-style-image: url("chrome://devtools/skin/images/add.svg"); Same icon as the create new rule of the ruleview. Should we file a follow up to find a better (different) icon?
Thanks Julian. Some replies inline below. (In reply to Julian Descottes [:jdescottes] from comment #7) > ::: devtools/client/inspector/inspector-panel.js:458 > (Diff revision 1) > > + canAddHTMLChild: function() { > > + return this.selection.isHTMLNode() && > > + this.selection.isElementNode() && > > + !this.selection.isPseudoElementNode() && > > + !this.selection.isAnonymousNode(); > > + }, > > Some edge cases to review : > - feature enabled when <html> is selected, but does nothing > - feature enabled when <iframe> is selected, but does nothing > - can add <div>s in <head> > - can add <div>s in <textarea>, <script> (but they are added as text) As discussed on IRC, I added checks for HTML and IFRAME because inserting in these elements has no visible effect. But I left it enabled for other elements such as head, textarea, link, style, etc. because doing $0.appendChild in the console works, so there isn't a real reason to prevent users from doing it here too. > ::: devtools/client/inspector/test/browser_inspector_addNode_02.js:46 > (Diff revision 1) > > + let btn = doc.querySelector("#inspector-element-add-button"); > > + let item = doc.querySelector("#node-menu-add"); > > + > > + // Force an update of the node-menu first, since it normally only updates on > > + // popupshown. > > + inspector._setupNodeMenu({target: {}}); > > This makes the test simpler but is a bit hacky. I guess the only alternative > is to actually open the context menu to trigger an update? > > I think it's best to avoid using "private" methods, even in tests. Given > that the alternative is painful to implement, up to you :) I agree, and I spent some time implementing an alternative solution where I'd open/close the menu instead. This turned out to be more complex than I thought and involved a lot more code, so I ended up reverting it and adding a comment instead. I hope it's alright. > ::: devtools/client/inspector/test/browser_inspector_addNode_03.js:47 > (Diff revision 1) > > + let onMutation = inspector.once("markupmutation"); > > + btn.click(); > > + let mutations = yield onMutation; > > + > > + // Expect an inspector-updated event right after the mutation event. > > + yield inspector.once("inspector-updated"); > > Any risk we could miss the "inspector-updated" event here? > Why not add the listener before the click, do we expect several > inspector-updated events? This shouldn't be a problem. We specifically want to listen for the markupmutation event first, to ensure the insertion worked, and then wait for the inspector-updated event which, we know, is triggered after, when the new node is selected. > ::: devtools/client/themes/inspector.css:44 > (Diff revision 1) > > } > > > > +/* Add element toolbar button */ > > + > > +#inspector-element-add-button { > > + list-style-image: url("chrome://devtools/skin/images/add.svg"); > > Same icon as the create new rule of the ruleview. Should we file a follow up > to find a better (different) icon? Yeah maybe. Right now, I see this icon being used in these locations: devtools\client\shared\widgets\filter-widget.css:239 in .add {} devtools\client\themes\inspector.css:44 in #inspector-element-add-button {} devtools\client\themes\rules.css:417 in #ruleview-add-rule-button::before {} What do you mean by finding a better icon? Do you mean that we should put these in behind a common class that we can easily reuse in all places?
(In reply to Patrick Brosset [:pbro] from comment #8) > > > ::: devtools/client/themes/inspector.css:44 > > (Diff revision 1) > > > } > > > > > > +/* Add element toolbar button */ > > > + > > > +#inspector-element-add-button { > > > + list-style-image: url("chrome://devtools/skin/images/add.svg"); > > > > Same icon as the create new rule of the ruleview. Should we file a follow up > > to find a better (different) icon? > Yeah maybe. Right now, I see this icon being used in these locations: > > devtools\client\shared\widgets\filter-widget.css:239 in .add {} > devtools\client\themes\inspector.css:44 in #inspector-element-add-button {} > devtools\client\themes\rules.css:417 in #ruleview-add-rule-button::before {} > > What do you mean by finding a better icon? Do you mean that we should put > these in behind a common class that we can easily reuse in all places? My only concern is that the ruleview is displayed at the same time as the inspector toolbar. So we have the exact same "+" icon displayed twice on the same screen, with two different behaviors. Feel free to ignore this.
(In reply to Julian Descottes [:jdescottes] from comment #9) > My only concern is that the ruleview is displayed at the same time as the > inspector toolbar. So we have the exact same "+" icon displayed twice on the > same screen, with two different behaviors. Feel free to ignore this. Oh I see. I originally thought that having the same icon would actually be nice from a consistency stand point. But I see the concern. I'll check up with Helen.
(In reply to Patrick Brosset [:pbro] from comment #10) > (In reply to Julian Descottes [:jdescottes] from comment #9) > > My only concern is that the ruleview is displayed at the same time as the > > inspector toolbar. So we have the exact same "+" icon displayed twice on the > > same screen, with two different behaviors. Feel free to ignore this. > Oh I see. I originally thought that having the same icon would actually be > nice from a consistency stand point. But I see the concern. I'll check up > with Helen. I agree with Patrick—I think the double '+' isn't an issue.
Depends on: 1262491
https://hg.mozilla.org/mozilla-central/rev/963e2caee42c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I've added a bit on this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Inserting_new_nodes I think the "Create New Node" popup menu item does the same thing, is that right? I updated the docs for that, too, in the belief that it does.
Flags: needinfo?(pbrosset)
Also I wondered, did you consider popping open the "Edit as HTML" panel when we insert a new node like this, so the user can set the node's content right away?
(In reply to Will Bamberg [:wbamberg] from comment #14) > I've added a bit on this: > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/ > Examine_and_edit_HTML#Inserting_new_nodes Thanks, the video looks great. > I think the "Create New Node" popup menu item does the same thing, is that > right? I updated the docs for that, too, in the belief that it does. Yes that's right, thanks for including this in the docs too. (In reply to Will Bamberg [:wbamberg] from comment #15) > Also I wondered, did you consider popping open the "Edit as HTML" panel when > we insert a new node like this, so the user can set the node's content right > away? No I did not consider this, interesting. I did consider other ideas though: - having an intermediate popup to choose the tag name and attributes before inserting the node - or switching the node's tagname to edit mode right away so you change it or press TAB to add attributes. But none seemed quite right, we can't know what users are going to want to do with the node, some might want to change the tagname, some might want to add a class for css styling, some might want to add HTML content, and some might be just fine with a simple <div>. So in the end I just decided to leave the element as is and let users enter edit mode themselves. I think we need more user feedback here.
Flags: needinfo?(pbrosset)
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: