[Files] Simplify image component implementation and usage #145347

Dosant · 2022-11-16T12:14:47Z

Needed for #81345

Summary

While working on the Image Embeddable I've struggle with using the image component, especially making the blur hash part work as I wanted. The main problems were caused by a custom container around the image which had custom styling - I had to properly style it separately to fit my use-case. The other problem was the BlurHash as since it was implemented as a separate image with custom positioning, I couldn't easily position it exactly the same as I positioned the original image.

To make Image component easier to use I suggest the following changes. I also wanted to make the component thinner.

Use a single tag for the underlying image - both for the src and for blurhash. This way we can be sure the image and the placeholder are always position in the same way. To make it work we first set src=blurhash, and then when it is ready we set a real source. The trick here is that the browser loads the new src without removing the previous image.
- The downside is that we lost the logic with blurDelayExpired. This isn't great, but I think we can live with that considering the simplicity of the new approach. I also reduced the blur box size so that it calculates a bit more faster, as it would have to do this for every image now.
- Also, to solve all the edge cases, I had to make the blurred image the same size as the original image by stretching it back to the original size. The problem is that I had to make that part of the code async [Files] Simplify image component implementation and usage #145347 (comment)
The image partially implemented some features from EuiImage (like predefined set of sizes). Instead of keeping the partial implementation with some code copy paste, I simply suggest we use EuiImage underneath. This make our image a tiny wrapper around EuiImage which also supports blurhash.
Added a storybook that tests custom sizing with css.
We discussed with @jloleysens that maybe we don't needs custom viewport visibility logic and just use a native browser attribute. The browsers which don't support it will simply load the image eagerly (only Safari atm). I think the native approach is fine here and we should remove the custom code to reduce bug surface.

Dosant · 2022-11-16T13:57:45Z

src/plugins/files/public/components/util/image_metadata.ts

+  ctx.putImageData(imageData, 0, 0);
+
+  return new Promise((resolve) => {
+    // On this point canvas contains a downsized blurred image


This is the part I don't like but couldn't find the workaround.
To make the blurred image sizing behave just like the original image we must have the blurred image stretched back to the original size. The problem is that I couldn't find a way to do this synchronously, so now there is a moment in time where image component doesn't render nor the blurred version, nor the src version

Maybe add this as a comment in the code?

Maybe something like below. image param can be another canvas.

ctx2.drawImage(ctx, 0, 0, blurWidth, blurHeight, 0, 0, width, height);

Actually, can we just return canvas.toDataURL() once we have the first canvas set up and skip the async image loading step? 😄

@vadimkibana, the idea with the second canvas works. thanks

I was trying it before like this:

ctx2.drawImage(ctx1, 0, 0, width, height);

But it actually should be like this (drawImage can receive canvas element, not ctx):

ctx2.drawImage(canvas1, 0, 0, width, height);

Actually, can we just return canvas.toDataURL() once we have the first canvas set up and skip the async image loading step?

I needed the second step (the image loading or the second canvas) to resize the blurred image back to the original size, so it behaves the same as the original image

vadimkibana

Code changes LGTM.

vadimkibana · 2022-11-17T08:55:52Z

src/plugins/files/public/components/image/image.tsx

-    const { isVisible, ref: observerRef } = useViewportObserver({ onFirstVisible });
+export const Image = ({ src, url, alt, onLoad, onError, meta, ...rest }: Props) => {
+  const imageSrc = (src || url)!; // <EuiImage/> allows to use either `src` or `url`
+  const [currentImageSrc, onBlurHashLoaded] = useCurrentImageSrc(imageSrc, meta);


Just an idea, I'm curious if we could have it in the future something like:

const src = useObservable(file.src$);

E.g. the Content Management Cache could return a content item file object:

const ContentThumbnail = ({ id }) => { const { cache } = useContent(); const file = cache.get(id); // but, then what is next: (1) there could be a generic system for content items to return an associated image: const { src, width, height } = useObservable(file.img$); // or, (2) some content items could register "custom" metadata in the registry const { src, width, height } = useObservable(file.custom.img$); return <img src={src} ... /> };

That is, any content item could specify an associated image, but also for some content items, like dashboard we could create a screenshot to use as a thumbnail.

vadimkibana · 2022-11-17T09:16:32Z

src/plugins/files/public/components/util/image_metadata.ts

+  ctx.putImageData(imageData, 0, 0);
+
+  return new Promise((resolve) => {
+    // On this point canvas contains a downsized blurred image


Maybe something like below. image param can be another canvas.

ctx2.drawImage(ctx, 0, 0, blurWidth, blurHeight, 0, 0, width, height);

jloleysens

Great work @Dosant , I tested this locally and it is working as intended.

It's really cool that we can get rid of the viewport observer. Could we make the default for the Image component that it uses lazy loading (i.e., by default set it, and accept a special value false to set it to undefined?

(will finish my review later today)

jloleysens · 2022-11-16T14:35:15Z

src/plugins/files/public/components/image/image.stories.tsx

+    (Story) => {
+      React.useLayoutEffect(() => {
+        // @ts-ignore
+        window.__image_stories_simulate_slow_load = true;


Nice hack 😄

Generally adding hack code into production for stories/tests should be avoided, but I see why you have done it in this case.

Agree that such hacks must be avoided.
I thought of also using process.env.NODE_ENV !== 'production but looks like env is production is storybook :(

src/plugins/files/public/components/image/image.tsx

jloleysens · 2022-11-17T08:53:54Z

src/plugins/files/public/components/util/image_metadata.ts

+  ctx.putImageData(imageData, 0, 0);
+
+  return new Promise((resolve) => {
+    // On this point canvas contains a downsized blurred image


Maybe add this as a comment in the code?

jloleysens

Discussed offline and left a few comments in the code. Most notable:

Would be nice to address readability comments (up to you if you want to keep hook outside component though)
Can we return canvas.getDataUrl() from the first canvas and skip the async Image loading step?

Summary of changes, we are trading the following:

Only show the blurhash when the image is taking a long time to load
Fade the blurhash away when the image is actually ready (UX effect)

For a simplified implementation that still does most of what the initial implementation did. Great work man 👏🏻

jloleysens · 2022-11-17T09:59:29Z

src/plugins/files/public/components/util/image_metadata.ts

+  ctx.putImageData(imageData, 0, 0);
+
+  return new Promise((resolve) => {
+    // On this point canvas contains a downsized blurred image


Actually, can we just return canvas.toDataURL() once we have the first canvas set up and skip the async image loading step? 😄

Dosant · 2022-11-17T13:49:50Z

@elasticmachine merge upstream

Dosant · 2022-11-17T15:58:09Z

@elasticmachine merge upstream

Dosant · 2022-11-18T09:48:47Z

@elasticmachine merge upstream

kibana-ci · 2022-11-18T10:47:19Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`files`	68	62	-6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`files`	18	16	-2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`files`	27.0KB	27.0KB	+15.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`files`	19.4KB	17.1KB	-2.3KB

Unknown metric groups

API count

id	before	after	diff
`files`	260	254	-6

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`files`	3	2	-1
`fleet`	59	65	+6
`osquery`	108	113	+5
`securitySolution`	442	448	+6
total			+18

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`files`	7	6	-1
`fleet`	67	73	+6
`osquery`	109	115	+6
`securitySolution`	519	525	+6
total			+19

History

💚 Build #88875 succeeded 7b3ae18
💔 Build #88817 failed 0a5339e
💚 Build #88760 succeeded a7cb59d
💚 Build #88439 succeeded 351c5cc
💔 Build #88381 failed 8fa55f6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…146159) ## Summary Follow up to #145347 Needed for #81345 Partially fixes #145567 Two things: - [x] Handle errors related to blurhash loading. Make sure that the original image is loaded if blurhash is failed to generate src or failed to load - [x] Make blurhash appear after the delay. This is needed for better UX when the original image loads fast. Implemented using `css` animation where first part of the animation is a static state with `opacity: 0,` and then the image is revealed in the end. If the original image loads faster, then it appears instantaneously without the delay

Dosant added 3 commits November 16, 2022 13:13

Simplify image component implementation and usage

8fa55f6

stretch the blurred image to the original size

6897185

update tests

85f99fe

Dosant commented Nov 16, 2022

View reviewed changes

Dosant requested review from vadimkibana and jloleysens November 16, 2022 14:02

Dosant changed the title ~~Simplify image component implementation and usage~~ [Files] Simplify image component implementation and usage Nov 16, 2022

Dosant changed the title ~~[Files] Simplify image component implementation and usage~~ [Discuss][Files] Simplify image component implementation and usage Nov 16, 2022

Dosant added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) feature:Files labels Nov 16, 2022

Dosant marked this pull request as ready for review November 16, 2022 14:03

Dosant requested a review from a team as a code owner November 16, 2022 14:03

remove not needed css

351c5cc

Dosant added the release_note:skip Skip the PR/issue when compiling release notes label Nov 16, 2022

vadimkibana approved these changes Nov 17, 2022

View reviewed changes

jloleysens reviewed Nov 17, 2022

View reviewed changes

jloleysens approved these changes Nov 17, 2022

View reviewed changes

improvements

a7cb59d

Dosant changed the title ~~[Discuss][Files] Simplify image component implementation and usage~~ [Files] Simplify image component implementation and usage Nov 17, 2022

Merge branch 'main' into d/2022-11-16-image-component-improve

0a5339e

Dosant mentioned this pull request Nov 17, 2022

[Files] <Image/> blurhash support improvements #145567

Open

3 tasks

Merge branch 'main' into d/2022-11-16-image-component-improve

7b3ae18

Merge branch 'main' into d/2022-11-16-image-component-improve

6e5c2b1

Dosant enabled auto-merge (squash) November 18, 2022 09:49

Dosant merged commit e3146a0 into elastic:main Nov 18, 2022

kibanamachine added the v8.7.0 label Nov 18, 2022

kibanamachine added the backport:skip This commit does not require backporting label Nov 18, 2022

Dosant mentioned this pull request Nov 23, 2022

[Files] Delay <Image/> blurhash reveal and handle blurhash errors #146159

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Files] Simplify image component implementation and usage #145347

[Files] Simplify image component implementation and usage #145347

Dosant commented Nov 16, 2022 •

edited

Loading

Dosant Nov 16, 2022

jloleysens Nov 17, 2022

vadimkibana Nov 17, 2022

jloleysens Nov 17, 2022 •

edited

Loading

Dosant Nov 17, 2022

vadimkibana left a comment

vadimkibana Nov 17, 2022

vadimkibana Nov 17, 2022

vadimkibana Nov 17, 2022

vadimkibana Nov 17, 2022

jloleysens left a comment

jloleysens Nov 16, 2022

jloleysens Nov 17, 2022

Dosant Nov 17, 2022

jloleysens Nov 17, 2022

jloleysens left a comment

jloleysens Nov 17, 2022 •

edited

Loading

Dosant commented Nov 17, 2022

Dosant commented Nov 17, 2022

Dosant commented Nov 18, 2022

kibana-ci commented Nov 18, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

[Files] Simplify image component implementation and usage #145347

[Files] Simplify image component implementation and usage #145347

Conversation

Dosant commented Nov 16, 2022 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimkibana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Dosant commented Nov 17, 2022

Dosant commented Nov 17, 2022

Dosant commented Nov 18, 2022

kibana-ci commented Nov 18, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Dosant commented Nov 16, 2022 •

edited

Loading

jloleysens Nov 17, 2022 •

edited

Loading

jloleysens Nov 17, 2022 •

edited

Loading