Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve getNode() so that is finds the correct node. #1447 #1459

Merged
merged 2 commits into from Jun 8, 2016

Conversation

Projects
None yet
3 participants
@dpvc
Copy link
Member

commented May 3, 2016

Improves getNode() so that it doesn't find elements inside nodes that come from other MathML nodes (those with IDs). Resolves issue #1447. See that issue for examples that cause the problem.

Davide P. Cervone
Improve getNode() so that it doesn't find elements inside nodes that …
…come from other MathML nodes (those with IDs). Resolves issue #1447.
for (var i = 0, m = node.childNodes.length; i < m; i++) {
var child = node.childNodes[i];
if (name.test(child.className)) return child;
if (child.id == null) return this.getNode(child,type);

This comment has been minimized.

Copy link
@zorkow

zorkow May 11, 2016

Member

Much easier to understand. No idea what the original was doing... But what if there are multiple children without id?
Another problem I can see is that you now accumulate stack space. It's not tail recursive and in JS < ES6 it would not help anyway. Also would a query selector not be a better alternative (assuming there is no IE8 problem)?

One way to make it a bit more lightweight is to bind the name test outside the recursive context.

This comment has been minimized.

Copy link
@dpvc

dpvc May 11, 2016

Author Member

All good observations, especially for a general search command.

This is not a general search command, however. It is used (in several places) for one specific purpose: when an embellished operator needs to have its operator to be stretched, the embellishments may need to be adjusted. For example, an munderover with a base that is stretched may need to have its over accent stretched as well. In these cases, the parent element (munderover in this example) needs to locate the elements that it has used for the base and over nodes. This routine allows the parent to locate those elements. These elements have class names that are things like mjx-base and mjx-over, and those elements are containers in which the actual child MathML nodes are rendered. Usually, they are the direct children of the element used for the munderover, but in some cases, there is an intervening element.

All the nodes that correspond to the original MathML elements are given ids, but the mjx-base and other such sub-parts of the munderover do not. So the test looks through the direct children of the given element for one with the correct type, but if the child has no id (there will be only one in that case), we look for its children instead.

In practice, the routine will only recurse once (at most), so I'm not too worked about tail recursion or the extra RegExp call here. But I suppose it could be done more directly as

getNode: function (node,type) {
  while (node && node.childNodes.length === 1 && node.firstChild.id == null) node = node.firstChild;
  if (node) {
    var name = RegExp("\\b"+type+"\\b");
    for (var i = 0, m = node.childNodes.length; i < m; i++) {
      var child = node.childNodes[i];
      if (name.test(child.className)) return child;
    }
  }
  return null;
}

which avoids the recursion all together.

This comment has been minimized.

Copy link
@dpvc

dpvc May 11, 2016

Author Member

PS, I originally did use a query selector, but the problem is we want to get only the nodes that correspond to the direct children of the given node. In the case of something like msubsup (whose code is shared with msub and msup), when there is no mjx-sup element, we want to return null. But a query selection might find an mjx-sup in an element further down (in the subscript or the base). We want to avoid that. Note that this version of the routine avoids walking into any child node that has an id. Every element that comes from a MathML element will have an id, so we never walk into other MathML nodes. That means we only check the structural elements that are part of the output for the parent node, not the renderings of any of the children.

Since the children could be arbitrarily complicated, query selector might have to look through lots of output. Here, we are going to look through three or four elements at most.

@pkra pkra added this to the MathJax v2.x.x milestone May 17, 2016

@zorkow

This comment has been minimized.

Copy link
Member

commented May 17, 2016

Thanks for the explanation. Sounds reasonable for a special purpose algorithm.
Can we put some of this into a comment for the function for future reference?

@dpvc

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

OK, I've added comments, and replaced the routine by the non-recursive version.

@dpvc dpvc changed the title Improve getNode() so that is finds the correct node Improve getNode() so that is finds the correct node. #1447 Jun 6, 2016

@zorkow

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

lgtm.

@dpvc dpvc merged commit 58e2717 into mathjax:develop Jun 8, 2016

@dpvc dpvc deleted the dpvc:issue1447 branch Jun 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.