Skip to content
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

[runtime-dom]: when <img> width and height are of string type, they will be set to 0 #7658

Closed
zh-lx opened this issue Feb 6, 2023 · 7 comments

Comments

@zh-lx
Copy link
Contributor

zh-lx commented Feb 6, 2023

Vue version

3.2.47

Link to minimal reproduction

https://sfc.vuejs.org/#eNp9UMtOwzAQ/JXFl17quA+KIAqV+A9fnHSTGGrHrJ0Wqcq/s24qQCBxsmd3PDOei3gJoTiNKEpRxYZsSBAxjWGvvXVhoAQXIGxhgpYGBwumLrTXvlIzm3kMErpwNAkZAVT9em9dVyk+r/hOSjjbQ+pLWK/ARvADOXME4w/Qo+14sbkPH3lj6ttOyvktC0Gk5lmLPqUQS6XOWAfTvBWvsRioU7YZvIzvoyGUkV8ei8ddbR62q6ZdNbt282Rwi9sinjot5hSstV4xyNZ9YpTNGSt2rNTXV8RSzA1IZwK7DZ47uuRQ+raIWpRwneQZN5Pxd9DYNrnZW06+FTT6ZB0WGJ2saThHJBbWYvlDQ/HwhCQJ/QEJ6T/NX9Q/ull20n4S0yeVPKVf

Steps to reproduce

Look the preview:
The height of <img> is 0, but it should be 24px

What is expected?

The height of <img> should be 24px

What is actually happening?

When height and width of some embedded element such as <img><video><source> and <canvas> are string type, they will be rendered as 0. And they are rendered normally when they are number type. This is because of the height and width of these elements must be set as attribute when they are string type, but in patchProp they are directly modified by patchDOMProp instead of patchAttr.

image

System Info

No response

Any additional comments?

No response

@zh-lx zh-lx changed the title Render exception when <img> width and height are of string type when <img> width and height are of string type, they will be set to 0 Feb 6, 2023
@zh-lx zh-lx changed the title when <img> width and height are of string type, they will be set to 0 [runtime-dom]: when <img> width and height are of string type, they will be set to 0 Feb 6, 2023
@edison1105
Copy link
Member

edison1105 commented Feb 7, 2023

Duplicate of #2801
the height or width must be an integer without a unit.
see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img?retiredLocale=de#attr-height

@LinusBorg
Copy link
Member

I'm with edison here. The developer should provide a number as that's what's expected by the HTML spec. We should not start adding code to special handle such cases.

@LinusBorg LinusBorg closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2023
@zh-lx
Copy link
Contributor Author

zh-lx commented Feb 7, 2023

Although the description of width on MDN must be a integer, it can also be a string type when used in html.I think the description on mdn is not necessarily accurate, and we should be more consistent with the actual use of html.

Here is an example to prove my point:

The attribute of <input> in mdn also has width and height and their description is consistent with that of <img>. But in fact, the width of input can only be changed through style. Neither setAttribute nor prop can change its width, even if its value is a integer. The following is the example about <input >:

https://sfc.vuejs.org/#eNp9j71Ow0AMx1/FeAlITa4MLFVaiY03YPFSEqdJlfuQ75IOUd4dX4oQAonN/w//ZC/4GkI1T4wHrGMjQ0gQOU3hRG6wwUuCBYQ7WKETb6HQakGOXONdTGDjBY45fyzeeBw9vHsZ24fiiVxt7jgFqUhsw3hOrAqg7p9Py7Itr2ttVG3u4MKUYC6tb3k8EmpOCLehTb2ql/2eUIu1+WbhDu83lvYcqmv0Tr9YMou+gkh4gM3Jnt6eNWGfUogHY2LX5N+vsfJyMTpVMrk0WK442vJD/C2yKJhw94Nh1JxZSmHXsrD8x/xV/cPN2JXciusn32yHkA==

@LinusBorg
Copy link
Member

it can also be a string type when used in html

Certain browsers may support that, but it's not in the spec:

The attributes, if specified, must have values that are valid non-negative integers.

https://html.spec.whatwg.org/multipage/embedded-content-other.html#attr-dim-height

Vue also supports strings, by the way, as long as they are convertible to integers which the spec expects as values. So if you provide a string containing just a number, free of a unit ('100' instead of '100px'), SSR and CSR work the same and work fine:

https://sfc.vuejs.org/#eNp9UEFOwzAQ/MriSy84TluKIAqV+IcvTrpJDLVj1k6LVOXvrNsKKpA42Tsznh3PSbyGUBwmFJWoY0s2JIiYprDV3rowUoITEHYwQ0ejgwVLF9prX6uLmnU8JHRhbxLyBFAPy611fa34PM93UsLR7tJQwbIEG8GP5MwejN/BgLZnYvUQPjNjmisn5eUtG0Gk9kWLIaUQK6WO2ATTvhdvsRipV7YdvYwfkyGUkV/ui6dNYx7XZduV7aZbPRtc47qIh14LqM4x2GyxLMtFBvL+Id0givfW6vtD4l5cepDOBN45em7qlKPpKxG1qOCMZIz7yfNP3Ni1ud9rWr4VNPlkHRYYnWxoPEYkNtbi/sZDMXhAkoR+h4T0n+cv6R/fbDtrP4v5CxFNpeI=

So this PR would "only" add code to be more compatible with nonstandard browser behavior. That is something we could want, but I'm still not exactly convinced it's worth being another special case to have in the codebase.

@Justineo in what way does this break in certain cases in CSR?

@zh-lx
Copy link
Contributor Author

zh-lx commented Feb 7, 2023

it can also be a string type when used in html

Certain browsers may support that, but it's not in the spec:

The attributes, if specified, must have values that are valid non-negative integers.

https://html.spec.whatwg.org/multipage/embedded-content-other.html#attr-dim-height

Vue also supports strings, by the way, as long as they are convertible to integers which the spec expects as values. So if you provide a string containing just a number, free of a unit ('100' instead of '100px'), SSR and CSR work the same and work fine:

https://sfc.vuejs.org/#eNp9UEFOwzAQ/MriSy84TluKIAqV+IcvTrpJDLVj1k6LVOXvrNsKKpA42Tsznh3PSbyGUBwmFJWoY0s2JIiYprDV3rowUoITEHYwQ0ejgwVLF9prX6uLmnU8JHRhbxLyBFAPy611fa34PM93UsLR7tJQwbIEG8GP5MwejN/BgLZnYvUQPjNjmisn5eUtG0Gk9kWLIaUQK6WO2ATTvhdvsRipV7YdvYwfkyGUkV/ui6dNYx7XZduV7aZbPRtc47qIh14LqM4x2GyxLMtFBvL+Id0givfW6vtD4l5cepDOBN45em7qlKPpKxG1qOCMZIz7yfNP3Ni1ud9rWr4VNPlkHRYYnWxoPEYkNtbi/sZDMXhAkoR+h4T0n+cv6R/fbDtrP4v5CxFNpeI=

So this PR would "only" add code to be more compatible with nonstandard browser behavior. That is something we could want, but I'm still not exactly convinced it's worth being another special case to have in the codebase.

@Justineo in what way does this break in certain cases in CSR?

All right, thanks for your answer.

@starcsb
Copy link

starcsb commented Feb 8, 2023

it can also be a string type when used in html

Certain browsers may support that, but it's not in the spec:

The attributes, if specified, must have values that are valid non-negative integers.

https://html.spec.whatwg.org/multipage/embedded-content-other.html#attr-dim-height

Vue also supports strings, by the way, as long as they are convertible to integers which the spec expects as values. So if you provide a string containing just a number, free of a unit ('100' instead of '100px'), SSR and CSR work the same and work fine:

https://sfc.vuejs.org/#eNp9UEFOwzAQ/MriSy84TluKIAqV+IcvTrpJDLVj1k6LVOXvrNsKKpA42Tsznh3PSbyGUBwmFJWoY0s2JIiYprDV3rowUoITEHYwQ0ejgwVLF9prX6uLmnU8JHRhbxLyBFAPy611fa34PM93UsLR7tJQwbIEG8GP5MwejN/BgLZnYvUQPjNjmisn5eUtG0Gk9kWLIaUQK6WO2ATTvhdvsRipV7YdvYwfkyGUkV/ui6dNYx7XZduV7aZbPRtc47qIh14LqM4x2GyxLMtFBvL+Id0givfW6vtD4l5cepDOBN45em7qlKPpKxG1qOCMZIz7yfNP3Ni1ud9rWr4VNPlkHRYYnWxoPEYkNtbi/sZDMXhAkoR+h4T0n+cv6R/fbDtrP4v5CxFNpeI=

So this PR would "only" add code to be more compatible with nonstandard browser behavior. That is something we could want, but I'm still not exactly convinced it's worth being another special case to have in the codebase.

@Justineo in what way does this break in certain cases in CSR?

I have the same problem. The description of width of <img> in mdn is that must be an integer, but it is not necessarily related to using setAttribute or prop to update width? The fact is that no matter what the type of this attribute is, all browsers will retain the value given by the user on the dom. However, in Vue, if the user gives width a non-numeric string, width will be set to 0 on the actual dom instead of keeping the value given by the user, which is completely illogical.

@Justineo
Copy link
Member

Justineo commented Feb 8, 2023

@LinusBorg

in what way does this break in certain cases in CSR?

if (
nc >= StringifyThresholds.NODE_COUNT ||
ec >= StringifyThresholds.ELEMENT_WITH_BINDING_COUNT
) {

There's currently a createStaticVNode optimization which stringifies vnodes into HTML so it works similar to SSR. You can see the difference here.

But if we decided not to support non-standard syntax which seems to be supported by all major browsers, current behavior should be fine. (And I just found out that browsers actually support not only ${value}px values, but also all string values starts with a number (eg. 24vue) and extract the number value with logic similar to parseInt.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants