173319 - Use Composer status bar for selectable hierarchical flat view of the selection

Status: RESOLVED FIXED

(SeaMonkey :: Composer, enhancement)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

The URL attached shows a possible use of Composer's status bar to show
the ancestors of selection. Each tag name in the bar is a clickable button, and
clicking on it selects the corresponding element. Right-clicking on it opens
a contextual menu allowing to (for instance) assign a class, set ID, remove
element, copy element, remove element but not contents, ...

The current status bar of Composer is totally useless, shows only "Done loading
page" under all circumstances. 

First patch on its way, stay tuned. In the meantime, I am open to all proposals
for this new "flat-view-of-the-selection-ancestors bar".

Comment 1

[Alex Bishop]

I've seen at least one other RFE requesting this.

Comment 2

[Alfonso Martinez]

DreamWeaver MX (sorry I don't remember about previous versions) also has this
feature.

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> DreamWeaver MX also has this feature.

Right, just like most of the SGML and HTML editors on the market since 1986. Just
for the record, this feature was already in the version of Grif (ancestor of Amaya)
I was working on back in 1989.

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

self reminder : "structure bar" (can't come with a better name now) should remain
                empty in Source mode.

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch take 1, no right-click popup menu ; feeback highly welcome

Comment 6

[Charles Manske]

This rocks. Must have.

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch take 2

This is the second version of the patch and I think it is ready for reviews.
Thanks to Kathy for comments on 1st patch.

- each entry in the bar is clickable to select the corresponding ancestor of
  the selection ; in case of discontinuous selection (ie table cells), the
  selection is the deepest one : a row if all cells are in the same row, the
  enclosing tbody instead
- each entry is right-clickable and it opens a contextual menu with the three
  following features:
   1) select element ; this is perhaps redundant with the above but I like it
   2) remove tag : this removes the "tag" of the element, ie deletes the
      enclosing element preserving its contents
   3) change tag : try it !!!! To validate your changes, press enter ; to
      discard all changes, press escape.

/* Enjoy */

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

*** Bug 102195 has been marked as a duplicate of this bug. ***

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

*** Bug 26250 has been marked as a duplicate of this bug. ***

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 102333 [details] [diff] [review]
patch take 2

oops, two changes are missing here ; new patch coming

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch take #3

Comment 12

[Kevin McCluskey (gone)]

cmanske: Why was this bug marked nsbeta1+? This is clearly a composer specific
"feature", which should be given a low priority relative to editor:core work.

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch take #4

in answer to Kathy's comments

Comment 14

[Kathleen :Brade]

Comment on attachment 102791 [details] [diff] [review]
patch take #4

* use SetElementEnabled where you can
* make sure that endTransaction is called if beginTransaction is called

r=brade

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch 4.1, carrying forward r=brade

Comment 16

[Charles Manske]

This is a much better implementation of what we currently have as the "All Tags"
mode in Composer. With this feature, we can reduce bloat by eliminating dozens 
of small images and a large stylesheet use for "All Tags" mode.
Daniel should support double-click on an element in status bar to bring up the
Advanced Properties dialog (in addition to the same support via a context menu
click?) That is the one of the primary uses in All tags mode.
By eliminating that mode, our overall UI will be simplified and reduce confusion
for novice users. Besides, there are certain elements that don't display a yellow
icon, so that feature is not 100% correct anyway.
Thus I give my support for using Daniel's status bar feature and removing "All
Tags" asap.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 102797 [details] [diff] [review]
patch 4.1, carrying forward  r=brade

>Index: ui/composer/content/StructBarContextMenu.js
>===================================================================
>RCS file: ui/composer/content/StructBarContextMenu.js
>diff -N ui/composer/content/StructBarContextMenu.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ ui/composer/content/StructBarContextMenu.js	14 Oct 2002 14:35:47 -0000
>@@ -0,0 +1,223 @@
>+/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

Eek, MPL triple-license please.

>+function InitStructBarContextMenu(elt, ancestorLevel)
>+{
>+  var element = GetAncestor(gLastFocusNode, ancestorLevel);

You could make InitStructBarContextMenu take buttonElt and element and drop
ancestorLevel and GetAncestor

>+
>+  var tag = element.nodeName.toLowerCase();
>+
>+  document.getElementById("structSelect").setAttribute("oncommand", "SelectFocusNodeAncestor(" + ancestorLevel + ");");

and then you could store element in a global var and just set the oncommand
attribute in the xul file. Now you're setting an attribute and recompiling the
event handler every time this gets called.

>+
>+  var structRemoveTag = document.getElementById("structRemoveTag");
>+  structRemoveTag.setAttribute("oncommand", "StructRemoveTag(" + ancestorLevel + ");");  

See above.

>+  var structChangeTag = document.getElementById("structChangeTag");
>+  gContextMenuNode = elt;
>+
>+  structChangeTag.setAttribute("oncommand", "StructChangeTag(" + ancestorLevel + ");");

See above.

>+  SetElementEnabled(structChangeTag, (tag != "body"))
>+  if (tag == "body")
>+    structChangeTag.setAttribute("disabled", "true");
>+  else
>+    structChangeTag.removeAttribute("disabled");
>+
>+}

I think you said you're going to remove this if ... else ... block.

>+function StructRemoveTag(ancestorLevel)
>+{
>+  var editor = GetCurrentEditor();
>+  if (!editor) return;
>+
>+  var element = GetAncestor(gLastFocusNode, ancestorLevel);

Take element and drop GetAncestor.

>+  var tag = element.nodeName.toLowerCase();
>+  var offset = 0;
>+  var child = element.parentNode.firstChild;
>+  while (child != element) {
>+    offset++;
>+    child = child.nextSibling;
>+  }

Get element.parentNode.childNodes and use an index to loop through it,
nextSibling is costly.

>+
>+  editor.beginTransaction();
>+
>+  try { 
>+
>+    if (tag != "table") {
>+
>+      child = element.firstChild;
>+      while (child) {
>+        var clone = child.cloneNode(true);
>+        editor.insertNode(clone, element.parentNode, offset + 1);
>+        child = child.nextSibling;
>+      }

Get element.childNodes and use an index to loop through it, nextSibling is
costly.

>+
>+    }
>+    else {
>+
>+      var nodeIterator = document.createTreeWalker(element,
>+                                                   NodeFilter.SHOW_ELEMENT,
>+                                                   TableCellFilter,
>+                                                   true);
>+      var node = nodeIterator.nextNode();
>+      var firstNode = node;

I think you can drop firstNode.

>+
>+      while (node) {
>+        child = node.firstChild;
>+        while (child) {
>+          clone = child.cloneNode(true);
>+          editor.insertNode(clone, element.parentNode, offset + 1);
>+          child = child.nextSibling;
>+        }

Get node.childNodes and use an index to loop through it, nextSibling is costly.

>+        node = nodeIterator.nextNode();
>+      }
>+
>+    }
>+    editor.deleteNode(element);
>+  }
>+  catch (e) {};
>+
>+  editor.endTransaction();
>+}
>+
>+function StructChangeTag(ancestorLevel)
>+{
>+  var textbox = document.createElementNS(XUL_NS, "textbox");
>+  textbox.setAttribute("value", gContextMenuNode.getAttribute("value"));
>+  textbox.setAttribute("size",  5);
>+  textbox.setAttribute("onkeypress", "OnKeyPress(event, this," + ancestorLevel + ");");

Switch this to addEventListener too?

>+function OnKeyPress(event, elt, ancestorLevel)
>+{
>+  var editor = GetCurrentEditor();
>+
>+  var keyCode = event.keyCode;
>+  if (keyCode == 13) {
>+    var newTag = elt.value;
>+
>+    var element = GetAncestor(gLastFocusNode, ancestorLevel);
>+
>+    var offset = 0;
>+    var child = element.parentNode.firstChild;
>+    while (child != element) { 
>+      offset++;
>+      child = child.nextSibling;
>+    }

Get element.parentNode.childNodes and use an index to loop through it,
nextSibling is costly.

>+
>+    editor.beginTransaction();
>+
>+    try {
>+      var newElt = editor.document.createElement(newTag);
>+      if (newElt) {
>+        child = element.firstChild;
>+        while (child) {
>+          var clone = child.cloneNode(true);
>+          newElt.appendChild(clone);
>+          child = child.nextSibling;
>+        }

Get element.childNodes and use an index to loop through it, nextSibling is
costly.

>+  var mixed = GetSelectionContainer();
>+  var element = mixed.node;

As you noted GetSelectionContainer can return null.

>+  if (element != gLastFocusNode) {

You could do an early return here:

if (element == gLastFocusNode) return;

>+    while (child) {
>+      tmp = child.previousSibling;
>+      toolbar.removeChild(child);
>+      child = tmp;
>+    }

Get toolbar.childNodes and use an index to loop through it, previousSibling is
costly.

>+    var tag, button;
>+    var ancestorLevel = 0;
>+    var bodyElement = GetBodyElement();
>+    do {
>+      tag = element.nodeName.toLowerCase();
>+
>+      button = document.createElementNS(XUL_NS, "toolbarbutton");
>+      button.setAttribute("label",         "<" + tag + ">");
>+      button.setAttribute("value",          tag);
>+      button.setAttribute("oncommand",     "SelectFocusNodeAncestor("  + ancestorLevel + ")");
>+      button.setAttribute("oncontextmenu", "InitStructBarContextMenu(this, " + ancestorLevel + ")");

And this becomes something like:

      var eventListener = function() { return SelectFocusNodeAncestor(element);
};
      button.addEventListener("command",     eventListener);
      eventListener = function() { return InitStructBarContextMenu(button,
element); };
      button.addEventListener("contextmenu", eventListener);

>+function SelectFocusNodeAncestor(ancestorLevel)
>+{
>+  var editor = GetCurrentEditor();
>+  if (editor) {
>+    var element = gLastFocusNode;
>+    while (ancestorLevel > 0) {
>+      element = element.parentNode;
>+      ancestorLevel--;
>+    }
>+    if (element == GetBodyElement())
>+      editor.selectAll();
>+    else
>+      editor.selectElement(element);
>+  }
>+  ResetStructToolbar();
>+}

This becomes 

function SelectFocusNodeAncestor(element)
{
  var editor = GetCurrentEditor();
  if (editor) {
    if (element == GetBodyElement())
      editor.selectAll();
    else
      editor.selectElement(element);
  }
  ResetStructToolbar();
}

>Index: ui/composer/content/ComposerCommands.js
>===================================================================
>RCS file: /cvsroot/mozilla/editor/ui/composer/content/ComposerCommands.js,v
>retrieving revision 1.159
>diff -u -r1.159 ComposerCommands.js
>--- ui/composer/content/ComposerCommands.js	9 Oct 2002 18:06:26 -0000	1.159
>+++ ui/composer/content/ComposerCommands.js	14 Oct 2002 14:36:02 -0000
>@@ -206,6 +206,7 @@
>     commandManager.registerCommand("cmd_PreviewMode",        nsPreviewModeCommand);
>     commandManager.registerCommand("cmd_FinishHTMLSource",   nsFinishHTMLSource);
>     commandManager.registerCommand("cmd_CancelHTMLSource",   nsCancelHTMLSource);
>+    commandManager.registerCommand("cmd_updateStructToolbar", nsUpdateStructToolbarCommand);

Should the U in cmd_update... uppercase?

>@@ -660,6 +667,21 @@
>     }
>   }
> };
>+
>+// STRUCTURE TOOLBAR
>+//
>+var nsUpdateStructToolbarCommand =
>+{
>+  isCommandEnabled: function(aCommand, dummy)
>+  {
>+    UpdateStructToolbar();

Is it really necessary to do this? My guess is yes, but just making sure ;-).

Comment 18

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: in answer to peterv's comments

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 102922 [details] [diff] [review]
in answer to peterv's comments

sr=peterv with the two nits I told you about.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 102922 [details] [diff] [review]
in answer to peterv's comments

a=[e-mail]

Comment 21

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in (trunk)
:-) :-) :-)

Comment 22

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fixes just a few minor stuff + a problem in item(i) I discussed with peterv and that Neil noticed too (not my fault, looks like XPConnect's)

Comment 23

[Kathleen :Brade]

Comment on attachment 103045 [details] [diff] [review]
fixes just a few minor stuff + a problem in item(i) I discussed with peterv and that Neil noticed too (not my fault, looks like XPConnect's)

r=brade

Comment 24

[[e-mail]]

Attachment: Fix overflow of struct toolbar

This makes the struct toolbar items "scroll" to the left if there are too many
nested elements to fit.

Comment 25

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Neil : yeah, I am so dumb... I tried this <spacer flex="1"/> strategy too but
forgot to update the remover loop. Thanks a lot, excellent contribution.

Comment 26

[Joe Francis]

so how do i turn this on in a build?  or is it always on?

Comment 27

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

*** Bug 203968 has been marked as a duplicate of this bug. ***