laurent22 / joplin

Joplin - the privacy-focused note taking app with sync capabilities for Windows, macOS, Linux, Android and iOS.
https://joplinapp.org
Other
45.52k stars 4.95k forks source link

[Bug] HTML image height cannot be specified #10755

Closed schlagmichdoch closed 1 month ago

schlagmichdoch commented 3 months ago

Operating system

Linux

Joplin version

Joplin 3.0.1 3 (prod, linux)

Desktop version info

Joplin 3.0.13 (prod, linux)

Client ID: 7433847af5fe4e838bf0bba7d2fe7980 Sync Version: 3 Profile Version: 47 Keychain Supported: No

Revision: 599cf5b

Backup: 1.4.1 Conflict Resolution: 1.2.3 Ez Table: 1.0.2 Favorites: 1.3.2 Home Note: 2.0.0 Inline TODO: 1.7.1 joplin-hackmd: 2.0.0 Menu items, Shortcuts, Toolbar icons: 1.1.0 MultiMarkdown Table Tools: 1.2.1 Note list and sidebar toggle buttons: 1.0.3 Note overview: 1.7.1 Note Tabs: 1.4.0 Persistent Editor Layout: 2.2.0 Quick Links: 1.3.2 Rich Markdown: 0.15.0 Simple Highlighter: 1.0.0 Spoilers: 1.0.6 Table Formatter Plugin: 1.2.1 Templates: 2.4.0 Text Colorize: 1.2.5

Current behaviour

To resize an image we have to use the html image syntax. While the width token works as expected, the height token is ignored.

![Joplin logo](https://joplinapp.org/favicon.ico)
<img src="https://joplinapp.org/favicon.ico" alt="Joplin logo" width="200" height="">
<img src="https://joplinapp.org/favicon.ico" alt="Joplin logo" width="100" height="200">
<img src="https://joplinapp.org/favicon.ico" alt="Joplin logo" width="" height="200">

Omitting the width or height token is equivalent to setting it to auto.

So current Joplin behaviour: image

Expected behaviour

As the original image dimensions are 48x48, image 1 and image 3 should render the same. Image 2 should be stretched into a vertical rectangle.

See GitHub render: Joplin logo

Joplin logo Joplin logo Joplin logo

Logs

No response

personalizedrefrigerator commented 2 months ago

This seems to be caused by the following CSS rule: https://github.com/laurent22/joplin/blob/6b1d31387b18f85942f87d93a7ccf36db58644e9/packages/renderer/noteStyle.ts#L316-L319

According to Joplin's development tools, this height: auto overrides the attribute-specified height: screenshot: img {line   max-width: 100%;line    height: auto;line}line img {line    border-style: none; line} line img left bracket Attributes Style right bracket {line strikethrough    height: 120px; end strikethrough line    aspect-ratio: auto 10 / 120;line    width: 10px; ...

schlagmichdoch commented 2 months ago

I can confirm the images are rendered correctly if I remove/deactivate the height: auto; token via the dev tools.

I'd like to have this line removed then: https://github.com/laurent22/joplin/blob/6b1d31387b18f85942f87d93a7ccf36db58644e9/packages/renderer/noteStyle.ts#L318

Until then, I'll add the following to my userstyle.css as a workaround:

/* Workaround to fix setting image height via attribute */
img {
    height: revert-layer;
}
srivastavaarpit977 commented 2 months ago

Hi @laurent22 plz assign this to me

personalizedrefrigerator commented 2 months ago

Hi @laurent22 @Terrance plz assign this issue to me.

We generally don't assign issues. (Though feel free to work on it anyway!)

MrPhenomenal3110 commented 2 months ago

Hey @schlagmichdoch @laurent22 is this issue still active ? Can I work on it ?

schlagmichdoch commented 2 months ago

This is an issue which revolves around a single line of css. It should not involve any overhead procedures as assigning issues, asking for permission to overtake the issue, or simply adding a pr which only includes the workaround solution which should not be part of a release. Please stop wasting the developers time.

If you have anything substantial to add, like experiences with the test or ideas how to solve this, simply express it here. When everything is ready, either I or someone in the dev team will prepare a PR.

Regarding the actual solution: The workaround overwrites the content of the currently shipped css file.

I'd suggest to simply omit the height attribute completely as it should be unset for images in a note. Any ideas why height was set to auto in the first place?

tomasz1986 commented 2 months ago

Any ideas why height was set to auto in the first place?

This is a good question because auto is the default value of height, so there should normally be no need to set it explicitly unless it's required to overwrite yet another value.

github-actions[bot] commented 1 month ago

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? If you require support or are requesting an enhancement or feature then please create a topic on the Joplin forum. This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

github-actions[bot] commented 1 month ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, feel free to create a new issue with up-to-date information.