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

[AUT-3670] fix: removing hb special symbols around placeholder #404

Merged
merged 2 commits into from
Jun 21, 2024
Merged

[AUT-3670] fix: removing hb special symbols around placeholder #404

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a869633
fix: removing hb special symbols around placeholder
Jun 12, 2024
460d8f5
fix: after review fixes
Jun 13, 2024
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
5 changes: 4 additions & 1 deletion src/qtiItem/core/Container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ var Container = Element.extend({
//@todo : container rendering merging, to be tested
tpl = tpl.replace(elt.placeholder(), elt.render(renderer));
} else {
tpl = tpl.replace(elt.placeholder(), '{{{' + serial + '}}}');
tpl = tpl
.replace(elt.placeholder(), serial)
.replace('{', '{')
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: if '{' is a { sign - can we use a standard HTML entity for this: '{'
and possibly use the regular expression in order to match all occurrences:
.replace(/\{/g, '{').
Could you please test if it will work in your case?

Copy link
Contributor

Choose a reason for hiding this comment

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

thought: I'm not sure { is better than {. It is less readable. The numeric version is acceptable when replacing chars with entities using their Unicode value without bothering about a dictionary. But I'd rather suggest, whenever possible, to always use the named entities for the sake of readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch regarding single occurrence, but HB parse brokes only first open brace in the begin of placeholder, other { can be ignored.
Agreed with @jsconan { is more readable

.replace(serial, '{{{' + serial + '}}}');
Copy link
Contributor

@jsconan jsconan Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: The temporary replacement with the final value for transforming the remaining keywords is not robust as this final value may exist in other places. I'd rather suggest an approach where possible collision is avoided. Moreover, the tpl variable is shared for the whole loop and it may introduce issues when multiple elements are used. A better solution should be implemented.

Regarding the placeholder replacer, one could do this:

Suggested change
tpl = tpl
.replace(elt.placeholder(), serial)
.replace('{', '{')
.replace(serial, '{{{' + serial + '}}}');
// Use a robust placeholder for replacing the element while converting entities
const tmp = `<!##${Math.floor(Math.random() * Math.pow(2, 32)).toString(16)}##!>`;
tpl = tpl
.replace(elt.placeholder(), tmp)
// Replace each occurrence of braces with computed HTML entities
.replace(/([{}])/gm, (match, group) => `&#${group.charCodeAt(0)};`)
.replace(tmp, '{{{' + serial + '}}}');

However, it will still create issues when having multiple elements in the same template, See:
image

And with the original proposed solution:
image

thought: I'm not sure we can replace all braces this way. An approach would be to rewrite the loop, splitting it in several occurrences: a first loop to remove the elements with robust placeholders, another loop to neutralize the braces, and finally a last loop to replace back elements with their serial... I'll let you figure out the implementation of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestions. I changed a bit approach of replacing.
As I see, serials is unique for each item and this replacement is happen for every item in container, so I not really sure about collision possibility.
Also important detail -- only open brace before placeholder breaks the flow

elementsData[serial] = elt.render(renderer);
}
} else {
Expand Down
Loading