Menu item types for IModelApp.uiAdmin.showContextMenu should allow React.ReactNode icon types

[ImVeryLost]

Not certain if this should be filed in itwinjs backlog or here, since its technically in appui-abstract package.

Is your feature request related to a problem? Please describe.

Menu items in IModelApp.uiAdmin.showContextMenu are of AbstractMenuItemProps type.
AbstractMenuItemProps.item.icon has the type string | ConditionalStringValue.
This icon is shown on the left side of the menu item.

However if you look at the implementation code that actually renders the menu item icon, you find that its rendering it as a Icon component:

{icon !== undefined && <Icon iconSpec={icon} />}

Which in turn supports icons of the following types:

export type IconSpec =
  | string
  | ConditionalStringValue
  | React.ReactNode
  | ConditionalIconItem;

/** Properties for the [[Icon]] React component
 * @public
 */
export interface IconProps extends CommonProps {
  /** CSS class name or SvgPath for icon. This is optional because it is improperly
   * used to extend other interfaces and changing it would break existing API.
   */
  iconSpec?: IconSpec;
}

So the icon could already be a React.ReactNode. In fact, if you pass a react component as the menu item icon, it does render correctly, you just need to cast it to any first, which is not ideal.

Describe the solution you'd like
CommonItemProps.icon or AbstractMenuItemProps.item.icon type would be IconSpec rather than string | ConditionalStringValue.
It already supports all IconSpec types due to underlying use of IconComponent, but does not explicitly allow using them.

Describe alternatives you've considered
Current workaround is to cast to any, which is a bit messy.

Additional context

Using IconSpecUtilities.createWebComponentIconSpec does not currently work for our use case. Our SVG is inlined using esbuild, and ends up looking like

data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><defs><style> ...

This then gets sanitized by the IconComponent till its missing half of its data, and thus does not render correctly (or at all)
Thats related to #421

[raplemie]

AppUI-Abstract package is forbidden to have any React dependency, the idea being that if you write for iTwin.js, any UI should be able to render your content, and this UI may not be React. So, while it does work to cast as any, it is because of an implementation detail that allows you to do so, and your code will only work if you are using AppUI. (iTwin.js and AppUI are 2 different things, AppUI-Abstract is misnamed in that sense.)

So, you have 2 options, write "iTwin.js" code, which is UI agnostic, or write "AppUI" code, in which case your code can depend on AppUI packages directly. (We are slowly moving more and more of the content of AppUI-Abstract to AppUI react because it became apparent that it is unlikely that other UI will be created, but that was the intent. As such, things like UiItemsProvider now exists in AppUI-React, and do support React icons. (for toolbar, as an example)

However, we dont have currently a "ShowContextMenu" API that would do what you need, (however, we could improve UiFramework.openCursorMenu() with React.ReactNode icons, unless the context menu are used on an object, in which case I would suggest a DropdownMenu, from iTwinUI: https://itwinui.bentley.com/docs/dropdownmenu

[ImVeryLost]

@raplemie thanks for the explanation.

however, we could improve UiFramework.openCursorMenu() with React.ReactNode icons,

Did i understood correctly that UiFramework.openCursorMenu is just the underlying AppUi implementation of IModelApp.uiAdmin.showContextMenu ? It seems to work the same way at least. If so, adding the missing ReactNode types would be perfect. Maybe also add some documentation for this method, if its expected to be used by consumers directly?

unless the context menu are used on an object, in which case I would suggest a DropdownMenu.

Not sure what you mean by "used on an object", we are using it to show the context menu for elements in the iModel viewer.
UiFramework.openCursorMenu() seems to be working fine.

So, you have 2 options, write "iTwin.js" code, which is UI agnostic, or write "AppUI" code, in which case your code can depend on AppUI packages directly

Honestly, my preference would be to use IconSpecUtilities.createWebComponentIconSpec or otherwise pass the svg directly, but that seems currently impossible due to inline svgs.

[raplemie]

IconSpecUtilities.createWebComponentIconSpect

As I mention in the other thread, this basically expects svg-loader import value to be provided, I think you could quite easily transform your data:image/svg+xml,<svg... to this format

"used on a object"

showContextMenu allow specifying an HTMLElement to anchor the context menu to the element, making it more or less a dropdown menu for this object, in which case, I would suggest using a proper dropdown, but that doesnt seem to be what you need here.

UiFramework.openCursorMenu is underlying AppUI implementation

Yes, except if you dont provide it direct x, y position and use the htmlElement, where it does additional logic to figure the position.

Expected to be used by consumers directly ?

At the moment, it is public, but since it offer no additional benefit than using uiAdmin.showContextMenu, there isnt much good to point people to this (to be honest, at this point, it should be an internal implementation detail... But it is public) If we do add the React.ReactNode, then it would be worth its proper documentation, however, it makes your code tied to AppUI (which is not a problem if you are using an application, but might be if you use an extension)

My preference would be to use IconSpecUtilities...

It would indeed make your code more portable, I strongly suggest writing an adapter function to convert from data to the svg-loader format. If this becomes a goto format, we could reuse/incorporate this function in our helper.

[raplemie]

Hmm, apparently the documentation is wrong, the svg-loader object is not what is expected, we already support data type of string, however currently we only support base64 strings, not direct strings. I'll see how I can reproduce this, we dont have this type of import in our test apps, the quickest way for this to work would likely be to transform your import to the base64 equivalent:

function toDataBase64(dataRaw: string) {
  const dataParts = dataRaw.split(",");
  const b64 = btoa(dataParts[1]);
  return `${dataParts[0]};base64,${b64}`;
}

[raplemie]

Just created a PR that will fix the IconSpecUtilities.createWebComponentIconSpec in the case of esbuild (if I understood correctly what this is doing). If everything goes well, we should be able to release it in 4.6.2 early next week.

[ImVeryLost]

Hmm, apparently the documentation is wrong, the svg-loader object is not what is expected, we already support data type of string, however currently we only support base64 strings, not direct strings

I was a bit confused by the documentation, since the Icon component wraps the svg in svg-loader itself. Thanks for looking into it further.

Just created a PR that will fix the IconSpecUtilities.createWebComponentIconSpec in the case of esbuild (if I understood correctly what this is doing). If everything goes well, we should be able to release it in 4.6.2 early next week.

That is great news, thank you!
Let me know if any additional info would be helpful. This is how we load svgs using esbuild

[raplemie]

Hi @ImVeryLost, a fix for Icons to support data:image/svg+xml uris have been released in 4.6.2. As this is likely the best option for portable code, let me know if this fixes your issue, and we can close this!

[ImVeryLost]

@raplemie sadly that did not fix the issue completely. Looking at the tests, seems like the major difference is using encodeURIComponent. esbuild does not do that.

When an icon gets to IconComponent sanitize it looks like:

'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><defs><style>.a{fill:url(%23a)}.b{fill:%23000}.c{fill:%23fff}</style><linearGradient id="a" x1="-92.852" y1="-152.013" x2="-107.8787" y2="-167.0397" gradientTransform="translate(-92.3119 -151.4729) rotate(180)" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="%23ffffff" /><stop offset="0.2178" stop-color="%23000000" /><stop offset="0.5306" stop-color="%23888888" /><stop offset="0.8993" stop-color="%23555555" /><stop offset="0.9947" stop-color="%23000000" /></linearGradient></defs><rect class="a" width="16" height="16" rx="1.746" ry="1.746" /><path class="b" d="M7,1v6H1v2h6v6h2V9h6V7H9V1H7z" /><path class="c" d="M7,1v6H1v2h6v6h2V9h6V7H9V1H7z" /></svg>'

And after sanitization it looks like:

'<svg-loader viewbox="0 0 16 16" src="data:image/svg+xml,<svg xmlns=">"&gt;</svg-loader>'

I tried to update the imported svg so the <svg> tag is encoded, to see if its easily fixable from my end.
And that does seemingly pass the sanitizer intact, but does not render correctly. Something is wrong with how the styles are applied.

'data:image/svg+xml,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20viewBox%3D%220%200%2016%2016%22%3E%3Cdefs%3E%3Cstyle%3E.a%7Bfill%3Aurl(%23a)%7D.b%7Bfill%3A%23000%7D.c%7Bfill%3A%23fff%7D%3C%2Fstyle%3E%3ClinearGradient%20id%3D%22a%22%20x1%3D%22-92.852%22%20y1%3D%22-152.013%22%20x2%3D%22-107.8787%22%20y2%3D%22-167.0397%22%20gradientTransform%3D%22translate(-92.3119%20-151.4729)%20rotate(180)%22%20gradientUnits%3D%22userSpaceOnUse%22%3E%3Cstop%20offset%3D%220%22%20stop-color%3D%22%…2%20stop-color%3D%22%23888888%22%20%2F%3E%3Cstop%20offset%3D%220.8993%22%20stop-color%3D%22%23555555%22%20%2F%3E%3Cstop%20offset%3D%220.9947%22%20stop-color%3D%22%23000000%22%20%2F%3E%3C%2FlinearGradient%3E%3C%2Fdefs%3E%3Crect%20class%3D%22a%22%20width%3D%2216%22%20height%3D%2216%22%20rx%3D%221.746%22%20ry%3D%221.746%22%20%2F%3E%3Cpath%20class%3D%22b%22%20d%3D%22M7%2C1v6H1v2h6v6h2V9h6V7H9V1H7z%22%20%2F%3E%3Cpath%20class%3D%22c%22%20d%3D%22M7%2C1v6H1v2h6v6h2V9h6V7H9V1H7z%22%20%2F%3E%3C%2Fsvg%3E'

But maybe its my own fault for trying to use a fancy svg - tried with a couple simplier icons (without <style> tags) and those work fine if I manually encode the svg using encodeURIComponent.
Its just a bit inconvenient, could you explain why is it needed?

[raplemie]

From that example, it seems that the datauris are containing double quotes, this is why it's not working and why I use decodeURIcomponent, because I did not expect esbuild to return uri as raw text with double quotes. Basically what is happening is that the src is cut at the end of that double quote

, which closes the tag, the rest is gibberish and do not make any sense in that context, that's why is sanitized... (and why most of the time these are in base64, to not have these problems.)

It also contains ,, which breaks our logic, but according to this syntax is still valid when not dealing with base64 content, so I'll update accordingly, however, " are not valid within a data uri, nor are < and >... https://datatracker.ietf.org/doc/html/rfc2396#section-2.4.3

For some reason, they are escaping % and # but not <, > or "... https://github.com/evanw/esbuild/blob/4e11b50fe3178ed0a78c077df78788d66304d379/internal/helpers/dataurl.go#L49

That appears to be a bug to me, as it does not follow the RFC...

Also, from esbuild very limited example, it seemed like it was actually encoding:

[raplemie]

This is what I get when I use the data uri provided above:

Which styles are not correct ? (c is on top of b, so the black + is not visible)

This is the code I used, I manually split the data to simplify the code snippet.

        icon: IconSpecUtilities.createWebComponentIconSpec(
          `data:image/svg+xml,${encodeURIComponent(
            decodeURIComponent(
              `<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><defs><style>.a{fill:url(%23a)}.b{fill:%23000}.c{fill:%23fff}</style><linearGradient id="a" x1="-92.852" y1="-152.013" x2="-107.8787" y2="-167.0397" gradientTransform="translate(-92.3119 -151.4729) rotate(180)" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="%23ffffff" /><stop offset="0.2178" stop-color="%23000000" /><stop offset="0.5306" stop-color="%23888888" /><stop offset="0.8993" stop-color="%23555555" /><stop offset="0.9947" stop-color="%23000000" /></linearGradient></defs><rect class="a" width="16" height="16" rx="1.746" ry="1.746" /><path class="b" d="M7,1v6H1v2h6v6h2V9h6V7H9V1H7z" /><path class="c" d="M7,1v6H1v2h6v6h2V9h6V7H9V1H7z" /></svg>`
            )
          )}`
        ),

[ImVeryLost]

That appears to be a bug to me, as it does not follow the RFC...

Fair enough, we will investigate what can be done there. If it does not follow standards, I don't expect you to add custom handling.
Maybe an esbuild plugin (either an existing or new) could properly encode the svg...

This is what I get when I use the data uri provided above:

You are absolutely right, using encodeURIComponent directly does work, it just somehow got flagged by our CSP and I ended up only seeing a grey square. Sorry about the confusion!

Also, ignore that the icon itself does not make much sense visually, I changed up the colors and paths since I'm not sure if the original is open sourced...

[ImVeryLost]

Looks like esbuild removed the encoding on purpose...

evanw/esbuild#1843
evanw/esbuild#3418

[ImVeryLost]

@raplemie added a custom esbuild plugin to our application that will just encode the svg to base64. Tested with those changes & everything works as expected. Thank you for looking into this,
feel free to close this and #421, unless you want to add "default esbuild behaviour" support for future proofing.

[raplemie]

I'll close for now, we'll see what esbuild answer is.