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

The Lines plugin: control your lines via CSS #2389

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

Conversation

galaxy4public
Copy link

This is a very lightweight plugin which was created with high performance in mind.

The primary job of this plugin is to wrap each physical line in a <div> so CSS styling could be applied to any desired line. In the documentation there some showcases how one can re-implement the visual look and feel of the Line Numbers Plugin using the semantic markup provided by this plugin and pure CSS; also there is an example how one can implement basic lines highlighting (a small subset of the Line Highlight plugin).

This plugin is not a replacement for any of the above, since the scope is different -- the examples are just to show how much freedom one can get if they can address a particular line.

@galaxy4public
Copy link
Author

If anybody is curious, the idea for this plugin came out of necessity to implement visible line wrapping indicators. I did some research and came up with quite a robust and efficient solution to the problem. You can see the full step by step explanation at my blog post about the wrap indicators.

While I was working on that, I realised that I needed PrismJS'es support too since it is my primary syntax highlighter. It turned out that pure CSS styling was almost incompatible with mostly JavaScript driven Line Numbers and Line Highlight plugins.

Moreover, that blog post with quite a few <pre> blocks earned a mere "30" score from Google's Lighthouse due to quite a high number of reflows initiated by the Line Numbers plugin.

I wanted line numbering and wished wrap indicators, but I was willing to sacrifice the numbering if it is so heavy. Once I implemented Lines, I thought that I will give it a go and will try to mimic Line Numbers plugin's look and feel using pure CSS.

Then I looked at Line Highlight and realised that its "active line numbers" is dependent on Line Numbers, so I looked at what the minimal required support is needed to make it possible to use some trivial line highlighting.

I am quite pleased with the result since it is fast, only uses scripting at the setup stage, and allows authors to skin their lines using CSS.

@RunDevelopment
Copy link
Member

Thank you for this PR @galaxy4public!

For the future: Please also tell us what parts of your contribution don't work (yet). This is very important because reviewers are also just human, so issues might get through until an unlucky user finds them.

The main flaw of the current implementation is that it breaks multi-line tokens.

Example

image

I say "breaks" but really what's happening is that the plugin is currently generating invalid markup and the browser is trying its best to save it.

The idea of using before-insert and altering the generated markup (instead of using e.g. after-tokenize and doing everything in tokens) is quite good because it's compatible with all plugins/languages that operate on token streams. I definitely want to keep that.
But that means we have to "parse" the generated markup, so we don't break tokens. "parse" in quotes because we only have to detect tokens (this can be done via a "simple" regex (see prism-markup.js)). Then "all" we have to do is to keep track of all currently open tags and replicate them accordingly (sounds simple, but isn't. See details).

Correct line wrapping

(Line ends are explicitly denoted with \n.)

there\n
<span class="x">are\n
<span class="y">so\n
many\n</span>
lines</span>\n
here

Has to be:

<div>there\n</div>
<div><span class="x">are\n</span></div>
<div><span class="x"><span class="y">so\n</span></span></div>
<div><span class="x"><span class="y">many\n</span></span></div>
<div><span class="x">lines</span>\n</div>
<div>here</div>

Keep in mind: The generated markup may be any valid markup, so empty tags and self-closing tags around line ends are likely going to be a problem. Example:

<span></span>\n
<br/>\n

I think I should also point out that tags may also contain line ends. Example:

<!-- no explicit \n this time -->
<span
attr="some
value"></span>

And now for the worst part: HTML doesn't require a closing tag for all non-self-closing tags (e.g. <br>), so we also have to keep track of the tag name to auto-close non-closed tags. (Thanks HTML...)


Implementing this correctly is really tricky but if we can do it, then we have something really powerful on our hands.
You said that this isn't supposed to replace Line Number, but I say: After we merge this, we'll re-implement Line Numbers with this approach (you've already shown that it's easily possible). This will get rid of a bunch of Line Numbers' problems all at once. Same for Line Highlight.

I really like this, so I hope that we can pull this off.
Again, this isn't easy, so if you have questions or need help, ask right away.

@RunDevelopment
Copy link
Member

Also regarding the added anchor: Once we have the line wrapping, adding some tags at the start/end of a line is trivial.

@galaxy4public
Copy link
Author

@RunDevelopment , done. Now the plugin fully parses the highlightedCode passed in env and splits it preserving the markup. This is not the most efficient way to do it, but I think it is passable for general use.

P.S. This was not the way I envisioned I would spend half of my Sunday :)

@galaxy4public
Copy link
Author

The only file I changed is the prism-lines.js file, JFYI.

@galaxy4public
Copy link
Author

galaxy4public commented May 17, 2020

Just found and fixed a bug that was coming out of me not paying attention. I was assuming that the shadow line element that was holding the current line would increase in size when new data encountered, but this was not the case for raw, unhighlighted code blocks.

Upon closer examination, there is no need for that check since we are supposed to always commit the last line buffer to the block (even if nothing was in fact added). I tested this thoroughly and as of now, I am not aware of any weaknesses in this plugin. So, please merge it when possible :)

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Thank you for your time @galaxy4public!

I looked at it and there are things that need to be improved.

However, the main problem I see is that the main algorithm isn't easy to understand and has a lot of edge cases (we already talked about a few), so we really need some tests. We already use JSDOM, so you can just create a file tests/plugins/lines/test.js, insert the following code, and start writing tests.

The test file
const { assert } = require('chai');
const jsdom = require('jsdom');
const { JSDOM } = jsdom;

const dom = new JSDOM();
const document = dom.window.document;
global.self = global;
global.document = document;
global.DOMParser = dom.window.DOMParser;
global.Node = dom.window.Node;

require('../../../prism');
require('../../../plugins/lines/prism-lines');

describe('Prism Lines Plugin', function () {

	function highlightedMarkup(text, language) {
		const pre = document.createElement('pre');
		pre.className = `language-${language}`;
		const code = document.createElement('code');
		code.textContent = text;
		pre.appendChild(code);

		Prism.highlightElement(code);

		return code.innerHTML;
	}

	it('- should <do something>', function () {
		const result = highlightedMarkup(`/* a \n b */`, 'javascript');
		assert.equal(result, `<div class="line"><a></a><span class="token comment">/* a \n</span></div><div class="line"><a></a><span class="token comment"> b */</span></div>`);
	});;
});

Note: This doesn't support the use of DocumentFragment, but we should get rid of that anyway (see comment).

var p1 = document.createElement(node.nodeName);
var p2 = document.createElement(node.nodeName);
if (node.hasAttributes()) {
for (i = 0; i < node.attributes.length ; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing var. Without the var, i will be defined in the global namespace.

Suggested change
for (i = 0; i < node.attributes.length ; i++) {
for (var i = 0; i < node.attributes.length ; i++) {

Copy link
Author

@galaxy4public galaxy4public May 17, 2020

Choose a reason for hiding this comment

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

Today I learned something I now don't like about JavaScript, thanks! It's counter-intuitive, but you are right about the scope.

Copy link
Member

Choose a reason for hiding this comment

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

That's just how it works. You basically said that i is a global variable and JS just did as it was told.

Well, this is a problem that (almost) every dynamically typed language has since variables can be (re-)defined at runtime.

In JavaScript, everything outside of vars inside functions (different behavior for let/const) is a global variable. Array, Object, RegExp are all global variables. You can even delete or replace them if you wanted to. (Please don't.)
This might seem horrible at first (and in many ways, it is) but this also really powerful because, in JS, you can bend everything to your will. (This is also what enables polyfills.)

Well, modern language features such as let and const are a lot stricter tho.

plugins/lines/prism-lines.js Show resolved Hide resolved
}
wrapped.appendChild(line);

if (wrapped.children) {
Copy link
Member

Choose a reason for hiding this comment

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

According to TS's lib.dom.d.ts, children is an HTMLCollection object and can therefore never be falsy.
Did you mean wrapped.children.length?

Copy link
Author

Choose a reason for hiding this comment

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

I guess it comes out of habit not to reference a subattribute without checking parent's existence. Even if some doc says it should be always there, I tend to verify.

plugins/lines/prism-lines.js Show resolved Hide resolved
plugins/lines/prism-lines.js Show resolved Hide resolved
plugins/lines/prism-lines.js Show resolved Hide resolved
}

if (kid && /\n/.test(kid.textContent)) {
// found a special one :)
Copy link
Member

Choose a reason for hiding this comment

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

Please also say what the special case is.

Copy link
Author

Choose a reason for hiding this comment

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

Well, the kid who has a line break within their grasp! The whole point of the function is to split the tree at the first new line break, but to do so, we need to go deep first and find the text node containing the line break

So, we do it recursively and building two trees (one for each side of the split) as we descend.

plugins/lines/prism-lines.js Show resolved Hide resolved
@@ -1185,6 +1185,11 @@
"description": "Line number at the beginning of code lines.",
"owner": "kuba-kubula"
},
"lines": {
"title": "Lines",
"description": "Introduces HTML markup around physical lines to allow styling of the particular line.",
Copy link
Member

Choose a reason for hiding this comment

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

This seems more complicated than it has to be. How about: "Adds wrappers around lines to allow line-bases styling"?


<meta charset="utf-8" />
<link rel="icon" href="favicon.png" />
<title>Line Numbers ▲ Prism plugins</title>
Copy link
Member

Choose a reason for hiding this comment

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

Wrong title.

@RunDevelopment
Copy link
Member

We should also discuss what we want this to be:

As I mentioned before, I see this as a library that can be used to implement a variety of line-based plugins (Line Numbers, Line Highlight, Line Wrap Indicator(?), etc.). I also think it should be opt-in because parsing and processing potentially megabytes of generated markup can take a while and should be avoided wherever possible.

Right now, this plugin is three plugins in one (Lines API + Line Numbers + Line Wrap Indicator) and I think we should separate those three.

You said yourself: "The primary job of this plugin is to wrap each physical line in a <div>," but right now it also creates anchor element and does some magic with ids to enable the other two plugins. I say: "The job of this plugin is to wrap each line" - full stop.
We can then add custom hooks (similar to what Line Numbers is doing) to allow other plugins to modify the created wrappers.

What do you think @galaxy4public?


There's also the issue that this plugin is currently incompatible with the Funky theme.

@galaxy4public
Copy link
Author

Can you please elaborate what's wrong with the Funky theme support?

@galaxy4public
Copy link
Author

@RunDevelopment, regarding your bigger strategic question on what's the future - I am not a developer, so I am not in a position to suggest one way or another. I needed that functionality for my personal blog, but trying a good Netizen I wanted to contribute back, this is the reason I raised the PR.

I already spent quite a bit of time on this, so investing more into the strategic decisions and the follow-up implementation won't cut it for me, unfortunately.

This is what I can do:

  • I will address all the highlighted issues with the code
  • I will create the initial test suite as proposed (however, I am really not a developer, so may struggle with the syntax)

Once the above is done, I would prefer somebody else to slice/dice or transform the idea any further.

How does this sound?

@RunDevelopment
Copy link
Member

I already spent quite a bit of time on this, so investing more into the strategic decisions and the follow-up implementation won't cut it for me, unfortunately.

That's understandable. It's quite a big feature and I want other parts of Prism to rely on this as well, so it's a lot of work.

Thank you for your work!

I would prefer somebody else to slice/dice or transform the idea any further.

Then can I branch your work and continue to work on it?
I'll make a new PR and close this one. You won't have to address any of comments that way.

@galaxy4public
Copy link
Author

Then can I branch your work and continue to work on it?
I'll make a new PR and close this one. You won't have to address any of comments that way.

Sure, that was the whole purpose of the exercise to improve Prism. I would appreciate some mention in a changelog, if possible, but even if not, I think it is more important to have the feature.

So, please go ahead and branch it. I will try to contribute when time permits.

@RunDevelopment
Copy link
Member

Thank you @galaxy4public!

I already made a branch and will publish a new PR in the next few days. I will close this once I open the new PR.

@miltone92
Copy link

Did this get implemented? I'm trying to wrap lines without loosing indentation. Thanks.

@RunDevelopment
Copy link
Member

@miltone92 See #2413. It's not finished yet (v2.0) but it does what you need.

@kennybc kennybc mentioned this pull request Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants