Fix two UI bugs: JS error in imagediff.js, 500 error in diff/compare.tmpl #19494
Conversation
wxiaoguang
changed the title
GiteaBot
added
the
lgtm/need 1
GiteaBot
added
lgtm/done
| const width = svg?.width?.baseVal; | ||
| const height = svg?.height?.baseVal; | ||
| if (width === undefined || height === undefined) { | ||
| return null; // in case some svg is invalid or doesn't have the width/height |
There was a problem hiding this comment.
SVG without width/height is not invalid. The attributes default to auto which essentially means 100% to fill the parent container:
https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/width#svg
Not sure whether we should actually fill the container or default them to something more sane like 400px maybe.
There was a problem hiding this comment.
What's your suggestion?
There was a problem hiding this comment.
Ideally I'd render with a max-width/max-height somewhere between 40% and 50% container width and render those size-less SVGs with that size as well. Maybe it can be done in CSS only, but JS-based solutions are also an option.
There was a problem hiding this comment.
Another simpler option may be to default to 300px width / 150px height as defined in the SVG spec here for rendering in CSS contexts:
https://svgwg.org/specs/integration/#svg-css-sizing
If any of the sizing attributes are missing, resolve the missing ‘svg’ element width to '300px' and missing height to '150px' (using CSS 2.1 replaced elements size calculation).
There was a problem hiding this comment.
Could you help to propose a PR about the refactoring?
There was a problem hiding this comment.
I can check what can be done, but it would be helpful to have a proper testing repo. https://try.gitea.io/wxiaoguang/test/pulls/6/files seems unsuitable as is has a namespace-less SVG which will never properly render.
There was a problem hiding this comment.
But https://try.gitea.io/wxiaoguang/test/pulls/6/files is why the bug occurs ..... end users made the mistakes. This PR just make UI more friendly to end users if they made mistakes.
What do you mean by suitable?
There was a problem hiding this comment.
Suiteable in the sense that it renders something, e.g. with namespace and some visible shape data.
There was a problem hiding this comment.
I can not understand why you need that .... the purpose of this PR is to handle invalid SVG gracefully.
If you work on a correct SVG, it's not related to this problem directly.
If you need some work for some special SVG, you can just prepare it .......... it would not be too difficult to construct a SVG manually by typing <svg> ...
There was a problem hiding this comment.
Yes, of course I can/will create test files myself.
Fix two UI bugs:
imagediff.js, caused by invalid svg. Replace deprecated rootElement with documentElement.diff/compare.tmpl, the$.Contextwas lost.Close #19414 (JS error in imagediff.js)
Screenshot after this fix: