Skip to content

Process Picture tags and multiple Images in a srcset#67

Merged
rkoopmans merged 7 commits intotinify:masterfrom
wcreateweb:bug/picture-in-picture
Sep 4, 2025
Merged

Process Picture tags and multiple Images in a srcset#67
rkoopmans merged 7 commits intotinify:masterfrom
wcreateweb:bug/picture-in-picture

Conversation

@tijmenbruggeman
Copy link
Copy Markdown
Collaborator

@tijmenbruggeman tijmenbruggeman commented Sep 2, 2025

With the compression feature turned on, the plug-in will parse images on template_redirect. This parsed elements from the body to replace them with a picture element. One overlooked scenario was elements already wrapped by a element.

Additionally, it was not semantically correct to have multiple sources of one srcset in the picture element. This scenario is now covered as well.

Changes

  • Added a unit test for the bug scenario
  • Filtered out picture elements when searching for img tags.
  • Improved documentation on various regexes as the regexes are getting quite complex
  • Created two classes, one for handling tags and one for elements. Shared functionality comes from the base class.

Considerations

  • Why not use DOMDocument to parse the html?
    DOMDocument is quite robust, it is not support or turned on for all servers/php runtimes. Therefor it is unsure wether we can use it or not.
  • Why not use an existing package to read DOM nodes or attributes?
    Adding a package should not be taken lightly. It can increase vulnerability and complexity. Parsing the page for elements is a relatively small feature and adding a package for this might not be worth it.

@tijmenbruggeman tijmenbruggeman marked this pull request as draft September 2, 2025 14:54
@tijmenbruggeman tijmenbruggeman self-assigned this Sep 3, 2025
@tijmenbruggeman tijmenbruggeman marked this pull request as ready for review September 3, 2025 11:52
@rkoopmans rkoopmans merged commit 0195316 into tinify:master Sep 4, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants