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.

Sign up for GitHub

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

Jump to bottom

Fix two UI bugs: JS error in imagediff.js, 500 error in diff/compare.tmpl #19494

Merged
merged 4 commits into from
Apr 26, 2022
Merged

Fix two UI bugs: JS error in imagediff.js, 500 error in diff/compare.tmpl #19494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e045d46
Fix UI bug
wxiaoguang Apr 25, 2022
830c93b
Merge branch 'main' into fix-ui-bug
6543 Apr 25, 2022
1a5e646
Merge branch 'main' into fix-ui-bug
6543 Apr 25, 2022
c9d62a5
Merge branch 'main' into fix-ui-bug
wxiaoguang Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion templates/repo/diff/compare.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@
<div class="twelve wide column issue-title">
{{.i18n.Tr "repo.pulls.has_pull_request" (Escape $.RepoLink) (Escape $.RepoRelPath) .PullRequest.Index | Safe}}
<h1>
<span id="issue-title">{{RenderIssueTitle .PullRequest.Issue.Title $.RepoLink $.Repository.ComposeMetas}}</span>
<span id="issue-title">{{RenderIssueTitle $.Context .PullRequest.Issue.Title $.RepoLink $.Repository.ComposeMetas}}</span>
<span class="index">#{{.PullRequest.Issue.Index}}</span>
</h1>
</div>
Expand Down
15 changes: 11 additions & 4 deletions web_src/js/features/imagediff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ function getDefaultSvgBoundsIfUndefined(svgXml, src) {
const DefaultSize = 300;
const MaxSize = 99999;

const svg = svgXml.rootElement;

const width = svg.width.baseVal;
const height = svg.height.baseVal;
const svg = svgXml.documentElement;
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@silverwind silverwind Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you help to propose a PR about the refactoring?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Suiteable in the sense that it renders something, e.g. with namespace and some visible shape data.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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> ...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, of course I can/will create test files myself.

}
if (width.unitType === SVGLength.SVG_LENGTHTYPE_PERCENTAGE || height.unitType === SVGLength.SVG_LENGTHTYPE_PERCENTAGE) {
const img = new Image();
img.src = src;
Expand All @@ -29,6 +31,7 @@ function getDefaultSvgBoundsIfUndefined(svgXml, src) {
height: DefaultSize
};
}
return null;
}

export default function initImageDiff() {
Expand Down Expand Up @@ -88,6 +91,10 @@ export default function initImageDiff() {
info.$image.on('load', () => {
info.loaded = true;
setReadyIfLoaded();
}).on('error', () => {
info.loaded = true;
setReadyIfLoaded();
info.$boundsInfo.text('(image error)');
});
info.$image.attr('src', info.path);

Expand Down