Opened 4 years ago

Closed 3 years ago

#1541 closed defect (fixed)

Allow more flexible configuration for formatblock options

Reported by: ejucovy Owned by: gogo
Priority: normal Milestone: 0.97
Component: Xinha Core Version: trunk
Severity: normal Keywords:
Cc:

Description

On my site we added a formatblock option "Pullquote." This wraps the selection in a <div class="pullquote">, and we have CSS that floats it to the right.

We had to modify XinhaCore.js internals to support this option to formatblock, for the following reasons:

  1. Since we're applying HTML that isn't just a tag name, we can't use a simple string to represent it in the formatblock config. Instead we had to provide a callback function to do the modification.
  2. Since other formatblock styles like heading, subheading, etc can be applied to elements within a pullquote, we had to use custom logic for detecting whether the current context is within a pullquote or not.
  3. We also had to use custom logic to deal with users selecting the "normal" option when the current context is within a pullquote, since that should remove the pullquote.

The attached patch is what we came up with. Currently the formatblock config's values are strings (HTML tag names) always. With the patch they can instead be objects with three items:

  • tag: string, HTML tag name to be applied
  • detect: callback function(xinha, el), with boolean return value, called to determine if the current cursor context is within this formatblock option
  • invoke: callback function(xinha), which is executed (instead of execCommand) when the formatblock option is selected

I don't love this patch .. it seems poorly encapsulated, and I don't like putting so much code in the config.js -- I'd rather register it in a plugin somehow. But I'm not sure how to take a better approach.

The attached patch also contains the snippet from our xinhaconfig.js, to demonstrate the use.

Attachments (2)

formatblock.diff (3.0 KB) - added by guest 4 years ago.
formatblock.2.diff (3.1 KB) - added by ejucovy 3 years ago.
a much simpler patch that lets me accomplish the same thing more elegantly

Download all attachments as: .zip

Change History (9)

Changed 4 years ago by guest

comment:1 Changed 4 years ago by guest

Actually, instead of cluttering the patch with the example usage, I'll just put it here -- this is our xinha_config.formatblock setting, with the "sidebar" option that requires the submitted patch. (Note that the patch is fully backwards-compatible.)

  xinha_config.formatblock = {
      'Normal' : {tag: 'p',
                  invoker: function (xinha) {
                      var blockquote = null
                      var firstparent = xinha.getParentElement();
                      if (firstparent.tagName != 'H2' && firstparent.tagName != 'H3' && firstparent.tagName != 'PRE') {
                          blockquote = firstparent;
                          while (blockquote !== null && blockquote.className.trim() != 'pullquote')
                          {
                              blockquote = blockquote.parentNode;
                          }
                      }
                      if (blockquote)
                      {
                          var blockParent = blockquote.parentNode;
                          var firstChild = null;
                          while (blockquote.childNodes.length) {
                              if (firstChild === null)
                              {
                                  firstChild = blockquote.childNodes[0];
                              }
                              blockParent.insertBefore(blockquote.childNodes[0], blockquote);
                          }
                          blockParent.removeChild(blockquote);
                          if (firstChild !== null)
                          {
                              // FIXME: this selects the entire first node, instead of just placing the                                                                                                                                                                                                                                                                                                                                                                                                                                        
                              // cursor at the beginning (or at the previous location where the cursor was).                                                                                                                                                                                                                                                                                                                                                                                                                                   
                              // Without this, the cursor hangs off to the side of the screen, where the                                                                                                                                                                                                                                                                                                                                                                                                                                       
                              // blockquote once had been.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     
                              xinha.selectNodeContents(firstChild);
                          }
                      }
                      else
                      {
                          if( !Xinha.is_gecko)
                          {
                              xinha.execCommand('formatblock', false, '<p>');
                          }
                          else
                          {
                              xinha.execCommand('formatblock', false, 'p');
                          }
                      }
                  },
                  // always return false, to give others a chance to override                                                                                                                                                                                                                                                                                                                                                                                                                                                                  
                  // if nobody else thinks they should be selected, then it will default                                                                                                                                                                                                                                                                                                                                                                                                                                                       
                  // to normal because it comes first                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          
                  detect: function(xinha, el) { return false; }
                 },
      'Heading' : 'h2',
      'Subheading' : 'h3',
      'Pre-formatted' : 'pre',
      'Sidebar' : {tag: 'div',
                      invoker: function (xinha) {
                          var el = xinha.getParentElement();
                          if (el.tagName.toUpperCase() == 'BODY')
                          {
                              //put div around selection                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       
                              if (xinha.hasSelectedText()){
                                  selhtml = xinha.getSelectedHTML();
                                  newhtml = '<div class="pullquote">' + selhtml + '</div>';
                                  xinha.insertHTML(newhtml);
                              }
                          }
                          else
                          {
                              //put div around current block                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   
                              while (el !== null & !Xinha.isBlockElement(el)){
                                  el = el.parentNode;
                              }
                              if (el) {
                                  el_parent = el.parentNode;
                                  div = xinha._doc.createElement('div');
                                  div.className = "pullquote";
                                  el_parent.replaceChild(div, el);
                                  div.appendChild(el);
                              }
                          }
                          xinha.updateToolbar()
                      },
                      detect: function (xinha, el) {
                        while (el !== null) {
                          if (el.nodeType == 1 && el.tagName.toUpperCase() == 'DIV') {
                            return /\bpullquote\b/.test(el.className);
                          }
                          el = el.parentNode;
                        }
                        return false;
                      }
                     }
  };

Changed 3 years ago by ejucovy

a much simpler patch that lets me accomplish the same thing more elegantly

comment:2 Changed 3 years ago by ejucovy

  • Cc ejucovy@… removed
  • Milestone set to 0.97
  • Reporter changed from guest to ejucovy

Oof. I've attached a better patch that accomplishes the same thing with much fewer (and completely localized!) core changes, and a much simpler configuration framework.

The two big realizations that allowed me to simplify were:

  1. For the actual work of performing a custom/complex formatblock command, we can call out to a plugin's onExecCommand("formatblock", false, "<mycustomcommand>") event hook. The plugin's onExecCommand method just needs to listen for cmdID=="formatblock" plus whatever custom commands it wants to handle, and return true after handling them (to prevent core from trying to handle the command).
  2. The core code for figuring out which formatblock pulldown option to mark as selected, is duplicating logic: first it calls editor._getFirstAncestor(el, array_of_tag_names_or_detection_functions) and then, if any of the elements from that array match, it iterates through that same array to figure out which item in it was the match. If we just modify the signature of editor._getFirstAncestor to return both the ancestor and the index of the matching item, this code can look a lot simpler.

comment:3 Changed 3 years ago by ejucovy

And, with this new patch, here's my snippet of configuration (equivalent to the one posted above in comment:1, but much cleaner I think)

  var execNoSidebar = function (xinha) {
    var blockquote = null
    var firstparent = xinha.getParentElement();
    if (firstparent.tagName != 'H2' &&
        firstparent.tagName != 'H3' && firstparent.tagName != 'PRE') {
        blockquote = firstparent;
        while (blockquote !== null && blockquote.className.trim() != 'pullquote') {
            blockquote = blockquote.parentNode;
        }
    }
    if (blockquote) {
        var blockParent = blockquote.parentNode;
        var firstChild = null;
        while (blockquote.childNodes.length) {
            if (firstChild === null) {
                firstChild = blockquote.childNodes[0];
            }
            blockParent.insertBefore(blockquote.childNodes[0], blockquote);
        }
        blockParent.removeChild(blockquote);
        if (firstChild !== null) {
            // FIXME: this selects the entire first node, instead of just placing the                                                                                                                
            // cursor at the beginning (or at the previous location where the cursor was).                                                                                                           
            // Without this, the cursor hangs off to the side of the screen, where the                                                                                                               
            // blockquote once had been.                                                                                                                                                             
            xinha.selectNodeContents(firstChild);
        }
    } else {
        if( !Xinha.is_gecko) {
            xinha.execCommand('formatblock', false, '<p>');
        } else {
            xinha.execCommand('formatblock', false, 'p');
        }
    }
  };

  var execSidebar = function( xinha ) {
    var el = xinha.getParentElement();
    if (el.tagName.toUpperCase() == 'BODY') {
        //put div around selection                                                                                                                                                                   
        if (xinha.hasSelectedText()){
            selhtml = xinha.getSelectedHTML();
            newhtml = '<div class="pullquote">' + selhtml + '</div>';
            xinha.insertHTML(newhtml);
        }
    } else {
        //put div around current block                                                                                                                                                               
        while (el !== null & !Xinha.isBlockElement(el)) {
            el = el.parentNode;
        }
        if (el) {
            el_parent = el.parentNode;
            div = xinha._doc.createElement('div');
            div.className = "pullquote";
            el_parent.replaceChild(div, el);
            div.appendChild(el);
        }
    }
    xinha.updateToolbar()
  };

  xinha_config.formatblock = {
      'Normal': 'nosidebar',
      'Heading': 'h2',
      'Subheading': 'h3',
      'Pre-formatted': 'pre',
      'Sidebar': 'sidebar'
  };

  xinha_config.Events.onExecCommand = function(cmdID, UI, param) {
      if( cmdID != 'formatblock' ) {
          return false;
      }
      if( param != '<nosidebar>' && param != '<sidebar>' ) {
          return false;
      }
      if( param == '<nosidebar>' ) {
          execNoSidebar(this);
          return true;
      }
      if( param == '<sidebar>' ) {
          execSidebar(this);
          return true;
      }
  };

  xinha_config.formatblockDetector['nosidebar'] = function() {
      return false;
  };
  xinha_config.formatblockDetector['sidebar'] = function(xinha, el) {
      console.log("fleem");
      while (el !== null) {
          if (el.nodeType == 1 && el.tagName.toUpperCase() == 'DIV') {
              console.log(el);
              return /\bpullquote\b/.test(el.className);
          }
          el = el.parentNode;
      }
      return false;
  };

comment:4 Changed 3 years ago by ejucovy

The patch still needs some cleanup and better self-documentation, but I think this is a reasonable approach now. Certainly it's on the right track, closer than the original patch. But, I should see what I think of it after I've gotten some sleep..

comment:5 Changed 3 years ago by ejucovy

To help summarize the changes here and what they'd accomplish, I wrote a tutorial-style article, on how to configure the formatblock dropdown with custom options:

http://www.coactivate.org/projects/xinha/custom-formatblock-options

I'm happy with this approach now -- it uses existing infrastructure where possible, and the new configuration option that it adds is unobtrusive and pretty self-explanatory.

There are a lot of moving parts for a user to keep track of if he wants to do this customization (as the article shows!) but I think it has a good separation of concerns, and when it's done the whole thing can be packaged up nicely in a plugin.

Of course most users won't ever want to do this. But it works well for those who do.

I can also imagine (pretty easily) another layer being built in the future which simplifies some of this and packages it together more closely, to make it a little easier for a user to accomplish. There's no need to build that now, but it could even be built as a plugin to experiment first.

comment:6 Changed 3 years ago by ejucovy

In r1289, I committed the first part of my patch. This doesn't add any functionality or new configuration options. It just refactors editor._getFirstAncestor by moving that method's logic into a new function, editor._getFirstAncestorAndWhy.

The new function returns a 2-item array: the second item is an integer index, which, combined with the "types" argument that was passed in to the function, can be used to identify what tag-or-callback the returned "first ancestor" matched. _getFirstAncestor still has the same interface as before -- it just calls _getFirstAncestorAndWhy, throws away the second element from the resulting array, and returns the first.

This simplifies, and slightly optimizes, the formatblock logic in updateToolbar -- which previously repeated the whole "matching" logic that had just been done in _getFirstAncestor.

comment:7 Changed 3 years ago by ejucovy

  • Resolution set to fixed
  • Status changed from new to closed

After mulling it over for a few days I'm still pretty happy with this approach, so I've committed r1296 which adds the xinha_config.formatblockDetector option.

Note: See TracTickets for help on using tickets.