Skip to content

Fix rendering issue for links with special characters #532

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

Merged
merged 7 commits into from
Apr 12, 2023

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Apr 5, 2023

Bug/issue #, if applicable: rdar://85531439

Summary

Fix an issue where symbol links or documentation links containing special characters resolved successfully but didn't render as expected on the page.

There are two parts to this fix:

  • Allow any previously resolved reference to be looked up without resolving it again. I do this because MarkupReferenceResolver replaces the links original destination with the absolute string of the resolves reference but the resolved reference replaces non-path characters with "-" resulting in potential ambiguity when re-resolving the link.
  • Preserve the isAutolink attribute of Link values when updating their destination with a new resolved destination

Dependencies

None.

Testing

  • Use one or more special characters—for example π or 😃—in an element that can be linked to. This can be either in a symbol name, article file name, or heading.
    See for example this unit test.
  • Write a symbol link to the symbol, article, or heading containing the special character.
    Expected: The link should resolve and render with its title as the link text
  • Also curate the symbol, article, or heading containing the special character.
    Expected: The link should resolve and render with its title as the link text

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

Despite adding a lookup of all resolved references I've not seen any regression in peak memory usage in my local testing

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Metric                                   │ Change          │ main                 │ current              │
├──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Duration for 'bundle-registration'       │ no change¹      │ 5.635 sec            │ 5.644 sec            │
│ Duration for 'convert-total-time'        │ no change²      │ 9.626 sec            │ 9.613 sec            │
│ Duration for 'documentation-processing'  │ no change³      │ 3.758 sec            │ 3.738 sec            │
│ Duration for 'finalize-navigation-index' │ no change⁴      │ 0.117 sec            │ 0.116 sec            │
│ Peak memory footprint                    │ no change⁵      │ 1,014.9 MB           │ 1,008.7 MB           │
│ Data subdirectory size                   │ no change       │ 280.4 MB             │ 280.4 MB             │
│ Index subdirectory size                  │ no change       │ 2.5 MB               │ 2.5 MB               │
│ Total DocC archive size                  │ no change       │ 371.7 MB             │ 371.7 MB             │
│ Topic Anchor Checksum                    │ no change       │ a8c40d797627683c5d7c │ a8c40d797627683c5d7c │
│ Topic Graph Checksum                     │ no change       │ da82bd7f35eae4bb56e8 │ da82bd7f35eae4bb56e8 │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────┘

 1: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : -0.387          degrees of freedom : 28       95% confidence critical value : +2.042

 2: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : +0.494          degrees of freedom : 28       95% confidence critical value : +2.042

 3: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : +1.539          degrees of freedom : 28       95% confidence critical value : +2.042

 4: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : +1.669          degrees of freedom : 28       95% confidence critical value : +2.042

 5: No statistically significant difference.
    The values are similar enough that the most probable explanation is that they're random samples from the same data set.
    t-statistic : +1.792          degrees of freedom : 28       95% confidence critical value : +2.042

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

Hm.. for some reason the CI is failing with errors that I can't reproduce locally with either Swift 5.8 or Swift 5.9

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist force-pushed the links-with-special-characters branch from 86650ed to c20c52e Compare April 6, 2023 20:52
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

After the latest changes I still see no measurable performance differences compared to main.

@@ -639,7 +639,7 @@ private extension CharacterSet {
///
/// If this step is not performed, the disallowed characters are instead percent escape encoded, which is less readable.
/// For example, a fragment like `"#hello world"` is converted to `"#hello-world"` instead of `"#hello%20world"`.
func urlReadableFragment(_ fragment: String) -> String {
func urlReadableFragment<S: StringProtocol>(_ fragment: S) -> String {
Copy link
Contributor

@daniel-grumberg daniel-grumberg Apr 12, 2023

Choose a reason for hiding this comment

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

I don't remember what version of swift we aim to support, would some StringProtocol be acceptable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, we support Swift 5.5 and the some keyword was added in Swift 5.7

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 186fb13 into swiftlang:main Apr 12, 2023
@d-ronnqvist d-ronnqvist deleted the links-with-special-characters branch April 12, 2023 18:08
d-ronnqvist added a commit to d-ronnqvist/swift-docc that referenced this pull request Apr 12, 2023
* Fast path to look up resolved references by their absolute string

rdar://85531439

* Fix bug where links with special characters used the link as title

rdar://85531439

* Skip new test when running old link resolver implementation

* Add more type annotations in test to accommodate other compiler versions

* Expand the fallback parsing of authored documentation links
d-ronnqvist added a commit that referenced this pull request Apr 14, 2023
* Fast path to look up resolved references by their absolute string

rdar://85531439

* Fix bug where links with special characters used the link as title

rdar://85531439

* Skip new test when running old link resolver implementation

* Add more type annotations in test to accommodate other compiler versions

* Expand the fallback parsing of authored documentation links
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants