From 9757b8191359059fb6da469faf3e6e8d93352c40 Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Thu, 8 Sep 2022 06:38:25 +0200 Subject: [PATCH 1/5] Wrap long signatures dynamically based on taken client width --- .../kotlin/renderers/html/HtmlRenderer.kt | 5 -- .../renderers/html/htmlPreprocessors.kt | 10 ++- .../DefaultTemplateModelFactory.kt | 4 +- .../kotlin/signatures/JvmSignatureUtils.kt | 27 +------- .../symbol-parameters-wrapper_deferred.js | 63 +++++++++++++++++++ .../test/kotlin/signatures/SignatureTest.kt | 47 -------------- .../PageTransformerBuilderTest.kt | 36 +++++++++++ .../src/test/kotlin/KotlinAsJavaPluginTest.kt | 47 -------------- 8 files changed, 113 insertions(+), 126 deletions(-) create mode 100644 plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js diff --git a/plugins/base/src/main/kotlin/renderers/html/HtmlRenderer.kt b/plugins/base/src/main/kotlin/renderers/html/HtmlRenderer.kt index cb7f58d2c4..e703344216 100644 --- a/plugins/base/src/main/kotlin/renderers/html/HtmlRenderer.kt +++ b/plugins/base/src/main/kotlin/renderers/html/HtmlRenderer.kt @@ -112,11 +112,6 @@ open class HtmlRenderer( } node.dci.kind == SymbolContentKind.Parameter -> { span("parameter $additionalClasses") { - if (node.hasStyle(ContentStyle.Indented)) { - // could've been done with CSS (padding-left, ::before, etc), but the indent needs to - // consist of physical spaces, otherwise select and copy won't work properly - repeat(4) { consumer.onTagContentEntity(Entities.nbsp) } - } childrenCallback() } } diff --git a/plugins/base/src/main/kotlin/renderers/html/htmlPreprocessors.kt b/plugins/base/src/main/kotlin/renderers/html/htmlPreprocessors.kt index 2de6f2b7ed..e2be3ef025 100644 --- a/plugins/base/src/main/kotlin/renderers/html/htmlPreprocessors.kt +++ b/plugins/base/src/main/kotlin/renderers/html/htmlPreprocessors.kt @@ -45,12 +45,20 @@ class CustomResourceInstaller(val dokkaContext: DokkaContext) : PageTransformer } class ScriptsInstaller(private val dokkaContext: DokkaContext) : PageTransformer { + + // scripts ending with `_deferred.js` are loaded with `defer`, otherwise `async` private val scriptsPages = listOf( "scripts/clipboard.js", "scripts/navigation-loader.js", "scripts/platform-content-handler.js", "scripts/main.js", - "scripts/prism.js" + "scripts/prism.js", + + // It's important for this script to be deferred because it has logic that makes decisions based on + // rendered elements (for instance taking their clientWidth), and if not all styles are loaded/applied + // at the time of inspecting them, it will give incorrect results and might lead to visual bugs. + // should be easy to test if you open any page in incognito or by reloading it (Ctrl+Shift+R) + "scripts/symbol-parameters-wrapper_deferred.js", ) override fun invoke(input: RootPageNode): RootPageNode = diff --git a/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt b/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt index 9f1ca57e70..6c746b3701 100644 --- a/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt +++ b/plugins/base/src/main/kotlin/renderers/html/innerTemplating/DefaultTemplateModelFactory.kt @@ -102,7 +102,7 @@ class DefaultTemplateModelFactory(val context: DokkaContext) : TemplateModelFact type = ScriptType.textJavaScript, src = if (it.isAbsolute) it else "$pathToRoot$it" ) { - if (it == "scripts/main.js") + if (it == "scripts/main.js" || it.endsWith("_deferred.js")) defer = true else async = true @@ -204,4 +204,4 @@ private class TemplateDirective(val configuration: DokkaConfiguration, val pathT const val PARAM_NAME = "name" const val PARAM_REPLACEMENT = "replacement" } -} \ No newline at end of file +} diff --git a/plugins/base/src/main/kotlin/signatures/JvmSignatureUtils.kt b/plugins/base/src/main/kotlin/signatures/JvmSignatureUtils.kt index 7ed7ff3f8c..f6c4f0dbdc 100644 --- a/plugins/base/src/main/kotlin/signatures/JvmSignatureUtils.kt +++ b/plugins/base/src/main/kotlin/signatures/JvmSignatureUtils.kt @@ -202,39 +202,18 @@ interface JvmSignatureUtils { fun PageContentBuilder.DocumentableContentBuilder.parametersBlock( function: DFunction, paramBuilder: PageContentBuilder.DocumentableContentBuilder.(DParameter) -> Unit ) { - val shouldWrap = function.shouldWrapParams() - val parametersStyle = if (shouldWrap) setOf(ContentStyle.Wrapped) else emptySet() - val elementStyle = if (shouldWrap) setOf(ContentStyle.Indented) else emptySet() - group(kind = SymbolContentKind.Parameters, styles = parametersStyle) { + group(kind = SymbolContentKind.Parameters, styles = emptySet()) { function.parameters.dropLast(1).forEach { - group(kind = SymbolContentKind.Parameter, styles = elementStyle) { + group(kind = SymbolContentKind.Parameter) { paramBuilder(it) punctuation(", ") } } - group(kind = SymbolContentKind.Parameter, styles = elementStyle) { + group(kind = SymbolContentKind.Parameter) { paramBuilder(function.parameters.last()) } } } - - /** - * Determines whether parameters in a function (including constructor) should be wrapped - * - * Without wrapping: - * ``` - * class SimpleClass(foo: String, bar: String) {} - * ``` - * With wrapping: - * ``` - * class SimpleClass( - * foo: String, - * bar: String, - * baz: String - * ) - * ``` - */ - private fun DFunction.shouldWrapParams() = this.parameters.size >= 3 } sealed class AtStrategy diff --git a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js new file mode 100644 index 0000000000..2e9d47ff00 --- /dev/null +++ b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js @@ -0,0 +1,63 @@ +// helps with some corner cases where starts working already, +// but the signature is not yet long enough to be wrapped +const leftPaddingPx = 60 + +const wrapAllSymbolParameters = () => { + document.querySelectorAll("div.symbol").forEach(symbol => wrapSymbolParameters(symbol)) +} + +const wrapSymbolParameters = (symbol) => { + let symbolBlockWidth = symbol.clientWidth + let innerTextWidth = Array.from(symbol.children) + .map(it => it.getBoundingClientRect().width).reduce((a, b) => a + b, 0) + + // if signature text takes up more than a single line, wrap params for readability + let shouldWrapParams = innerTextWidth > (symbolBlockWidth - leftPaddingPx) + if (shouldWrapParams) { + let parameters = symbol.querySelector("span.parameters") + if (parameters != null) { + parameters.classList.add("wrapped") + parameters.querySelectorAll("span.parameter").forEach(param => { + // has to be a physical indent so that it can be copied. styles like + // paddings and `::before { content: " " }` do not work for that + param.prepend(createNbspIndent()) + }) + } + } +} + +const createNbspIndent = () => { + let indent = document.createElement("span") + indent.append(document.createTextNode("\u00A0\u00A0\u00A0\u00A0")) + indent.classList.add("nbsp-indent") + return indent +} + +const resetAllSymbolParametersWrapping = () => { + document.querySelectorAll("div.symbol").forEach(symbol => resetSymbolParametersWrapping(symbol)) +} + +const resetSymbolParametersWrapping = (symbol) => { + let parameters = symbol.querySelector("span.parameters") + if (parameters != null) { + parameters.classList.remove("wrapped") + parameters.querySelectorAll("span.parameter").forEach(param => { + let indent = param.querySelector("span.nbsp-indent") + if (indent != null) indent.remove() + }) + } +} + +if (document.readyState === 'loading') { + window.addEventListener('DOMContentLoaded', () => { + wrapAllSymbolParameters() + }) +} else { + wrapAllSymbolParameters() +} + +window.onresize = event => { + // need to re-calculate if params need to be wrapped after resize + resetAllSymbolParametersWrapping() + wrapAllSymbolParameters() +} diff --git a/plugins/base/src/test/kotlin/signatures/SignatureTest.kt b/plugins/base/src/test/kotlin/signatures/SignatureTest.kt index 61c9556e6b..13e103b440 100644 --- a/plugins/base/src/test/kotlin/signatures/SignatureTest.kt +++ b/plugins/base/src/test/kotlin/signatures/SignatureTest.kt @@ -849,53 +849,6 @@ class SignatureTest : BaseAbstractTest() { } } - @Test - fun `fun with single param should NOT have any wrapped or indented parameters`() { - val source = source("fun assertNoIndent(int: Int): String = \"\"") - val writerPlugin = TestOutputWriterPlugin() - - testInline( - source, - configuration, - pluginOverrides = listOf(writerPlugin) - ) { - renderingStage = { _, _ -> - val signature = writerPlugin.writer.renderedContent("root/example/assert-no-indent.html").firstSignature() - signature.match( - "fun", A("assertNoIndent"), "(", Parameters( - Parameter("int: ", A("Int")), - ), "): ", A("String"), - ignoreSpanWithTokenStyle = true - ) - assertFalse { signature.select("span.parameters").single().hasClass("wrapped") } - assertFalse { signature.select("span.parameters > span.parameter").single().hasClass("indented") } - } - } - } - - @Test - fun `fun with many params should have wrapped and indented parameters`() { - val source = source("fun assertParamsIndent(int: Int, string: String, long: Long): String = \"\"") - val writerPlugin = TestOutputWriterPlugin() - - testInline( - source, - configuration, - pluginOverrides = listOf(writerPlugin) - ) { - renderingStage = { _, _ -> - writerPlugin.writer.renderedContent("root/example/assert-params-indent.html").firstSignature().match( - "fun", A("assertParamsIndent"), "(", Parameters( - Parameter("int: ", A("Int"), ",").withClasses("indented"), - Parameter("string: ", A("String"), ",").withClasses("indented"), - Parameter("long: ", A("Long")).withClasses("indented") - ).withClasses("wrapped"), "): ", A("String"), - ignoreSpanWithTokenStyle = true - ) - } - } - } - @Test fun `const val with default values`() { val source = source("const val simpleVal = 1") diff --git a/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt b/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt index 8685e3c9aa..67e4d16fcd 100644 --- a/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt +++ b/plugins/base/src/test/kotlin/transformerBuilders/PageTransformerBuilderTest.kt @@ -8,7 +8,10 @@ import org.jetbrains.dokka.transformers.pages.PageTransformer import org.jetbrains.dokka.transformers.pages.pageMapper import org.jetbrains.dokka.transformers.pages.pageScanner import org.jetbrains.dokka.transformers.pages.pageStructureTransformer +import org.jsoup.Jsoup import org.junit.jupiter.api.Test +import utils.TestOutputWriterPlugin +import utils.assertContains import kotlin.test.assertEquals class PageTransformerBuilderTest : BaseAbstractTest() { @@ -168,6 +171,39 @@ class PageTransformerBuilderTest : BaseAbstractTest() { } } + @Test + fun `should load script as defer if name ending in _deferred`() { + val configuration = dokkaConfiguration { + sourceSets { + sourceSet { + sourceRoots = listOf("src/main/kotlin") + } + } + } + val writerPlugin = TestOutputWriterPlugin() + testInline( + """ + |/src/main/kotlin/test/Test.kt + |package test + | + |class Test + """.trimMargin(), + configuration, + pluginOverrides = listOf(writerPlugin) + ) { + renderingStage = { _, _ -> + val generatedFiles = writerPlugin.writer.contents + + assertContains(generatedFiles.keys, "scripts/symbol-parameters-wrapper_deferred.js") + + val scripts = generatedFiles.getValue("root/test/-test/-test.html").let { Jsoup.parse(it) }.select("script") + val deferredScriptSources = scripts.filter { it.hasAttr("defer") }.map { it.attr("src") } + + // important to check symbol-parameters-wrapper_deferred specifically since it might break some features + assertContains(deferredScriptSources, "../../../scripts/symbol-parameters-wrapper_deferred.js") + } + } + } private fun Collection.assertCount(n: Int, prefix: String = "") = assert(count() == n) { "${prefix}Expected $n, got ${count()}" } diff --git a/plugins/kotlin-as-java/src/test/kotlin/KotlinAsJavaPluginTest.kt b/plugins/kotlin-as-java/src/test/kotlin/KotlinAsJavaPluginTest.kt index 6b7a2ae0f1..b43cea05ae 100644 --- a/plugins/kotlin-as-java/src/test/kotlin/KotlinAsJavaPluginTest.kt +++ b/plugins/kotlin-as-java/src/test/kotlin/KotlinAsJavaPluginTest.kt @@ -489,53 +489,6 @@ class KotlinAsJavaPluginTest : BaseAbstractTest() { } } - @Test - fun `should add wrapping and indent to parameters if too many`() { - val writerPlugin = TestOutputWriterPlugin() - val configuration = dokkaConfiguration { - sourceSets { - sourceSet { - sourceRoots = listOf("src/") - externalDocumentationLinks = listOf( - DokkaConfiguration.ExternalDocumentationLink.jdk(8), - stdlibExternalDocumentationLink - ) - } - } - } - testInline( - """ - |/src/main/kotlin/kotlinAsJavaPlugin/Wrapped.kt - |package kotlinAsJavaPlugin - | - |class Wrapped(val xd: Int, val l: Long, val s: String) - """.trimMargin(), - configuration, - pluginOverrides = listOf(writerPlugin), - cleanupOutput = true - ) { - pagesGenerationStage = { root -> - val content = root.children - .flatMap { it.children() } - .map { it.content }.single().mainContents - - val text = content.single { it is ContentHeader }.children - .single() as ContentText - - assertEquals("Constructors", text.text) - } - renderingStage = { _, _ -> - writerPlugin.writer.renderedContent("root/kotlinAsJavaPlugin/-wrapped/-wrapped.html").firstSignature().match( - A("Wrapped"), A("Wrapped"), "(", Parameters( - Parameter(A("Integer"), "xd,").withClasses("indented"), - Parameter(A("Long"), "l,").withClasses("indented"), - Parameter(A("String"), "s").withClasses("indented"), - ).withClasses("wrapped"), ")", ignoreSpanWithTokenStyle = true - ) - } - } - } - /** * Kotlin Int becomes java int. Java int cannot be annotated in source, but Kotlin Int can be. * This is paired with DefaultDescriptorToDocumentableTranslatorTest.`Java primitive annotations work`() From f2e2591b80d04a6a6090fb814528826e285fb57b Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Wed, 14 Sep 2022 16:49:28 +0200 Subject: [PATCH 2/5] Ignore annotation blocks when calculating width --- .../symbol-parameters-wrapper_deferred.js | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js index 2e9d47ff00..16ab93d98c 100644 --- a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js +++ b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js @@ -7,22 +7,25 @@ const wrapAllSymbolParameters = () => { } const wrapSymbolParameters = (symbol) => { + let parametersBlock = symbol.querySelector("span.parameters") + if (parametersBlock == null) { + return // nothing to wrap + } + let symbolBlockWidth = symbol.clientWidth let innerTextWidth = Array.from(symbol.children) + .filter(it => !it.classList.contains("block")) // blocks are usually on their own (like annotations), so ignore it .map(it => it.getBoundingClientRect().width).reduce((a, b) => a + b, 0) // if signature text takes up more than a single line, wrap params for readability let shouldWrapParams = innerTextWidth > (symbolBlockWidth - leftPaddingPx) if (shouldWrapParams) { - let parameters = symbol.querySelector("span.parameters") - if (parameters != null) { - parameters.classList.add("wrapped") - parameters.querySelectorAll("span.parameter").forEach(param => { - // has to be a physical indent so that it can be copied. styles like - // paddings and `::before { content: " " }` do not work for that - param.prepend(createNbspIndent()) - }) - } + parametersBlock.classList.add("wrapped") + parametersBlock.querySelectorAll("span.parameter").forEach(param => { + // has to be a physical indent so that it can be copied. styles like + // paddings and `::before { content: " " }` do not work for that + param.prepend(createNbspIndent()) + }) } } From 129bb1bec5175b3e6bc7d6c552f8e5e4a169b249 Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Wed, 14 Sep 2022 23:33:29 +0200 Subject: [PATCH 3/5] Handle a corner case where not all styles have been loaded --- .../scripts/symbol-parameters-wrapper_deferred.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js index 16ab93d98c..66cf736d75 100644 --- a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js +++ b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js @@ -13,6 +13,17 @@ const wrapSymbolParameters = (symbol) => { } let symbolBlockWidth = symbol.clientWidth + + // Even though the script is marked as `defer` and we wait for `DOMContentLoaded` event, + // it can happen that `symbolBlockWidth` is 0, indicating that something hasn't been loaded. + // Re-try after some time, should work. Should not go into infinite recursion because + // symbol blocks that have parameters definitely have width above 0 + if (symbolBlockWidth === 0) { + setTimeout(function() { + wrapSymbolParameters(symbol); + }, 100) + } + let innerTextWidth = Array.from(symbol.children) .filter(it => !it.classList.contains("block")) // blocks are usually on their own (like annotations), so ignore it .map(it => it.getBoundingClientRect().width).reduce((a, b) => a + b, 0) From e1e1b09c8c1eae82d93b2398081901189ef6ad59 Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Thu, 15 Sep 2022 01:00:45 +0200 Subject: [PATCH 4/5] Return if not all styles have been applied/loaded --- .../dokka/scripts/symbol-parameters-wrapper_deferred.js | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js index 66cf736d75..9dc0e19fe2 100644 --- a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js +++ b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js @@ -22,6 +22,7 @@ const wrapSymbolParameters = (symbol) => { setTimeout(function() { wrapSymbolParameters(symbol); }, 100) + return } let innerTextWidth = Array.from(symbol.children) From 5731a9103642c1ca20dfdc6b9d85bfec242cf214 Mon Sep 17 00:00:00 2001 From: IgnatBeresnev Date: Tue, 20 Sep 2022 18:02:52 +0200 Subject: [PATCH 5/5] Use `ResizeObserver` instead of relying on timeout --- .../scripts/symbol-parameters-wrapper_deferred.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js index 9dc0e19fe2..248d0ab033 100644 --- a/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js +++ b/plugins/base/src/main/resources/dokka/scripts/symbol-parameters-wrapper_deferred.js @@ -2,6 +2,14 @@ // but the signature is not yet long enough to be wrapped const leftPaddingPx = 60 +const symbolResizeObserver = new ResizeObserver(entries => { + entries.forEach(entry => { + const symbolElement = entry.target + symbolResizeObserver.unobserve(symbolElement) // only need it once, otherwise will be executed multiple times + wrapSymbolParameters(symbolElement); + }) +}); + const wrapAllSymbolParameters = () => { document.querySelectorAll("div.symbol").forEach(symbol => wrapSymbolParameters(symbol)) } @@ -16,12 +24,9 @@ const wrapSymbolParameters = (symbol) => { // Even though the script is marked as `defer` and we wait for `DOMContentLoaded` event, // it can happen that `symbolBlockWidth` is 0, indicating that something hasn't been loaded. - // Re-try after some time, should work. Should not go into infinite recursion because - // symbol blocks that have parameters definitely have width above 0 + // In this case, just retry once all styles have been applied and it has been resized correctly. if (symbolBlockWidth === 0) { - setTimeout(function() { - wrapSymbolParameters(symbol); - }, 100) + symbolResizeObserver.observe(symbol) return }