Open Bug 1067323 Opened 10 years ago Updated 2 years ago

[markup view] Getting out of "edit as HTML" mode isn't great

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: pbro, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [devtools-ux])

Attachments

(1 file, 6 obsolete files)

While discussing in bug 1067318, I realized that our current mechanism for editing HTML in the markup-view, and more specifically for committing the changes isn't perfect.

Cmd/Ctrl+ENTER is the best way to do this because it retains node selection and doesn't force the user to go back to using the mouse, but it's not discoverable at all.

Another way is to click outside the editor, but this is bad because it selects another node.

Firebug currently switches the whole markup-view into the editor and adds an "Edit" button in the toolbar that can be used to go out of edit mode.
Blocks: firebug-gaps
Note that in Firebug you can press Cmd/Ctrl+E[1] to switch to the edit mode.
Switching the whole markup view has its disadvantages and we (the Firebug Working Group) actually wanted to switch to an inline editor[2] like the devtools have it.

So to improve the UX I believe it would be the best if you just added a button to toggle the edit mode like Firebug has it and keep the rest as it is.
If you want to add full-viewport editing, I suggest you add another toggle button for that similar to what GitHub has for comments.

Sebastian

[1] https://code.google.com/p/fbug/issues/detail?id=6708
[2] https://code.google.com/p/fbug/issues/detail?id=4618
(In reply to Sebastian Zartner from comment #1)
> So to improve the UX I believe it would be the best if you just added a
> button to toggle the edit mode like Firebug has it and keep the rest as it
> is.

Yeah, I think this would be better than going into a full-page editing mode.  It is helpful to see the surrounding nodes.  

I believe I had a WIP patch at one point with a 'finish' and 'cancel' button but never wrapped it up.  Any buttons should probably be added to the html-editor component, and just have a 'finish' button call this.hide() while a 'cancel' button calls this.hide(false): http://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/html-editor.js#156
Whiteboard: [devtools-ux]
Inspector Bugs Triage - Filter on CLIMBING SHOES
Priority: -- → P3
(In reply to (Unavailable until Apr 3) [:bgrins] from comment #2)
> (In reply to Sebastian Zartner from comment #1)
> > So to improve the UX I believe it would be the best if you just added a
> > button to toggle the edit mode like Firebug has it and keep the rest as it
> > is.
> 
> Yeah, I think this would be better than going into a full-page editing mode.
> It is helpful to see the surrounding nodes.  

Sometimes it's helpful to see surrounding nodes, but sometimes it's not. It's important to differentiate the use-cases for HTML editing. 

If you just want to make minor edits (e.g. change a text node, or change an attribute value) then the inline editor is fine and seeing surrounding nodes is helpful. 

But if you're doing "major surgery" editing, like prototyping a multi-node piece of UI like a form or table, then seeing surrounding nodes is quite distracting because you have to avoid clicking on them, because they rob you of real estate you can use for your editor pane, and because you need to worry about scrolling your editor viewport into view when the node(s) you're editing are wider and/or taller than the dev tools pane. 

Personally, I found Firebug's editing model matched these two use-cases really well. There was an inline editor for minor edits, but when you wanted to do serious editing, you could toggle into Edit Mode with a full-pane editor. For me it was a feature not a bug that these two modes had very different UX because what I was trying to achieve in each case was also very different. 

Finally, the Firebug editor did allow you to move around in the node hierarchy, even when you were in Edit mode, by clicking on one of the nodes in the node hierarchy. But honestly I didn't use this feature very often-- instead, when I wanted to move to another node, I would toggle out of Edit mode, select the other node, and go back into Edit mode.
(In reply to Justin Grant from comment #4)
> (In reply to (Unavailable until Apr 3) [:bgrins] from comment #2)
> > (In reply to Sebastian Zartner from comment #1)
> > > So to improve the UX I believe it would be the best if you just added a
> > > button to toggle the edit mode like Firebug has it and keep the rest as it
> > > is.
> > 
> > Yeah, I think this would be better than going into a full-page editing mode.
> > It is helpful to see the surrounding nodes.  
> 
> Sometimes it's helpful to see surrounding nodes, but sometimes it's not.
> It's important to differentiate the use-cases for HTML editing. 
> 
> If you just want to make minor edits (e.g. change a text node, or change an
> attribute value) then the inline editor is fine and seeing surrounding nodes
> is helpful. 
> 
> But if you're doing "major surgery" editing, like prototyping a multi-node
> piece of UI like a form or table, then seeing surrounding nodes is quite
> distracting because you have to avoid clicking on them, because they rob you
> of real estate you can use for your editor pane, and because you need to
> worry about scrolling your editor viewport into view when the node(s) you're
> editing are wider and/or taller than the dev tools pane.

All valid points. Thank you for pointing that out!

So, maybe my idea of comment 1 to add a maximize button (and maybe also a shortcut for it) wasn't that bad... :-)

> Finally, the Firebug editor did allow you to move around in the node
> hierarchy, even when you were in Edit mode, by clicking on one of the nodes
> in the node hierarchy. But honestly I didn't use this feature very often--
> instead, when I wanted to move to another node, I would toggle out of Edit
> mode, select the other node, and go back into Edit mode.

Also a good point. I assume that most users (like you) didn't use this feature often, though. And I'd say it's out of the scope of this bug. So, if you think it should be possible within the DevTools, I suggest you open a new bug, CC me and mark it as blocker of bug 991806.

Sebastian
Assignee: nobody → sheldon.roddick
Status: NEW → ASSIGNED
Attached patch Bug-1067323.patch v1 (obsolete) — Splinter Review
Attachment #8839740 - Flags: review?(gl)
Comment on attachment 8839740 [details] [diff] [review]
Bug-1067323.patch v1

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

Couple of issues:
- You will find that clicking on the toolbar itself will cause it to hide. You need to rebind the click event to the entire container. 
- Mousewheel scrolling doesn't seem to work anymore on my testing as well.

I think this gets us closer to what we want to have, which is a "Edit mode" button that will toggle this inline editor to be take up the full HTML markup view's space.

::: devtools/client/inspector/markup/views/html-editor.js
@@ +31,5 @@
> +  this.toolbar = this.doc.createElement("div");
> +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> +
> +  this.toolbarCommitButton = this.doc.createElement("button");
> +  this.toolbarCommitButton.innerHTML = "Commit";

We need to localize these strings. I think you can just add them to inspector.properties. 
s/innerHTML/textContent

@@ +34,5 @@
> +  this.toolbarCommitButton = this.doc.createElement("button");
> +  this.toolbarCommitButton.innerHTML = "Commit";
> +  this.toolbarCommitButton.addEventListener("click", this.hide);
> +
> +  this.toolbarCancelButton = this.doc.createElement("button");

We should style these buttons according to how other buttons are styled in the toolbox. I think this.toolbarCancelButton.className ="devtools-button" should do the trick.

@@ +35,5 @@
> +  this.toolbarCommitButton.innerHTML = "Commit";
> +  this.toolbarCommitButton.addEventListener("click", this.hide);
> +
> +  this.toolbarCancelButton = this.doc.createElement("button");
> +  this.toolbarCancelButton.innerHTML = "Cancel";

s/innerHTML/textContent

@@ +36,5 @@
> +  this.toolbarCommitButton.addEventListener("click", this.hide);
> +
> +  this.toolbarCancelButton = this.doc.createElement("button");
> +  this.toolbarCancelButton.innerHTML = "Cancel";
> +  this.toolbarCancelButton.addEventListener("click", this.hide.bind(this, false));

I think instead of this.hide.bind(this, false) just do () => this.hide(false) since we already bind hide below.

::: devtools/client/themes/markup.css
@@ +65,5 @@
>  
> +.html-editor-toolbar {
> +  border-width: 1px;
> +  /* Keep the editor away from the markup view floating scrollbard */
> +  -moz-margin-end: 12px;

Use margin-inline-end
Attachment #8839740 - Flags: review?(gl)
Attached patch Bug-1067323.patch v2 (obsolete) — Splinter Review
Attachment #8839740 - Attachment is obsolete: true
Attachment #8842350 - Flags: review?(gl)
Comment on attachment 8842350 [details] [diff] [review]
Bug-1067323.patch v2

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

::: devtools/client/themes/markup.css
@@ +72,2 @@
>  .html-editor-inner {
>    border: solid .1px;

Can you remove this line?
Attachment #8842350 - Flags: review?(gl)
Attached patch Bug-1067323.patch v3 (obsolete) — Splinter Review
Attachment #8842350 - Attachment is obsolete: true
Attachment #8846526 - Flags: review?(gl)
Comment on attachment 8846526 [details] [diff] [review]
Bug-1067323.patch v3

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

::: devtools/client/inspector/markup/views/html-editor.js
@@ +30,5 @@
>    this.container.style.display = "none";
> +  this.toolbar = this.doc.createElement("div");
> +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> +
> +  this.toolbarCommitButton = this.doc.createElement("button");

It would be nice if you could use the devtools-button class

@@ +31,5 @@
> +  this.toolbar = this.doc.createElement("div");
> +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> +
> +  this.toolbarCommitButton = this.doc.createElement("button");
> +  this.toolbarCommitButton.innerHTML = "Commit";

We tend to prefer using textContent wherever we can, since it's more safe.

Also this string needs to be localized.

@@ +32,5 @@
> +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> +
> +  this.toolbarCommitButton = this.doc.createElement("button");
> +  this.toolbarCommitButton.innerHTML = "Commit";
> +  this.toolbarCommitButton.addEventListener("click", this.hide.bind(this, true));

You should make sure this code gets cleaned properly in the destroy() function: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/views/html-editor.js#160

@@ +35,5 @@
> +  this.toolbarCommitButton.innerHTML = "Commit";
> +  this.toolbarCommitButton.addEventListener("click", this.hide.bind(this, true));
> +
> +  this.toolbarCancelButton = this.doc.createElement("button");
> +  this.toolbarCancelButton.innerHTML = "Cancel";

Same comments apply here

::: devtools/client/themes/markup.css
@@ +67,5 @@
>  
> +.html-editor-toolbar {
> +  border-width: 1px;
> +  /* Keep the editor away from the markup view floating scrollbard */
> +  -moz-margin-end: 12px;

margin-inline-end
(In reply to Tim Nguyen :ntim from comment #11)
> Comment on attachment 8846526 [details] [diff] [review]
> Bug-1067323.patch v3
> 
> Review of attachment 8846526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/inspector/markup/views/html-editor.js
> @@ +30,5 @@
> >    this.container.style.display = "none";
> > +  this.toolbar = this.doc.createElement("div");
> > +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> > +
> > +  this.toolbarCommitButton = this.doc.createElement("button");
> 
> It would be nice if you could use the devtools-button class
> 
> @@ +31,5 @@
> > +  this.toolbar = this.doc.createElement("div");
> > +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> > +
> > +  this.toolbarCommitButton = this.doc.createElement("button");
> > +  this.toolbarCommitButton.innerHTML = "Commit";
> 
> We tend to prefer using textContent wherever we can, since it's more safe.
> 
> Also this string needs to be localized.
> 
> @@ +32,5 @@
> > +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> > +
> > +  this.toolbarCommitButton = this.doc.createElement("button");
> > +  this.toolbarCommitButton.innerHTML = "Commit";
> > +  this.toolbarCommitButton.addEventListener("click", this.hide.bind(this, true));
> 
> You should make sure this code gets cleaned properly in the destroy()
> function:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> markup/views/html-editor.js#160
> 
> @@ +35,5 @@
> > +  this.toolbarCommitButton.innerHTML = "Commit";
> > +  this.toolbarCommitButton.addEventListener("click", this.hide.bind(this, true));
> > +
> > +  this.toolbarCancelButton = this.doc.createElement("button");
> > +  this.toolbarCancelButton.innerHTML = "Cancel";
> 
> Same comments apply here
> 
> ::: devtools/client/themes/markup.css
> @@ +67,5 @@
> >  
> > +.html-editor-toolbar {
> > +  border-width: 1px;
> > +  /* Keep the editor away from the markup view floating scrollbard */
> > +  -moz-margin-end: 12px;
> 
> margin-inline-end

I don't think I have much more to add since most of what ntim said is what I have mentioned in my previous review in comment 7. It seems somehow those changes have been lost.
Attachment #8846526 - Flags: review?(gl)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #12)
> (In reply to Tim Nguyen :ntim from comment #11)
> > Comment on attachment 8846526 [details] [diff] [review]
> > Bug-1067323.patch v3
> > 
> > Review of attachment 8846526 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: devtools/client/inspector/markup/views/html-editor.js
> > @@ +30,5 @@
> > >    this.container.style.display = "none";
> > > +  this.toolbar = this.doc.createElement("div");
> > > +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> > > +
> > > +  this.toolbarCommitButton = this.doc.createElement("button");
> > 
> > It would be nice if you could use the devtools-button class
> > 
> > @@ +31,5 @@
> > > +  this.toolbar = this.doc.createElement("div");
> > > +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> > > +
> > > +  this.toolbarCommitButton = this.doc.createElement("button");
> > > +  this.toolbarCommitButton.innerHTML = "Commit";
> > 
> > We tend to prefer using textContent wherever we can, since it's more safe.
> > 
> > Also this string needs to be localized.
> > 
> > @@ +32,5 @@
> > > +  this.toolbar.className = "html-editor-toolbar devtools-toolbar";
> > > +
> > > +  this.toolbarCommitButton = this.doc.createElement("button");
> > > +  this.toolbarCommitButton.innerHTML = "Commit";
> > > +  this.toolbarCommitButton.addEventListener("click", this.hide.bind(this, true));
> > 
> > You should make sure this code gets cleaned properly in the destroy()
> > function:
> > https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> > markup/views/html-editor.js#160
> > 
> > @@ +35,5 @@
> > > +  this.toolbarCommitButton.innerHTML = "Commit";
> > > +  this.toolbarCommitButton.addEventListener("click", this.hide.bind(this, true));
> > > +
> > > +  this.toolbarCancelButton = this.doc.createElement("button");
> > > +  this.toolbarCancelButton.innerHTML = "Cancel";
> > 
> > Same comments apply here
> > 
> > ::: devtools/client/themes/markup.css
> > @@ +67,5 @@
> > >  
> > > +.html-editor-toolbar {
> > > +  border-width: 1px;
> > > +  /* Keep the editor away from the markup view floating scrollbard */
> > > +  -moz-margin-end: 12px;
> > 
> > margin-inline-end
> 
> I don't think I have much more to add since most of what ntim said is what I
> have mentioned in my previous review in comment 7. It seems somehow those
> changes have been lost.

Looks like I may have made a mistake when rebasing this and generating the new patch. Addressing ASAP
Attached patch Bug-1067323.patch v4 (obsolete) — Splinter Review
Attachment #8846526 - Attachment is obsolete: true
Attachment #8847947 - Flags: review?(gl)
Comment on attachment 8847947 [details] [diff] [review]
Bug-1067323.patch v4

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

::: devtools/client/inspector/markup/views/html-editor.js
@@ +186,5 @@
>        this.refresh, true);
>      this.container.removeEventListener("click", this.hide);
>      this.editorInner.removeEventListener("click", stopPropagation);
>  
> +    this.toolbarCommitButton.removeEventListener("click", this.hide);

I'm not sure the listener is removed properly, as this.hide !== this.hide.bind(this, true);

@@ +187,5 @@
>      this.container.removeEventListener("click", this.hide);
>      this.editorInner.removeEventListener("click", stopPropagation);
>  
> +    this.toolbarCommitButton.removeEventListener("click", this.hide);
> +    this.toolbarCommitButton.textContent = "";

I don't think you need to clear the textContent, as removing the element takes care of that. The only important thing here is removing the elements and the event listeners.

@@ +189,5 @@
>  
> +    this.toolbarCommitButton.removeEventListener("click", this.hide);
> +    this.toolbarCommitButton.textContent = "";
> +    this.toolbarCancelButton.removeEventListener("click", this.hide);
> +    this.toolbarCancelButton.textContent = "";

Same.

@@ +194,5 @@
> +
> +    this.toolbar.removeEventListener("click", stopPropagation);
> +
> +    this.toolbar.removeChild(this.toolbarCommitButton);
> +    this.toolbar.removeChild(this.toolbarCancelButton);

this.toolbarCommitButton.remove();
this.toolbarCancelButton.remove();

@@ +196,5 @@
> +
> +    this.toolbar.removeChild(this.toolbarCommitButton);
> +    this.toolbar.removeChild(this.toolbarCancelButton);
> +
> +    this.editor.removeChild(this.toolbar);

You can just do this.toolbar.remove();
Attached patch Bug-1067323.patch v5 (obsolete) — Splinter Review
Attachment #8847947 - Attachment is obsolete: true
Attachment #8847947 - Flags: review?(gl)
Attachment #8848374 - Flags: review?(gl)
Attached patch Bug-1067323.patch v5a (obsolete) — Splinter Review
Sorry for the extra ping, missed a variable name change in the last patch
Attachment #8848374 - Attachment is obsolete: true
Attachment #8848374 - Flags: review?(gl)
Attachment #8848375 - Flags: review?(gl)
Attachment #8848375 - Attachment is obsolete: true
Attachment #8848375 - Flags: review?(gl)
Attachment #8851179 - Flags: review?(gl)
Comment on attachment 8851179 [details] [diff] [review]
Bug-1067323.patch v6

So, the height of the html editor isn't quite right. The html editor actually overlays on top of the markup and covers up the entire section of the markup that you are editing.

We can't actually set a max-height because it needs to cover the entire region the editor is editing. For instance, if you go to a page with lots of markup and start editing at the root <html> node, you would expect the html editor to cover the entire section that you are editing. The current max height means you can still scroll and see the underlying nodes you are editing.
Attachment #8851179 - Flags: review?(gl)
See Also: → 1383613
Product: Firefox → DevTools

Resetting the assignee field as it has been 2 years. This way, others might be able to pick up from where this was left off.
Feel free to pick it up again if you did intend to work on it again.

Assignee: sheldon.roddick → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: