Skip to content

fix(common): execute checks and remove placeholder when image is already loaded #55444

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

Closed

Conversation

arturovt
Copy link
Contributor

@arturovt arturovt commented Apr 20, 2024

With this commit, we're now able to perform checks even when the image has already
been loaded (e.g., from the browser cache), and its load event would never be triggered.
We use the complete property, as specified,
which indicates that the image state is fully available when the user agent has retrieved all
the image data. This approach effectively triggers checks, as we no longer solely rely on the
load event and consider that the image may already be loaded.

This will not remove the placeholder until the load event fires (and it won't fire if the
image is already "there").

This prevents memory leaks in development mode, as load and error event listeners are
still attached to the image element.

@arturovt arturovt force-pushed the fix/ng-optimize-complete-state branch 4 times, most recently from 0d687eb to b224ebd Compare April 20, 2024 23:20
@arturovt arturovt marked this pull request as ready for review April 22, 2024 16:05
@pullapprove pullapprove bot requested a review from dylhunn April 22, 2024 16:05
@AndrewKushnir AndrewKushnir requested review from kara and AndrewKushnir and removed request for dylhunn April 22, 2024 22:33
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release common: image directive labels Apr 22, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 22, 2024
…ady loaded

With this commit, we're now able to perform checks even when the image has already
been loaded (e.g., from the browser cache), and its `load` event would never be triggered.
We use the [complete](https://html.spec.whatwg.org/#dom-img-complete) property, as specified,
which indicates that the image state is fully available when the user agent has retrieved all
the image data. This approach effectively triggers checks, as we no longer solely rely on the
`load` event and consider that the image may already be loaded.

This will not remove the placeholder until the `load` event fires (and it won't fire if the
image is already "there").

This prevents memory leaks in development mode, as `load` and `error` event listeners are
still attached to the image element.
@arturovt arturovt force-pushed the fix/ng-optimize-complete-state branch from b224ebd to db5e8b3 Compare September 18, 2024 16:04
@arturovt arturovt changed the title fix(common): execute checks when image is already loaded fix(common): execute checks and remove placeholder when image is already loaded Sep 18, 2024
Comment on lines +1362 to +1364
if (img.complete && img.naturalWidth) {
callback();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atcastle could you please help review this change when you get a chance?

Copy link
Contributor

@atcastle atcastle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good change to me. I hadn't personally encountered the image directive getting stuck in the placeholder state, but I can see how race conditions could occur, and it makes sense to do the img.complete check.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 24, 2024
@AndrewKushnir AndrewKushnir removed the request for review from kara September 24, 2024 22:13
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 24, 2024
@alxhub
Copy link
Member

alxhub commented Sep 26, 2024

This PR was merged into the repository by commit c3115b8.

The changes were merged into the following branches: main, 18.2.x

@alxhub alxhub closed this in c3115b8 Sep 26, 2024
alxhub pushed a commit that referenced this pull request Sep 26, 2024
…ady loaded (#55444)

With this commit, we're now able to perform checks even when the image has already
been loaded (e.g., from the browser cache), and its `load` event would never be triggered.
We use the [complete](https://html.spec.whatwg.org/#dom-img-complete) property, as specified,
which indicates that the image state is fully available when the user agent has retrieved all
the image data. This approach effectively triggers checks, as we no longer solely rely on the
`load` event and consider that the image may already be loaded.

This will not remove the placeholder until the `load` event fires (and it won't fire if the
image is already "there").

This prevents memory leaks in development mode, as `load` and `error` event listeners are
still attached to the image element.

PR Close #55444
@arturovt arturovt deleted the fix/ng-optimize-complete-state branch September 26, 2024 21:22
and-oli pushed a commit to and-oli/angular that referenced this pull request Sep 30, 2024
…ady loaded (angular#55444)

With this commit, we're now able to perform checks even when the image has already
been loaded (e.g., from the browser cache), and its `load` event would never be triggered.
We use the [complete](https://html.spec.whatwg.org/#dom-img-complete) property, as specified,
which indicates that the image state is fully available when the user agent has retrieved all
the image data. This approach effectively triggers checks, as we no longer solely rely on the
`load` event and consider that the image may already be loaded.

This will not remove the placeholder until the `load` event fires (and it won't fire if the
image is already "there").

This prevents memory leaks in development mode, as `load` and `error` event listeners are
still attached to the image element.

PR Close angular#55444
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package common: image directive target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants