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

Highlight.js and <details> #4695

Open
AlexDawsonUK opened this issue Apr 18, 2024 · 10 comments
Open

Highlight.js and <details> #4695

AlexDawsonUK opened this issue Apr 18, 2024 · 10 comments
Labels

Comments

@AlexDawsonUK
Copy link

AlexDawsonUK commented Apr 18, 2024

Description of problem

When using progressive disclosure using the details element in a ReSpec document, Highlight.js ceases to function and erases the content from the rendering tree.

URL to affected spec or repo:

https://w3c.github.io/sustyweb/drafts/ (Note as a temporary fix we're using nohighlight as a workaround.)

What happened (e.g., it crashed)?:

The code inside the pre element is erased and an empty tag is rendered instead of syntax being highlighted.

Expected behavior (e.g., it shouldn't crash):

The syntax should be highlighted, as it is for any code that would exist outside of the details element.

Optional, steps to reproduce:

  1. Create a ReSpec draft document.
  2. Add a details element with some syntax inside.
Show / Hide content.

!function(e,t){"use strict";"object"==typeof module&&"object"==typeof module.exports?module.exports=e.document?t(e,!0):function(e){if(!e.document)throw new Error("jQuery requires a window with a document");return t(e)}:t(e)}("undefined"!=typeof window?window:this,function(g,e){"use strict";var t=[],r=Object.getPrototypeOf,s=t.slice,v=t.flat?function(e){return t.flat.call(e)}:function(e){return t.concat.apply([],e)},u=t.push,i=t.indexOf
  • This test showcases a basic ReSpec with it non-functional test.txt
  • This test showcases a basic HTML file with it functional test2.txt

Note: I have verified that this issue does not occur in a normal HTML document using highlight.js therefore it possibly is due to ReSpec or its implementation of highlight?

@sidvishnoi
Copy link
Member

@AlexDawsonUK I cannot reproduce the error with test.txt you shared.

image

Please check there's no < like unescaped HTML tags in your <pre> content. Also, please confirm if it works fine without a parent <details>.

@AlexDawsonUK
Copy link
Author

Using the test.txt (As an HTML file), I am able to reproduce the error, though upon closer examination, the issue apparently seems to be isolated to Google Chrome. Using Mac build 124.0.6367.62 (latest). Unsure if it could be therefore a browser issue (as its only occurring in that browser) or if its a ReSpec issue (as its only an issue when using ReSpec). Unsure how to proceed with this, in any case I've submitted a report with Google as this might be a wider issue.
test

@sidvishnoi
Copy link
Member

sidvishnoi commented Apr 18, 2024

Thanks @AlexDawsonUK. I'll check in Chrome too. Could possibly be a ReSpec issue.

Edit: Confirmed happening in Chrome

@sidvishnoi
Copy link
Member

Doesn't happen with <details open> in Chrome 🤓

@AlexDawsonUK
Copy link
Author

AlexDawsonUK commented Apr 18, 2024

Yes, I noted that odd quirk of behavior, and I even tried writing some JavaScript to close all the details components once everything had finished loading (hoping that it would give Highlight enough time todo its thing and then close them until required). But alas, whatever is causing the bug wouldn't have any of it. 🤷🏻‍♂️

I've expanded the testing and can confirm that all chromium browsers are affected (I tested in Chrome / Edge / Brave).
Firefox and Safari are both free from the issue though so I guess we can blame Google for a weird blink implementation of details? 😈

@sidvishnoi
Copy link
Member

sidvishnoi commented Apr 18, 2024

Agrees on blaming Blink behavior, as Safari and Firefox work fine. The actual issue is Blink returns empty string with innerText when content is not visible. I think ReSpec could use textContent, but I'm concerned about breaking whitespace.
https://github.com/w3c/respec/blob/fca26bdd681a2f3ecc3ff769375f09472fed439b/src/core/highlight.js#L26

cc @marcoscaceres Any idea why did we use innerText? Changed in https://github.com/w3c/respec/pull/2132.

@sidvishnoi
Copy link
Member

@AlexDawsonUK
Copy link
Author

Update: I've created a potential workaround patch that allows functioning rendering engines to continue to enjoy syntax highlighting while chromium functions with it disabled. It requires the use of the nohighlight class on all affected code.

<script async>
	const isFirefox = navigator.userAgent.toLowerCase().includes('firefox');
	const isSafari = navigator.vendor && navigator.vendor.indexOf('Apple') > -1 &&
             navigator.userAgent &&
             navigator.userAgent.indexOf('CriOS') == -1 &&
             navigator.userAgent.indexOf('FxiOS') == -1;
	if (isFirefox || isSafari) {
		for (let i = 0; i < document.getElementsByTagName('pre').length; i++) {
			if (document.getElementsByTagName('pre')[i].className = "nohighlight" ) {
				document.getElementsByTagName('pre')[i].className = ""
			}
		}
	}
</script>

It requires highlighting to be enabled on functional browsers rather than disabling it on non-functional ones due to the issue of the loss of content being displayed from the rendering tree as mentioned previously regarding innerText.

@marcoscaceres
Copy link
Contributor

I can't recall, but probably because .textContent is wonky. We could try it though.

@nigelmegitt
Copy link
Contributor

if (document.getElementsByTagName('pre')[i].className = "nohighlight" )
document.getElementsByTagName('pre')[i].className = ""

You probably want:

document.getElementsByTagName('pre')[i].classList.remove("nohighlight");

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

No branches or pull requests

4 participants