Closed Bug 659710 Opened 9 years ago Closed 8 years ago

[htmltree] full structured editing

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: dangoor, Assigned: getify)

References

Details

Attachments

(1 file, 1 obsolete file)

This bug may replace bug 575234 or may also wind up being a meta bug.

This bug is for the implementation of the HTML Tree editing feature:

https://wiki.mozilla.org/DevTools/Features/HTMLTreeEditor
We'd like to get some UX input on this prototype:

http://test.getify.com/mozilla/html-inline-edit.html

Here's a quick screencast showing the various features of this inline HTML editing:

http://www.screenr.com/FZj

Basically, our goal is to allow a user to edit an entire attribute list all at once, in addition to the more structured add/edit/delete per-attribute UX that is common in something like Firebug. Editing an entire attribute list at once allows you to change multiple attributes at the same time, add, delete, and even rename them, as well as modify their values, but it keeps you still within the context of only that one node, as opposed to the fully free-form editing of the whole DOM.
Keywords: uiwanted
I think you can and should break this into separate bugs. We can add editing support to the individual attributes' contents and worry about the +/- UI separately. That should unblock you and allow us to get some very simple editing of attributes started.
My first instinct is that it seems weird to have a floating editing UI on top of the existing UI instead of just dropping you into the text itself with the element pre-selected.

Also, the hovering deletion functionality seems a bit scary, and easy to trigger by accident. Especially since clicking might mean delete or edit, and you have to parse the icons to figure out which it is.

I would let editing happen in-place (pre-selecting the element/text is cool, though), and let delete be something like a right-click context option or something that is sufficiently different from the editing operation.
The right-click context menu items for adding/renaming/deleting attributes is probably one of the most frustrating UX-wise things for me in daily usage of Firebug, which is why I strongly felt like we needed some more streamlined approach, and the icon hovering UI metaphor seemed appropriate. Do you have any other suggestions (other than context menu) that could address this? I had considered perhaps having a "toolbar" with the icons, that was context sensitive to which item you highlight as active. It's a little more cumbersome, but certainly more streamlined than hiding things in a context menu.

I should have mentioned that I absolutely believe, especially for delete, that there needs to be an "undo" button (at least the "undo last operation" and possibly even an undo stack of the last 10 or so actions, or something like that). Do you think having "undo" would mitigate the scariness of the accidental delete? Or would having a quick yes/no prompt to confirm the delete be helpful?

The reason for the editing UI to be floating above (offset below) is so that you can see the original attribute list as you're working on the edited copy, meaning that you haven't "committed" the change until you dismiss the editing box.

When you're editing only a single value, it's not so much an issue, but I really felt like if you click to edit a long attribute list all at once, and you start to make some changes to various attributes, being able to have a reference point of the "original" text is helpful, especially if you accidentally mis-type or delete something you didn't intend to, so you can see visually right above the input box what was there before and correct your mistake. 

When there's both the original and the being-edited copy of text, it seems to lessen the feeling of risk while editing, because you know you can always hit escape to cancel the hover-edit and revert to what it was before. But that type of metaphor is a lot less common with in-place editing mechanisms, and I think is less likely to be an instinctual action.
Deletion, undo and additional behavior should really be handled in follow-up bugs. We can get basic editing (which includes undo) without worrying about all of that up front. Seriously.
The "browsing" logic here is: "highlight all the attribute", then "highlight the value of one attribute".

There's no "highlight the pair attribute+value" step, and this is quite confusing because expected.

I'm not sure if highlighting attribute+value is actually useful or even possible (would highlighting all the attribute be possible then?). But at least, it would make clear that the delete button would delete just this attribute, not all the attributes. Maybe just a separator.
(in reply to comment #5)

Rob-
I heard and understand your assertion that the +/- should be handled separately from the editing. However, I don't agree that it's entirely as clean a separation as you make it out to be.

Regardless, this is not a "blocker" at the moment, as you asserted, because I have plenty of other stuff I'm working on here. I don't think it's even been officially decided at all what features are in or out. So I think it's premature to start suppressing productive discussions of the UX of features. The whole purpose of the prototype was to solicit feedback on all the subtle points of the UX.

The whole concept of the + was invented during my work on the prototype because I quickly realized that if you highlight an entire attribute list, and (intentionally or accidentally) hit the delete button to remove the whole list, as soon as you dismiss the edit popup and "commit" that change, now you've done a 1-way action that you can't get back, because now you have no attribute list to edit and add to.

So, the frustrating nature of that 1-way scenario, especially as easy as it is to do accidentally, dictates that either an `undo` or a + should be present. I don't feel like the feature would be responsible if it left obvious traps like that for users.

Now, is - strictly required by that argument? No. But it's also no extra work really, and it maintains feature symmetry (both + and -). It also addresses a pain point that Kevin and I identified when we were first talking about this project, which is that to delete an attribute (somewhat common task, actually) you have to go through the slower/awkward context-menu flow.

I like Paul's suggestion of having the "hover" over the minus also tell you that it will apply to the whole attribute/value, but only that attr, and not the whole attribute list. At first glance, I'd probably do that with a thin border around the attribute and value, rather than with the color overlay, since the color overlays right now indicate editability.
(in reply to comment #6)

> I'm not sure if highlighting attribute+value is actually useful or even possible (would highlighting all the attribute be possible then?). 

There's a subtle distinction to be made here, and that is that just because you can see the - icon visible doesn't mean that a click at that point will do the delete. I specifically made it so that the - icon is visible when you hover over any part of the attribute (easier discoverability that such a feature exists), but you have to intentionally move your mouse directly over the minus to get it to do the delete. If you simply click anywhere in the blue, but not directly on the icon, it does the whole attr-list edit, as you noted.

SO, to satisfy your suggestion of highlighting the entire attribute+value for a delete action, I'd simply have it that the thin border only showed up when you are directly over the delete button. That would further reinforce that the delete icon is merely visible, but not active, unless you are directly over just the delete icon when you click.
Is there a patch for this? I would be happy to do feedback or review
FWIW, my last coding task (atm) against the independent prototype is to finish "phase 2" which was to hook the code up to manipulating (and indeed, now leveraging for attr-list parsing) the DOM, as opposed to just strings. Once that is complete, the very next step is to move the code into the actual HTML tree pane in the firefox codebase.

As I've said, I think it's healthy and good for the UX discussions around the finer points of features to continue in parallel -- the outcomes of which thankfully are not at all blocking me (yet).
Paul (& others)--
I added the outline hover effect (and some transparency on the - icon) for the attribute delete. Please check the prototype again (make sure to do a full refresh) to see if that helps at all to address your concerns about that part of the UX. I'm pleased that I think it's a helpful improvement.
Here's an updated screencast showing the additional "alpha effect" on the delete icon, as well as the green-border highlighting of the attr+val when hovering directly on the delete icon.

http://www.screenr.com/QkTs
I think the delete buttons are too small, and yet still manage to obscure a good chunk of the attribute name. Also, having a delete UI like this one without a means of undoing it (short of reloading the page) is problematic.

The editing is ok, except it's a little confusing when a user clicks on an attribute and gets the whole set of attributes for the tag or inside the attribute value and gets just the one. I could see that being error-prone, though not really a deal-breaker. I also agree with Limi that editing in situ would be preferable to another popup.

How about default behavior is to click inside the attributes, get a full edit of all of them and do away entirely with the fiddly +/- buttons?

Also, I'm not trying to put a stop to "productive UI feedback". I'm trying to encourage you to get a patch against our actual codebase up instead of more prototyping that isn't going to really help you when it comes to the actual implementation. Review time is precious. We need to make use of it. The earlier the better.

Let me know if you need pointers to where the code lives. Start in:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/inspector.js

and see also,

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/domplate.jsm
Status: NEW → ASSIGNED
(in reply to comment #13)

> I think the delete buttons are too small, and yet still manage to obscure a good chunk of the attribute name.

I've now tweaked the position (not it's offset below and to the right) of the delete button and i think it now is a bit less obscuring and a bit more noticeable/easy-to-use. you?


> Also, having a delete UI like this one without a means of undoing it (short of reloading the page) is problematic.

Agreed. All along I've said there will need to be an undo. Since the undo won't be an inline thing for the UX, it was outside the scope of what I was researching with this prototype. But, it will certainly need to be there in the chrome of the panel (or some toolbar or something).


> do away entirely with the fiddly +/- buttons?

As I have asserted before, without a +, the UX is unacceptable in my opinion (in that it falls short of the basic reason for this redesign from the firebug UI), because empty tags (those without any attributes) can't be added to without the clunky context-menu (the express goal of this prototype is to eliminate the majority need for those menu items).

Also, without a +, the trap is that if you delete the attribute list entirely (on purpose or on accident), you are in the same dead-end situation as an original empty/no-attr element. Undo would only solve part of this problem, if you undid such a deletion right away, but if you've edited something else and now want to add an attribute back, you're out of luck (undo is insufficient).

I'm not convinced the hover delete icon will end up working. But I *am* convinced that I absolutely dislike with a strong flavor the right-click-context-menu-delete flow. So either there needs to be a hover delete icon, or there needs to be some toolbar where I can one-click delete an attribute. At the moment, I think the - hover icon is the best solution; just its correct positioning/styling is still under consideration and tweaking.


> I'm trying to encourage you to get a patch against our actual codebase up instead of more prototyping

I'm not really actively "prototyping" at this point, other than 2 minute CSS tweaks here and there, or quickly fixing a bug if I find it in my other work.

I am actively working on fitting the phase1 prototype code with DOM manipulation logic (aka phase2 -- replacing many of the regexes with DOM code), so that it's suitable to drop into the actual browser code base (and not just deal with strings and regex's).

So, while I understand your concern about time being "wasted", what I am trying to explain to you is that the efforts to explore and tweak the UX of the phase 1 prototype are not deterring me from the DOM manipulation work of phase 2, nor are they the source of any delays in getting patches against the browser code-base.


> that isn't going to really help you when it comes to the actual implementation

Actually, the express goal of what I'm doing in phase2 is getting the code that was written for the prototype ready to drop in (at least as starting point code) to the browser code base. I'd say that's absolutely time well spent (both phase1 and phase2), that will help with the implementation. I have no intention of starting from scratch and not using the code/algorithms from the prototype in the final implementation -- that would clearly have made most of this effort to date a waste.
Status: ASSIGNED → NEW
(In reply to comment #14)
> (in reply to comment #13)

> I've now tweaked the position (not it's offset below and to the right) of
> the delete button and i think it now is a bit less obscuring and a bit more
> noticeable/easy-to-use. you?

The positioning is better, but on shorter attribute/element names (<p>, or cols= for example) I actually can't really see the icon underneath my mouse pointer :(

> Also, without a +, the trap is that if you delete the attribute list
> entirely (on purpose or on accident), you are in the same dead-end situation
> as an original empty/no-attr element. Undo would only solve part of this
> problem, if you undid such a deletion right away, but if you've edited
> something else and now want to add an attribute back, you're out of luck
> (undo is insufficient).

Could just include some blank space in empty elements that is clickable like the normal attribute list?  Might not be quite as easily discoverable, but if it highlights/changes the cursor it should be fairly obvious (and we can still include the context menu for people that really don't see it?) 

<p [ ]>

> I'm not convinced the hover delete icon will end up working. But I *am*
> convinced that I absolutely dislike with a strong flavor the
> right-click-context-menu-delete flow. So either there needs to be a hover
> delete icon, or there needs to be some toolbar where I can one-click delete
> an attribute. At the moment, I think the - hover icon is the best solution;
> just its correct positioning/styling is still under consideration and
> tweaking.

Maybe (this was brought up earlier in another context) instead of one-click delete, we could use a one-click "edit the attribute/value as a whole".  A click/delete key/enter triplet seems pretty reasonable for a destructive operation.  And I have spent hours editing html and never felt like I needed a one-click delete to get rid of an attribute, editing it out was just fine.

If it were me, I'd build a "click on the attribute list to edit it as text" UI in the browser as a first step.  I'd use padding in blank elements to avoid needing a + button.  This would let us get the least controversial part of this prototype shipping to our nightly users as soon as possible.  Everybody (I think?) can agree on at least that behavior, so let's get it in there.  Then I'd file a followup bug for editing of individual attributes, and one for quick attribute deletion where we can hash out the details.  This seems like the best way to balance continued UI feedback against measurable forward progress.
(In reply to comment #15)
> (In reply to comment #14)
> If it were me, I'd build a "click on the attribute list to edit it as text"
> UI in the browser as a first step.  I'd use padding in blank elements to
> avoid needing a + button.  This would let us get the least controversial
> part of this prototype shipping to our nightly users as soon as possible. 
> Everybody (I think?) can agree on at least that behavior, so let's get it in
> there.  Then I'd file a followup bug for editing of individual attributes,
> and one for quick attribute deletion where we can hash out the details. 
> This seems like the best way to balance continued UI feedback against
> measurable forward progress.

(yes, this is basically the exact thing robcee has suggested.  I hope you'll reconsider your disagreement with him given new information presented here).
(in reply to comment #15)

> Could just include some blank space in empty elements that is clickable like the normal attribute list?  Might not be quite as easily discoverable, but if it highlights/changes the cursor it should be fairly obvious (and we can still include the context menu for people that really don't see it?) 

I think that's a great idea, and simplifies things (removes the + icon people have been complaining about). It was a 15-second tweak to the CSS. I think it's a lot better now. You?


> Maybe (this was brought up earlier in another context) instead of one-click delete, we could use a one-click "edit the attribute/value as a whole".

I am not sure how we could accomplish both the "edit the whole attribute list at once" and "edit only a single attribute+value at a time" in the same hover UI, because the hit-area for the former would be almost entirely swallowed by the collective hit-areas for the latter.

One of the main goals of this new UX exploration was to get the "edit the whole attribute list at once" in there, so I'm a bit concerned that we'd be losing that goal in favor of another goal, one which I think is a little less important.


> If it were me, I'd build a "click on the attribute list to edit it as text" UI in the browser as a first step.  I'd use padding in blank elements to avoid needing a + button.  This would let us get the least controversial part of this prototype shipping to our nightly users as soon as possible.  Everybody (I think?) can agree on at least that behavior, so let's get it in there.  Then I'd file a followup bug for editing of individual attributes, and one for quick attribute deletion where we can hash out the details.  This seems like the best way to balance continued UI feedback against measurable forward progress.

> ... I hope you'll reconsider your disagreement with [Rob] ...

As far as I can tell, this is exactly how I'm proceeding (this stuff we're discussing is decidedly not the blocker/long-poll), so I'm still not sure I fully understand your (and Rob's) concerns?

I've made a number of tweaks to the styling per suggestions/concerns in this thread, which all-told I've spent maybe 20 minutes tops on, and I think I've addressed decently and fairly most of the issues raised. Delete-attr seems like it's the last issue that has non-trivial unfinished business. 

Well, that and where to place the popup edit box... which I think could very trivially be a preference so each dev can choose where they want it placed. I for one really prefer to be able to see the original text while I'm typing, so having the offset-position is quite attractive to me. Others may feel opposite. Since it's driven almost entirely by subjective opinion, and nothing more than a single property rule in CSS, let's just let devs choose!?

I've asserted several times that I think the delete-attr feature (sans context-menu flow) is important (a fundamental goal of mine for this tool) for final shipping, **but I've never said that it has to be, or will be, in the same patch**. In fact, I think the decision of what it's and what's out is still up in the air. And the lack of decision (yet) on that hasn't (yet) slowed me down in any way.

Moreover, whether delete-attr hover in (visible) or out (hidden) is literally a one-line CSS toggle, and the associated code for it is a very minor 4-5 lines. So I think the repeated concerns over whether it's in, or when it's in, is being a tiny bit over-emphasized compared to its triviality of implementation. But that's just my 2 cents.
(in reply to comment #17)

I should point out that actually physically taking the CSS and code out that affects/generates the DOM elements for the delete-attr hover is a bit trickier/riskier than just turning the hover off with CSS, which is what I mean by CSS toggle and "visible" vs. "hidden". 

In my opinion, we should only take that code out once we finally have decided that the delete-attr hover is being nixed (I hope not) altogether.

For the initial patch, I think it makes sense to just have it hidden so as to keep the simplicity of the patch apparent (and suppress it as a point of contention), and then the follow-up bug will argue over either unhiding/tweaking it, or removing it entirely.
Security triage has nominated this for security review
I'd love to have any feedback anyone may have on this work-in-progress patch.
BTW, I'm only looking for code comments at the moment... the blocking bugs I'm waiting on make this code somewhat non-functional, if you try to actually run it.
Depends on: 668437
Depends on: 655297
About the delete button:

Since there is a "delete" button, the user will look for the "add" button.
Also, the "delete" button forces us to add a border around the attribute/value pair. The 'button' + 'border' bring a lot of complexity to the UI for a non-primary usage.

An alternative approach would be to select the attribute/value in the textbox once the attribute is selected. Hitting the Delete key then the Enter key would immediately remove the attribute. The behavior is a natural behavior and won't disturb the user.

This is not optimal because we have to hit 2 keys to achieve this, but I think it could be a good compromise.
Comment on attachment 544813 [details] [diff] [review]
work in progress on the html inspector

A couple of things:

1. you've replaced the existing tree code.
2. the tree implementation doesn't work on OS X.
3. more questions inline:

+<!DOCTYPE html>
+<html xmlns="http://www.w3.org/1999/xhtml">
 <head>
   <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
-  <link rel="stylesheet" href="chrome://browser/skin/inspector.css" type="text/css"/>
 </head>
-<body role="application">
-</body>
+<body role="application"></body>

Why these changes?

+      //this.treeIFrame.setAttribute("onclick", "InspectorUI.onTreeClick(event)");

presumably this won't be sticking around.

...

     if (this.treeIFrame)
       delete this.treeIFrame;
-    delete this.ioBox;
 
     if (this.domplate) {
       this.domplateUtils.setDOM(null);

the domplate checks can presumably be removed too if you're going to rip all this out.

+  var _this = this;

in the inspectorTree.js file.

+
+function InspectorTree(docElement, treeElement) {
+  var _this = this;

use let instead of var in firefox code. You should also have a javadoc comment preceding the function declaration explaining what this function object is for. What it's parameters are and what they're used for. We also typically use parameters with an a prefix (denoting argument). Makes them easier to spot inside the function.

+  // setup event handlers  
+  // handle all clicks on any part of the tree element
+  this.treeElement.addEventListener("click", function(evt){
+    var targetClassname = evt.target.className || "";

Space between function(evt) and {

+// static "constant"
+InspectorTree.selfClosingTags = {
+  "meta": 1,
+  "link": 1,
+  "area": 1,
+  "base": 1,
+  "col": 1,
+  "input": 1,
+  "img": 1,
+  "br": 1,
+  "hr": 1,
+  "param": 1,
+  "embed": 1
+};

this looks familiar...

...

+  // render out the DOM elements for a line based on the metadata structure for the line in question
+  renderLine: function(lineEntry)

these all need javadoc comments.

+  {
+    
+    // render hover for a tag with no attribute-list
+    function renderTagNoAttributesHover()
+    {
+      // `el4` is defined in the parent loop
+      var el5;

uh oh...

...

+    // render hovers for an attribute-list
+    function renderAttributeListHover()
+    {
+      // `el2` is defined in the parent loop
+      // `el4` is defined in the parent loop
+      var el5;
+      var el6;
+      var el7;
+      var tmpAttr;
+      var tmpAttrList;
+      var attrParts;
+      // `j` is defined in the parent loop
+      var k;
+      var m;

this just got very confusing. And because your variables are all operating inside inner function scopes it's going to be pretty hard to make sense of what's coming from where.

+    var el;
+    var el2;
+    var el3;
+    var el4;
+    var syntaxElements;
+    var j;
+    var k;
+    var targetElement;

what are all these?

I might be able to decode what this section of code does without comments, but I really don't want to. The cure may be worse than the disease here.

I'm going to stop here for now and make a couple of comments:

This bug's summary is "add attribute editing". This patch is a whole lot more than that.

Despite the apparent complexity of DOMPlate it isn't *that* much harder to understand than what's here. Further, the resultant HTML tree is manipulable if you take a look at it. It should be possible to bolt on basic attribute editing behavior to what's in place.
Attachment #544813 - Flags: review-
Whiteboard: [minotaur]
fixed some accidental mis-match in CSS and markup
Attachment #544813 - Attachment is obsolete: true
Depends on: 673468
OS: Mac OS X → All
Hardware: x86 → All
Summary: [htmltree] add attribute editing → [htmltree] full structured editing
Leaving a note to clarify the newly agreed scope for this bug:

Full structured editing for the HTML panel includes:
* attribute-list editing (all attributes at once)
* attribute value editing
* attribute name editing (rename)
* attribute adding (including adding attributes to a node with no attribute-list yet)
* attribute deleting
* text node editing (including adding text back to an empty text node)

Things not included in this type of editing include (would require full *unstructured* editing):
* adding new text nodes where they do not yet exist
* adding new DOM nodes where they do not yet exist
* deleting DOM nodes entirely
* deleting text nodes entirely
* changing the type of node (changing a <div> to a <span>, etc)
Status: NEW → ASSIGNED
Comment on attachment 547184 [details] [diff] [review]
updated patch for UX overhaul of html panel

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

::: browser/base/content/inspectorTree.js
@@ +50,5 @@
> +  
> +  this.mapEntryToTargetDOM = new WeakMap();
> +  this.mapTargetDOMToTreeElement = new WeakMap();
> +  this.mapTreeElementToEntry = new WeakMap();
> +  

mapEntryToTargetDOM and mapTreeElementToEntry aren't buying you anything here that a simple object property wouldn't get you.
Whiteboard: [minotaur]
I don't think we're going to use this solution. Reopen if someone wants to take this on.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.