If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.2beta

Status

SeaMonkey
Composer
--
enhancement
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: glazou, Assigned: glazou)

Tracking

Trunk
mozilla1.2beta

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

15 years ago
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

15 years ago
I've seen at least one other RFE requesting this.
(Assignee)

Updated

15 years ago
Blocks: 150779

Comment 2

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

Comment 3

15 years ago
> 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.
(Assignee)

Comment 4

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

Comment 5

15 years ago
Created attachment 102231 [details] [diff] [review]
patch take 1, no right-click popup menu ; feeback highly welcome

Comment 6

15 years ago
This rocks. Must have.
Keywords: nsbeta1+
Target Milestone: --- → mozilla1.3alpha

Updated

15 years ago
Target Milestone: mozilla1.3alpha → mozilla1.2beta
(Assignee)

Comment 7

15 years ago
Created attachment 102333 [details] [diff] [review]
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 */
Attachment #102231 - Attachment is obsolete: true
(Assignee)

Comment 8

15 years ago
*** Bug 102195 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 9

15 years ago
*** Bug 26250 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

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

oops, two changes are missing here ; new patch coming
Attachment #102333 - Attachment is obsolete: true
(Assignee)

Comment 11

15 years ago
Created attachment 102572 [details] [diff] [review]
patch take #3
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.
Keywords: nsbeta1+ → nsbeta1
(Assignee)

Comment 13

15 years ago
Created attachment 102791 [details] [diff] [review]
patch take #4

in answer to Kathy's comments

Comment 14

15 years ago
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
Attachment #102791 - Flags: review+
(Assignee)

Comment 15

15 years ago
Created attachment 102797 [details] [diff] [review]
patch 4.1, carrying forward  r=brade
Attachment #102572 - Attachment is obsolete: true
Attachment #102791 - Attachment is obsolete: true

Comment 16

15 years ago
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 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 ;-).
(Assignee)

Comment 18

15 years ago
Created attachment 102922 [details] [diff] [review]
in answer to peterv's comments
Attachment #102797 - Attachment is obsolete: true
Comment on attachment 102922 [details] [diff] [review]
in answer to peterv's comments

sr=peterv with the two nits I told you about.
Attachment #102922 - Flags: superreview+
Attachment #102922 - Flags: review+
Comment on attachment 102922 [details] [diff] [review]
in answer to peterv's comments

a=rjesup@wgate.com
Attachment #102922 - Flags: approval+
(Assignee)

Comment 21

15 years ago
checked in (trunk)
:-) :-) :-)
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

15 years ago
Created 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)

Comment 23

15 years ago
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
Attachment #103045 - Flags: review+

Comment 24

15 years ago
Created attachment 103057 [details] [diff] [review]
Fix overflow of struct toolbar

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

Comment 25

15 years ago
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

15 years ago
so how do i turn this on in a build?  or is it always on?
(Assignee)

Comment 27

15 years ago
*** Bug 203968 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.