Respec: Link generation does not work in <pre> blocks
Important info
<!DOCTYPE html>
<html>
<head>
<meta charset='utf-8'>
<script src='https://www.w3.org/Tools/respec/respec-w3c-common' async class='remove'></script>
<script class='remove'>
var respecConfig = {
specStatus: "unofficial"
};
</script>
</head>
<body>
<h1>My Awesome API</h1>
<section id='abstract'>
<p>
This is required.
</p>
</section>
<section id='sotd'>
<p>
This is required.
<pre>
<a>Foo</a>
<a href="w3.org">W3</a></pre>
</p>
<p><dfn>Foo</dfn> is great.</p>
<p>Did I mention <a>Foo</a>.</p>
</section>
</body>
</html>
Description of problem
<a>Foo</a> in <pre> is not changed into a link to the definition of Foo.
palemieux
All 37 comments
To link, pre would need to have class=idl (and would need to be valid WebIDL).
marcoscaceres
on 22 Dec 2018
Sorry, I misunderstood. It鈥檚 removing the link? I鈥檒l take a look.
marcoscaceres
on 22 Dec 2018
@marcoscaceres Yes, links to defined terms do not seem to work when in <pre>
palemieux
on 22 Dec 2018
Looks like there is a regression in core/structure.
PS: it wasn't the cause of this issue. (The change is not released yet, good to catch it early.)
saschanaz
on 22 Dec 2018
I investigated it, the issue seems to be in core/highlight plugin. It run perfectly well without including it. I will see more into the plugin to find exact cause.
After excluding core/highlight
devanshbatra04
on 22 Dec 2018
@devanshbatra04, do you want to take this one?
marcoscaceres
on 7 Jan 2019
Yes definitely!
devanshbatra04
on 7 Jan 2019
@devanshbatra04 Any update? If your are not working then I would like to take this up.
pradeepgangwar
on 22 Jan 2019
Not been well at all. I will work on it and the other issues asap. Please have a look other issues. :)
devanshbatra04
on 22 Jan 2019
@devanshbatra04 mind if I start working on this issue ?
CodHeK
on 15 Feb 2019
@CodHeK feel free to take it.
marcoscaceres
on 17 Feb 2019
@marcoscaceres this is where the highlight is added right ? https://github.com/w3c/respec/blob/develop/src/core/highlight.js#L62
Looks to me as some problem in link-to-dfn.js ..
CodHeK
on 17 Feb 2019
Also, what I noticed is that when it's selecting all the elements to be linked, here it is not able to select <a> elements that are inside the <pre> tag! (which ideally shouldn't happen, since the selection is not specific to any tag, it's just a ) That could probably be a reason why, it's not able to highlight any <a> elements inside it...
CodHeK
on 17 Feb 2019
@CodHeK core/link-to-dfn.js is alright, it does include this links and is not the source of this issue.
You have identified correctly that the highlights are added at https://github.com/w3c/respec/blob/develop/src/core/highlight.js#L62
However on the line preceding this (on #L61) The innerHTML gets rewritten for the <pre> element from the message received from https://github.com/w3c/respec/blob/develop/src/core/highlight.js#L48
If you look closely, code: element.textContent does not include the html anchor tags, thus only the textContent is displayed (and not the HTML)
That's where you need to make some small change, I tried using innerHTML isntead of textConent, but that doesn't work because it gets parsed later to text in the value field somehow.
devanshbatra04
on 17 Feb 2019
marcoscaceres
on 17 Feb 2019
Wow I had been looking at the wrong worker all this while :"(
devanshbatra04
on 17 Feb 2019
If you look closely,
code: element.textContentdoes not include the html anchor tags, thus only the textContent is displayed (and not the HTML)
Yeah, this is the problem! Probably, we'd have to have to save element.childNodes and insert them later
CodHeK
on 17 Feb 2019
@devanshbatra04 commenting out element.innerHTML = value fixed the bug!
CodHeK
on 17 Feb 2019
that's not a fix. that's undoing anything core/highlight and respec-worker were doing in the first place.
devanshbatra04
on 17 Feb 2019
Why do we need to assign innerHTML = value in the first place ?
CodHeK
on 17 Feb 2019
Maybe we should run the highlighter first and then do linking?
saschanaz
on 17 Feb 2019
@saschanaz wouldn't work because the highlighter removes anchor tags, just keeps the innerText of the <pre> block.
I think we need to rewrite respec-worker to use innerHTML instead
devanshbatra04
on 17 Feb 2019
wouldn't work because the highlighter removes anchor tags
@devanshbatra04 I retried locally and okay, you're right.
saschanaz
on 17 Feb 2019
So I would suggest two ways to fix (or workaround) this:
- Just use
class=nohighlightwhen it's not really using a programming language - Or, to make things strict, require a valid language hint (
class="(language name)") to run highlighter.
saschanaz
on 17 Feb 2019
I think we need to rewrite
respec-workerto useinnerHTMLinstead
That means here. Yeah, maybe we could instead pass element.innerHTML and the highligher may keep the anchor intact... I don't have a strong opinion here.
saschanaz
on 17 Feb 2019
Here's what happens if I just pass the element.innerHTML, as I said we need to rewrite respec-worker using this approach. I do prefer class=nohighlight instead.
devanshbatra04
on 17 Feb 2019
<pre class="nohighlight">
<a>Foo</a>
<a href="w3.org">W3</a>
</pre>
This code is working fine already, so I believe the issue stands resolved?
devanshbatra04
on 17 Feb 2019
Although I believe we should still use element.innerHTML to allow support for HTML.
devanshbatra04
on 17 Feb 2019
I think I'm also leaning towards what @saschanaz said in https://github.com/w3c/respec/issues/1970#issuecomment-464439658 (i.e., the author should use class="nohighlight") as otherwise this mixes programming languages with spec stuff.
@palemieux can you live with the above? I.e., we can't really fix this because ReSpec depends on an external library to do the syntax highlighting.
marcoscaceres
on 18 Feb 2019
closing as "won't fix".
marcoscaceres
on 18 Feb 2019
@marcoscaceres
The HTML spec recommends using <pre> and <code> together. Should we follow it? (Related issues: https://github.com/whatwg/html/issues/3764, https://github.com/whatwg/html/pull/3768)
If then we could fix this by requiring <code> inside <pre> to run highlighter, although it would be a breaking change...
saschanaz
on 18 Feb 2019
I'd prefer not to do that... but we should make our output be HTML-spec conforming (ie., <pre><code>, which is currently isn't).
So would prefer authors just keep using <pre class="lang"> everywhere.
marcoscaceres
on 18 Feb 2019
marcoscaceres
on 18 Feb 2019
So would prefer authors just keep using
<pre class="lang">everywhere.
I'm okay with that but the issue here is about <pre> without a language tag. Maybe we could:
- If
<pre>has a class, keep the current behavior - If it has no class and no
<code>in it, do not run highlighter for the element.
saschanaz
on 18 Feb 2019
2. If it has no class and no
<code>in it, do not run highlighter for the element.
We might need to check some specs to see how much breakage there would be. The assumption thus far with the highlighter is that it automatically detect the language for you (even if it's a bit hit or miss).
marcoscaceres
on 18 Feb 2019
Does this mean, if the <pre> element has class="nohighlight" we need not wrap the contents of the pre inside code ? And wrap it other case when some other class or no class is used for the pre tag.
CodHeK
on 20 Feb 2019
This issue is closed so I'll comment in #2098.
saschanaz
on 20 Feb 2019
Related issues
sidvishnoi
路
4Comments
saschanaz
路
3Comments
sidvishnoi
路
4Comments
marcoscaceres
路
5Comments
andrea-perego
路
3Comments