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

DYN-3652 docs browser Artifakt font fails if not installed #13654

Merged
merged 4 commits into from
Feb 3, 2023

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Jan 4, 2023

Purpose

This PR addresses DYN-3652 issue: docs browser font fails to load on machines without Artifakt installed already.
Solition: Embedded Artifakt Element as base64

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

  • to ensure the DocumentationBrowser always loads with the correct font, the Artifakt Element font is now included in the css style sheet as base64

Reviewers

@reddyashish
@mjkkirschner

FYIs

@Amoursol

- to ensure the DocumentationBrowser always loads with the correct font, the Artifakt Element font is now included in the css style sheet as base64
@mjkkirschner
Copy link
Member

so I have bad news!

according to @RobertGlobant20 - webView2 currently has a 2 - 3 meg limit when loading html using navigateToString - I think sticking the entire font into the css is not the ideal given this new limitation... (web browser control did not have this).

@RobertGlobant20 can you suggest an approach using the virtual host names so the font can be referenced from disk without embedding?

@RobertGlobant20
Copy link
Contributor

so I have bad news!

according to @RobertGlobant20 - webView2 currently has a 2 - 3 meg limit when loading html using navigateToString - I think sticking the entire font into the css is not the ideal given this new limitation... (web browser control did not have this).

@RobertGlobant20 can you suggest an approach using the virtual host names so the font can be referenced from disk without embedding?

@mjkkirschner well, I'm not completely sure if this case is related to the WebView2.NavigateToString() limit, I will need to reproduce the problem in Dynamo and check if the problem is the html limit.
I can see that the problem is happening in DynamoRevit when seeing the documentation for the Orchid nodes.
Then I guess I need to install the Orchid package (using PackageManager) and see the documentation for any node, am I right?
Thanks

@dnenov
Copy link
Collaborator Author

dnenov commented Jan 10, 2023

so I have bad news!

according to @RobertGlobant20 - webView2 currently has a 2 - 3 meg limit when loading html using navigateToString - I think sticking the entire font into the css is not the ideal given this new limitation... (web browser control did not have this).

@RobertGlobant20 can you suggest an approach using the virtual host names so the font can be referenced from disk without embedding?

Should I be doing something on my end to try to resolve the issue? I took this approach as it was a more straightforward solution to the problem at hand, but I could try loading the font that dynamo ships with, the same way we do for WPF.

image

@mjkkirschner
Copy link
Member

yes, I don't think you should be embedding the font.

How critical that is depends on how large it is when base64 encoded.

@dnenov
Copy link
Collaborator Author

dnenov commented Jan 16, 2023

We have been chatting with @RobertGlobant20 today and I understood where the 2 MB limitation will come into play. I tried adding the font into the fallback directory and referencing it as 'http://appassets/[FONT_LOCATION]' but that didn't work out.
Regardless, the base64 font string is 74 KB, just to keep that in mind in case creating additional virtual folders adds too much complexity.

- now substitutes an embedded resource containing the font style and converts to base64 before rendering the html page
@dnenov
Copy link
Collaborator Author

dnenov commented Jan 20, 2023

I have now refactored the code to be using a similar structure and ways to process the font as done in the user guide, however it should be known that in essence, the HTML string which processed still contains a base64 version of the font. The difference is that, instead of being hard-coded inside the css part of the MardownStyling.html, at run time, the font (as embedded resource) is dynamically converted to base64 and swapped in the HTML file.

I believe there is a (security) limitation to serving fonts from a file location unless they are installed.

@dnenov
Copy link
Collaborator Author

dnenov commented Jan 31, 2023

Hi @mjkkirschner and @RobertGlobant20 - I have asked @sm6srw to merge this PR unless you have any outstanding issues.

@RobertGlobant20
Copy link
Contributor

Hi @mjkkirschner and @RobertGlobant20 - I have asked @sm6srw to merge this PR unless you have any outstanding issues.

Looks Good to Me

@dnenov
Copy link
Collaborator Author

dnenov commented Feb 1, 2023

Hi @mjkkirschner and @RobertGlobant20 - I have asked @sm6srw to merge this PR unless you have any outstanding issues.

Looks Good to Me

Thank you, @RobertGlobant20 !

@sm6srw sm6srw marked this pull request as ready for review February 1, 2023 17:50
- brought back a fontface css rule to be whitelisted by the Md2HtmlSanitizer
- all DocumentationBrowserExtension tests passing
@sm6srw
Copy link
Contributor

sm6srw commented Feb 3, 2023

@sm6srw
Copy link
Contributor

sm6srw commented Feb 3, 2023

I will go ahead and merge this.

@sm6srw sm6srw merged commit e598254 into DynamoDS:master Feb 3, 2023
QilongTang pushed a commit that referenced this pull request Mar 25, 2024
* Embedded Artifakt Element as base64

- to ensure the DocumentationBrowser always loads with the correct font, the Artifakt Element font is now included in the css style sheet as base64

* Refactored font resource

- now substitutes an embedded resource containing the font style and converts to base64 before rendering the html page

* Revert unnecessary changes

* FontFace Css rule reintroduced

- brought back a fontface css rule to be whitelisted by the Md2HtmlSanitizer
- all DocumentationBrowserExtension tests passing
@QilongTang QilongTang mentioned this pull request Mar 25, 2024
9 tasks
QilongTang added a commit that referenced this pull request Mar 26, 2024
* [DYN-4302 + DYN-5259] Add visual indication for deprecated packages (#13413)

* add deprecated label

* update extension

* sync package manger and package view displays

* finishes

* set horizontal offset

* update HD profile icon

* Update PackageDetailsViewModel.cs

* add comment

* conflicts fix

* Update PackageDetailItem.cs

* Revert "conflicts fix"

This reverts commit 2e60462.

* conflict fix2

* DYN-3652 docs browser Artifakt font fails if not installed (#13654)

* Embedded Artifakt Element as base64

- to ensure the DocumentationBrowser always loads with the correct font, the Artifakt Element font is now included in the css style sheet as base64

* Refactored font resource

- now substitutes an embedded resource containing the font style and converts to base64 before rendering the html page

* Revert unnecessary changes

* FontFace Css rule reintroduced

- brought back a fontface css rule to be whitelisted by the Md2HtmlSanitizer
- all DocumentationBrowserExtension tests passing

* DYN-5297 Import Excel Small Numbers (#13680)

* read scientific notation

* remove unecessary reference

* Fix for situation with formulas and add unit tests for scientific notation

* Dyn 5314 open xml (#13647)

* Fix String display

* Adding Unit test

* Checking Formula

* Fixing Ghost warning (#13820)

* Fixing Ghost warning

* Keeping the Warning Color for the new state

* Adding the Unit test

* Revert "Keeping the Warning Color for the new state"

This reverts commit 40e9643.

* Revert "Fixing Ghost warning"

This reverts commit 04c14c1.

* Updates, Implementation and Test

* fix List.AllIndicesOf node (#13773)

* Handle null values in watch node/preview bubble (#13855)

* Handle null values in watch node/preview bubble

* Add test

* Update WatchNodeTests.cs

* Add null check (#13908)

* DYN-6073 civil3 d packages tour crashing (#14338)

* DYN-6073 Civil3D Packages Guide Crashing

It was crashing due that the user tried to close the Step 4 clicking the PackageSearch window and the Popup is not closed but seems that is already disposed.

* DYN-6073 Civil3D Package Tour Crashing

This change is disabling the close button in the PackageManagerSearch window when running the Packages tour, when passing to the next Step the button is enabled again (unless the next step also requires to disable the button). I've added a new icon image that will be shown when the close button is disabled.
In this way we will be preventing the crash when the user try to close the Packages tour by closing the PackageManagerSearch window

* DYN-6073 Civil3D Package Tour Crashing

This fix will solve the problem of the packages guide crashing when clicking the Library (package installed) for passing from Step8 to Step9.

* Update to remove extra code

* Remove the test causing build failure

* DYN-6123 Fix groups loose associations with notes and nested groups when a custom node is created (#14220)

* Fix note in group in customnode

* add test

* merge test from other PR

* DYN-6130 remove the toast notification (#14317) (#14332)

* Make sure close the Toast notification when Dynamo closes

* Removing extra file

* Removing extra file

* Relocate the responsibility about the toast notification to the DynamoViewModel

* removing unnecessary using

* take advantage of already implementation to apply minimal changes

* Moving the code to a proper place

---------

Co-authored-by: Jesus Alfredo Alviño <[email protected]>

* DependencyType  match (#14314)

* remove extra code

---------

Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: Deyan Nenov <[email protected]>
Co-authored-by: filipeotero <[email protected]>
Co-authored-by: jesusalvino <[email protected]>
Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: reddyashish <[email protected]>
Co-authored-by: Roberto T <[email protected]>
Co-authored-by: Jesus Alfredo Alviño <[email protected]>
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.

4 participants