OfficeDev / office-js

A repo and NPM package for Office.js, corresponding to a copy of what gets published to the official "evergreen" Office.js CDN, at https://appsforoffice.microsoft.com/lib/1/hosted/office.js.
https://learn.microsoft.com/javascript/api/overview
Other
683 stars 95 forks source link

Icons not being displayed for some of the buttons in a contextual tab #2981

Closed wh1t3cAt1k closed 1 month ago

wh1t3cAt1k commented 1 year ago

Your Environment

Expected behavior

All icons are displayed in the ribbon created dynamically.

Current behavior

Some icons are not displayed. Also note that Excel for Windows decided to make three of the icons small for no reason:

image

image

On Excel Online all icons are displayed well as large icons. Can we hope for some consistency here?

image

I CHECKED THAT ICON URLS ARE ALL ACCESSIBLE. Furthermore, this works just fine in Excel for the Web in the browser.

Steps to reproduce

Use script lab https://gist.github.com/wh1t3cAt1k/6e60abb420b76fc137b160fb1cd56763

Provide additional details

I tried clearing the office cache (wef folder contents) and ExcelXX.xlb in AppData/Roaming/Microsoft/Office/16.0, relaunching Excel afterwards and restarting, nothing helps.

Renaming the IDs of the controls does not help either.

Useful logs

No particular error in console.

ghost commented 1 year ago

Thank you for letting us know about this issue. We will take a look shortly. Thanks.

wh1t3cAt1k commented 1 year ago

I also just tried reducing the number of available icon sizes to just 32x32 and 80x80 (the obligatory sizes). Didn't help:

image

image

gmichaud commented 1 year ago
Screenshot 2022-11-17 at 15 52 50

I have replicated the same thing on Excel for Mac... looks like it's only working properly on Excel Online.

wh1t3cAt1k commented 1 year ago

Note that I am not able to run the script lab in Excel Online because "the ribbon API is not available".

But please trust me when I say that even when running the script lab sample inside a proper add-in:

Please prioritise this issue as we have a release on November 24th and need to have this working by then.

RuizhiSunMS commented 1 year ago

@Rick-Kirkham Hi Rick, Do you know anyone owning the Ribbon API? The only one I know is Roger but he is OOF.

trigurl commented 1 year ago

We've checked in a fix to address the icons in the menu. The Windows layout is actually correct, when there are 4 controls in the group, the first control is large while the last 3 controls are small. I do see that there is some inconsistency with Excel online however.

gmichaud commented 1 year ago

@trigurl thanks! When you check in a fix to something related to Office-js, does it go through the same release process as other Excel features (beta channel update, current channel (preview), etc...) ? Or is the release flow different?

trigurl commented 1 year ago

The fix is not in Office-js, it's a product fix. The fix was submitted by a colleague in middle of November. So it should be in Office Windows builds greater than 15918 approximately.

wh1t3cAt1k commented 1 year ago

@trigurl I disagree that the Windows layout is correct, some icons are not being displayed at all as I mentioned:

image

wh1t3cAt1k commented 1 year ago

@trigurl on Excel for Mac the problem is the opposite, it only reproduces ONCE, after restarting Excel it went away.

image

Compare it to my previous screenshot, large icons are not OK, small icons are OK. just to check if there's any inconsistency on Mac, so far we only discussed that the fix is being delivered for Windows,

cc @Wenjun-Gong @gmichaud

wh1t3cAt1k commented 9 months ago

We haven't observed this issue in quite a while. @gmichaud can you confirm on your side?

gmichaud commented 9 months ago

Haven't seen this in a while either, good to close @wh1t3cAt1k

wh1t3cAt1k commented 9 months ago

image

Speak of the devil... Still relevant. :(

I cleared the add-in cache and added the add-in from My Add-ins menu:

image

The first time every single icon was corrupted.

Upon reopening Excel, all good:

image

I hope this issue doesn't celebrate its second birthday :))

wh1t3cAt1k commented 9 months ago

@trigurl @SiruiSun-MSFT @Rick-Kirkham the bug with missing icons replicated both on Windows and Mac when we removed the add-in, cleared the add-in cached in the trust centre, and then reopened Excel and re-added the add-in.

I suggest doing a code review of the asset loading code, if there are any unnecessarily strict timeouts, optimistic loading or potential inconsistencies there.

jrgilman commented 2 months ago

Is there any update on this @SiruiSun-MSFT @Rick-Kirkham @trigurl? I posted on 4446 which seems to be a duplicate of this. @fzumstein was able to replicate this as well on Windows. Mac OS X and Excel Online both seem to be fine.

@wh1t3cAt1k did you guys find a fix for this?

shanshanzheng-dev commented 1 month ago

Hi @wh1t3cAt1k @jrgilman sorry for slow response. We're actively investigating this issue. @jrgilman May I know which add-in is able to repro this issue, could you share us manifest? That could be help us to investigate. Thanks.

fzumstein commented 1 month ago

@shanshanzheng-dev In our case we figured out that it's caused by Cache-Control and Pragma: no-cache response headers. Once you remove them on the icons, it works correctly with Windows Desktop icons. It's confusing, as Windows Desktop is the only platform that is affected by this. See also https://learn.microsoft.com/en-us/answers/questions/507931/icons-are-not-showing-in-the-ribbon-in-desktop-app

wh1t3cAt1k commented 1 month ago

I cannot confirm that cache control has anything to do with this bug... also for us it was replicating both on the Windows and the Mac. You can verify the headers from the below asset that we never say "no-cache"

https://secure.velixo.com/icons-v2/96x96/writeback-96x96.png

Just out of curiousity, when I hit it with Postman, the first time Cloudfront says "cache miss", and second time "cache hit":

First time:

image

Second time:

image

But I am not confident at all if it has anything to do with the issue. On our side, it replicated whenever we cleared the add-in cache, both on Windows and Mac.

Velixo internal tracking item

trigurl commented 1 month ago

Hello, I'd like to clarify some things and summarize as well.

1) The server hosting the image should not return a Cache-Control header specifying no-cache, no-store, or similar options in the HTTP response. Caching just be supported. You can use Fiddler to inspect the response after the icon has been fetched to look for no-cache. Here's an example:

image

2) On install, the icons are first downloaded then cached on the individual user's machine, we do this for performance reasons. They are in this folder C:\Users\[UserId]\AppData\Local\Microsoft\Office\16.0\Wef\Resources - if we fail to locate the downloaded icon in the Resources, that's when we show the default icon. (I'd also like to note that if you ever update the icon to rename it to something different so that we redownload it instead of using the existing old one on the user's machine). Also note that the cached icons in the Resources folder are for all add-ins with AppCommands on all Office apps. So if you delete all the cached icons under the Resources folder, you will need to wait for all the icons to get downloaded, if the icon is not ready yet the default icon will show. If your Excel has like 20 add-ins with a lot of icons, then fetching them will take some time. It might be interesting to have Fiddler open to see how long it's taking.

3) For Contextual tabs api, we require that you list all your URIs for the icons in the manifest so that we can download them in advance.

jrgilman commented 1 month ago

Hi @wh1t3cAt1k @jrgilman sorry for slow response. We're actively investigating this issue. @jrgilman May I know which add-in is able to repro this issue, could you share us manifest? That could be help us to investigate. Thanks.

Hi - apologies for the long response time, here is the manifest.xml in question: https://xlwings.artemis.bi/manifest

wh1t3cAt1k commented 1 month ago

@trigurl I confirmed that we don't return any no-cache headers for the icons.

However, there are a lot of icons on our contextual tab in all sizes up to 96x96.

It may take a while to download them. What is the cut-off time for displaying the icons?

We do not satisfy requirement 3 from your message. Where can I read more about it?

jrgilman commented 1 month ago

I just want to add that removing the Content-Control and Pragma headers from our response did indeed correctly this issue.

trigurl commented 1 month ago

@wh1t3cAt1k I just looked at the docs for Context Tab API, they can be found here: https://learn.microsoft.com/en-us/office/dev/add-ins/design/contextual-tabs

The download is async so it gets put onto the queue and Office will process them accordingly. If you have a lot of icons, and you click on your tab immediately, it's possible you might see the default icon because the icon wasn't ready. For majority of users, it's usually not an issue because when they start office, there's usually a small moment to figure out where they left off.

For requirement #3, all resources for your add-in need to be listed in the manifest including the contextual tabs. If it is' not clear, I can ask our tech writers to update the docs.

wh1t3cAt1k commented 1 month ago

@trigurl I could not find the requirement to reference contextual tab icons in the documentation link that you provided. Can you please pinpoint me to the correct location?

If you have a lot of icons, and you click on your tab immediately, it's possible you might see the default icon because the icon wasn't ready. For majority of users, it's usually not an issue because when they start office, there's usually a small moment to figure out where they left off.

Regarding this, I can attest that today our CEO @gmichaud has run into this issue and clients on slower machines experience it, too.

I would vote to consider increasing the limit and not show any icon as long as the async download queue is not empty - showing the default icon should only happen when the download failed altogether.

gmichaud commented 1 month ago

@wh1t3cAt1k it was not a slower machine, but rather a server accessed via RDP... icons never appeared or loaded, reinitializing the add-ins cache did not help.

Screenshot 2024-09-18 at 5 17 20 PM
trigurl commented 1 month ago

I've forwarded to someone on our team who is responsible for updating the docs as I don't see in there either, but it is necessary, and you MUST list all icons in the manifest XML file.

It is not recommended to delete the cache folder, this will delete all the icons for all Office applications on the user's machine and they will need to all be downloaded again and icons might not show up until multiple boots afterwards - and this doesn't only affect your add-in but all other add-ins as well.

Right now, our team is not taking any design changes to contextual tabs due to resource constrains.

wh1t3cAt1k commented 1 month ago

@trigurl I am fine adding that to the manifest file as long as I know what to do, before it is documented, can you put in an example here so that I can experiment?

trigurl commented 1 month ago

So here is snippet:

1) Go to the Resources element in the XML manifest then in Images element, (similar to how you specified your icons for your buttons on the Ribbon - https://learn.microsoft.com/en-us/office/dev/add-ins/develop/create-addin-commands)

`

// other resources....etc ` 2) JSON snippet -from the looks of your screenshot, you already set this up correctly and just missing the step 1 for the XML "tabs": [ { "id": "tab1", "label": "mytab1", "visible": true, "groups": [ { "id": "group1", "label": "mygroup1", "icon": [ { "size": 16, "sourceLocation": "https://myserver.microsoft.com/AppCommands/RibbonAPITest/images/TaskPanes.16x16x32.png" }, { "size": 32, "sourceLocation": "https://myserver.microsoft.com/AppCommands/RibbonAPITest/images/TaskPanes.32x32x32.png" }, { "size": 80, "sourceLocation": "https://myserver.microsoft.com/AppCommands/RibbonAPITest/images/TaskPanes.80x80x32.png" } ], "controls": [ { "type": "Button", "id": "button1", "enabled": true, "icon": [ { "size": 16, "sourceLocation": "https://myserver.microsoft.com/AppCommands/RibbonAPITest/images/TaskPanes.16x16x32.png" }, { "size": 32, "sourceLocation": "https://myserver.microsoft.com/AppCommands/RibbonAPITest/images/TaskPanes.32x32x32.png" }, { "size": 80, "sourceLocation": "https://myserver.microsoft.com/AppCommands/RibbonAPITest/images/TaskPanes.80x80x32.png" } ], "label": "mybutton1", "toolTip": "my button 1 tooltip", "superTip": { "title": "button 1 more info", "description": "button 1 does so and so when you click it." }, "actionId": "showTaskpanewiki" },
wh1t3cAt1k commented 1 month ago

@trigurl Angela, I am WIP in adding the icon URLs to our manifest file, but I found that the length of the image reference ID element is too restrictive: I filed https://github.com/OfficeDev/office-js/issues/4910

For now, I worked around by taking an MD5 hash of the "proper" icon name and taking the first 32 symbols.

Moreover, I'd like to confirm with you: which icon sizes are actually used by the Office? Our manifest became pretty large due to the number of assets that I'm referencing:

<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.16x16" DefaultValue="https://secure.velixo.com/icons-v2/16x16/add-16x16.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.20x20" DefaultValue="https://secure.velixo.com/icons-v2/20x20/add-20x20.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.24x24" DefaultValue="https://secure.velixo.com/icons-v2/24x24/add-24x24.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.32x32" DefaultValue="https://secure.velixo.com/icons-v2/32x32/add-32x32.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.40x40" DefaultValue="https://secure.velixo.com/icons-v2/40x40/add-40x40.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.48x48" DefaultValue="https://secure.velixo.com/icons-v2/48x48/add-48x48.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.64x64" DefaultValue="https://secure.velixo.com/icons-v2/64x64/add-64x64.png" />
<bt:Image id="34ec78fcc91ffb1e54cd85e4a0.80x80" DefaultValue="https://secure.velixo.com/icons-v2/80x80/add-80x80.png" />

This is just one icon. Coupled with the large number of icons that we have, it can be heavy to preload.

Therefore, could you please confirm if there are any sizes that are never used by the ribbon?

wh1t3cAt1k commented 1 month ago

@trigurl our team (@JuliaKassina) confirmed that adding icon URLs to the manifest prevents the issue from happening.

I suggest this gets documented publicly.

Can you please respond regarding the required icon sizes so that we can (potentially) reduce the number of assets that we're using?

wh1t3cAt1k commented 1 month ago

Documentation says:

Each icon must have three <Image> elements, one for each of the three mandatory sizes:

16x16
32x32
80x80

The following additional sizes are also supported, but not required.

20x20
24x24
40x40
48x48
64x64

However, it is a little vague regarding the "also supported" bit: are they actually being used by the host application, or currently providing these sizes gives no extra benefit?

I also call the following feature to your attention, I believe it has the opportunity to drastically reduce the asset sizes and maintenance burden on the developers:

https://techcommunity.microsoft.com/t5/microsoft-365-developer-platform/support-svg-as-icon-for-add-ins/idi-p/3797009

trigurl commented 1 month ago

Office tries to display the best icon based on the DPI for each platform (Win32, Mac/iOS and Online).  The 3 mandatory sizes are minimum but developers can provide more icons to improve the visuals on higher resolution screens. My recommendation is to start with the 3, then have your customers try it out. (Just an FYI if you have multi-monitors with say a low resolution and high resolution screen, the lower resolution icon will be used and if you drag it to the high resolution monitor the icon might look fuzzy).
Note - coworker says Online uses the 80x80 icon. Note 2 - our doc folks have updated the code snippets in the docs and I've signed off on them. Should see them live soon.

wh1t3cAt1k commented 1 month ago

I can confirm that adding all icons as bt:Image resources to the manifest fixes the issue. Thank you Angela.