Problem/Motivation
Currently the SVG icons aren't preloaded. When you click and hold on an icon you haven't clicked before, the icon entirely disappears for a few seconds while its active version is loaded.
Proposed resolution
Either find a way to preload the active state icons, or find a way to reduce the flickering effect.
Remaining tasks
Manual testing of the latest MR
Screencasts linked to in the user interface changes section
User interface changes
Hopefully, less of a jarring user experience when clicking icons.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#37 | after.gif | 4.04 MB | lauriii |
#37 | before.gif | 2.67 MB | lauriii |
#8 | fix_flicker-2209063-8.patch | 855 bytes | tarekdj |
#2 | 2209063-2-fix-flicker-active-icons.patch | 12.26 KB | Sam152 |
Issue fork drupal-2209063
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
Sam152 CreditAttribution: Sam152 commentedComment #2
Sam152 CreditAttribution: Sam152 commentedHere is an initial patch which attempts to solve this issue. It uses :before and :after to put one icon on top of the other, while the active icon is loading. The end result is that the old icon stays in place while the new one loads.
It's more a proof of concept than anything else, I'd be interested to hear what other solutions people have in mind.
Comment #3
Sam152 CreditAttribution: Sam152 commentedComment #5
thehong CreditAttribution: thehong commentedPut one icon on top of other does not make the icon load faster. I think to preload images, we need a custom javascript code, a custom module maybe needed.
Comment #7
Wim LeersI think #2 is … creative, but quite fishy :D
The only way I can think of to do this is by having some JS preload the SVGs for the toolbar items, possibly using a heuristic such as only doing the preloading when the user hovers over the toolbar.
Comment #8
tarekdj CreditAttribution: tarekdj commentedAttached patch is a full css fix. Yes! It preloads all images in the same time but it doesn't need javascript.
Comment #9
Wim Leers#8 is also creative, but equally fishy: it won't work if additional top-level admin menu links are added. Or if some are removed, then too many images will be preloaded.
I honestly don't think we can solve this issue in a reasonable way. This is up to browsers to fix. They should be smart enough to preload images that are used for alternative link states.
Comment #10
Sam152 CreditAttribution: Sam152 commentedI agree this is probably something that should be addressed at the browser level, however the reality is the current user experience is jarring and should still be addressed. I would also disagree with the "high latency" qualification, even locally there is a noticeable flicker.
#8's abuse of multiple background images seems fine due to the fact that we no longer support IE8. One of the issues might be as the heuristics around asset loading get better, could browsers perhaps opt out of loading assets attached to display:none elements? I remember perhaps reading this had already been implemented in some mobile browsers?
If javaScript is going to deliver the only non-fishy solution, I think it would be worth investigating.
Comment #11
Wim LeersOh, wow! Can you look in your browser's inspector and see whether the requests take a long time to be answered by your web server then?
Agreed.
Comment #12
LewisNyman CreditAttribution: LewisNyman commentedThere are a few issues that could fix this problem, I've added one to the related issues. I'd rather we fix this for all situations then but a short term hacky fix in like this.
Comment #13
LewisNyman CreditAttribution: LewisNyman commentedThe problem with both of these issue is it required changing our implementation of icons across the whole of core. It would be nice to have a lightweight wrapper so we can make changes to how icons are handled in one place: #1849712: Add theming template and prepare logic for rendering icons
Comment #14
Wim LeersI'm tempted to postpone this on #1849712: Add theming template and prepare logic for rendering icons. OTOH, that issue has had so much activity and no consensus yet. And this is probably the most visible/noticeable implementations of SVG icons, so also the most important one.
Hence leaving this NW for now, but if that other issue starts to move forward fast, I think we might want to spend our time on that one first.
Comment #15
YesCT CreditAttribution: YesCT commentedchanging to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.
Comment #23
nod_Still an issue for slow connections
Comment #31
bnjmnmIf we rename the icon-including css files to use the pcss.css extension, they are targeted by a CSS build process already in core that was introduced with Claro. This process includes changing CSS using
url('./path/to/svg')
tourl("data:image/svg+xml etc...")
. Changing these to data uris eliminates the latency reported here.Comment #32
quietone CreditAttribution: quietone at PreviousNext commentedThis was a bugsmash target today. There is a new MR that needs manual testing. i have updated the Issue Summary accordingly.
Comment #33
lauriiiLeft comment on the MR.
I assume this bug would not exist for the core icons when using Claro because Claro overrides this CSS. I'm also wondering if we should do something to reach out to contrib maintainers to encourage all contrib modules adding new items to the toolbar to also preload their icons.
Comment #34
bnjmnmComment #35
bnjmnmComment #36
tim.plunkettComment #37
lauriiiLooks good. Thank you for addressing the feedback @bnjmnm!
Comment #38
lauriiiComment #40
ckrinaCommitted 22fa968 and pushed to 10.1.x. Thanks!
Comment #41
quietone CreditAttribution: quietone at PreviousNext commentedRemoving testing tag, there are gifs in #37