Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Fix recursive substitutions, add an option to use the XKCD font, and add mouseovers. #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

piquan
Copy link

@piquan piquan commented May 20, 2016

I had to rewrite much of the substitution code, but tried to keep the original style. Let me know if there's anything I missed.

This hasn't been heavily tested.

This also will add mouseovers with the original text.
return;
}
var cls = parent.getAttribute("class");
if (cls && cls.indexOf("xkcdSubstitutionsExtensionSubbed") != -1) {
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this would catch the nested terms in #28

We already ignore the immediate children of script, form,
etc. elements, as well as spans that we've inserted ourselves.  This
change makes us skip the entire subtree of such an element.

One important effect is that if another extension changes one of our
spans using a similar algorithm to ours (inserting a new subtree) we
previously could end up reprocessing that node in a future
document_end event.  Now, we prevent that.
@piquan
Copy link
Author

piquan commented May 21, 2016

@bencoveney: My original pull request would catch nested terms like #28 describes if we're the only thing mutating the DOM. However, since we can get called multiple times, if another extension changed our subtree, we might end up reprocessing it. I've updated the pull request to account for this.

For instance, we substitute "expand" → "physically expand". Suppose that another extension classifies words according to their parts of speech. Now, see how that would play out.

We substitute expand<span class="xkcd">physically expand</span>. The other extension makes that <span class="xkcd"><span class="adverb">physically</span> <span class="verb">expand</span></span>. Later, we may get another document_end event and reprocess the tree, leading to <span class="xkcd"><span class="adverb">physically</span> <span class="verb"><span class="xkcd">physically expand</span></span></span>. You see the problem.

In the code I originally submitted, we only checked the immediate parent to see if it was a span of class xkcdSubstitutionsExtensionSubbed. Clearly, we should behave well in the presence of other DOM-mutating extensions.

I've updated the pull request to prune the entire subtree at the point of xkcdSubstitutionsExtensionSubbed. This also prunes the subtrees of elements on the ignore list, which has elements like form etc. (Again, previously the code only checked the immediate parent, which makes me wonder if form should have been in the ignore list in the first place.)

@piquan piquan changed the title Add an option to use the XKCD font, and add mouseovers. Fix recursive substitutions, add an option to use the XKCD font, and add mouseovers. May 28, 2016
Copy link

@ravenbuilder934 ravenbuilder934 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the about page it should be capitalized as xkcd; in multiple instances you have it as XKCD

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants