Feature/ms 447/scorer highlight plugin - #101
Conversation
| function highlight(highlighter, selection) { | ||
| highlighter.highlightRanges(getAllRanges(selection)); | ||
| //Sending the highlighIndex to parent so that it can be saved on MS side | ||
| parent.postMessage({ event: 'indexUpdated', payload: highlighter.getHighlightIndex() }, '*'); |
There was a problem hiding this comment.
passing * is not a very good practice, as we know the target where we want the message to be sent we should use that, the only problem is we are not storing it in tao so IDK how and where we should store this info in tao. Failing to provide a specific target discloses the data you send to any interested malicious site.
There was a problem hiding this comment.
issue: Indeed, this might be a flaw.
suggestion: We use to have iframe by the past, and we have a legacy component to post message to a parent window: iframNotifier.
A property must be set by the parent to make the iframe elligible to communication.
However, I'm wondering if this module is not deprecated.
Even if you cannot control the parent window, couldn't you still declare a valid context from the config?
nitpick: Moreover, please always use window to namespace global context objects.
There was a problem hiding this comment.
Hmm, seems like iframNotifier doesn't work with cross-origin, hasAccess doesn't pass
https://github.com/oat-sa/tao-core-libs-fe/blob/06905acf3ca51d67c54e474fc59806c4172264fc/src/iframeNotifier.js#L22
There was a problem hiding this comment.
Yes, you are right. The iframeNotifier is meant to be used with fully controlled parents. If you use different domain names and servers it won't work. Moreover, I don't know if this module is still usable.
The overall idea is to secur the messaging, preventing the window to communicate with untrusted wrappers.
|
|
||
| width: 100%; | ||
| height: 100%; | ||
| padding-top: 3rem; |
There was a problem hiding this comment.
why do we need to touch the scale plugin? 🤔
There was a problem hiding this comment.
thought: Indeed, please be careful with the scale plugin, especially with its style. It might introduce regressions as it is very sensible.
There was a problem hiding this comment.
Unfortunately I have scroll bars for .devices-previewer - height: 100%; when the highlighter element is added to .content-wrapper 😔
There was a problem hiding this comment.
Can we add display: flex; flex-direction: column; to .content-wrapper or it will break something, WDYT?
There was a problem hiding this comment.
Well, this is hard to tell. AFAIR the devicesPreviewer plugin was a pain to make working on every browsers.
But the isn't .content-wrapper used in other places? If you target it, you must ensure it will only apply for the previewer.
There was a problem hiding this comment.
Seems like .item-previewer-scope -> .content-wrapper is used only here
jsconan
left a comment
jsconan
left a comment
There was a problem hiding this comment.
Code review only.
A few remarks, but the overall look good. GJ!
|
|
||
| width: 100%; | ||
| height: 100%; | ||
| padding-top: 3rem; |
There was a problem hiding this comment.
thought: Indeed, please be careful with the scale plugin, especially with its style. It might introduce regressions as it is very sensible.
| function highlight(highlighter, selection) { | ||
| highlighter.highlightRanges(getAllRanges(selection)); | ||
| //Sending the highlighIndex to parent so that it can be saved on MS side | ||
| parent.postMessage({ event: 'indexUpdated', payload: highlighter.getHighlightIndex() }, '*'); |
There was a problem hiding this comment.
issue: Indeed, this might be a flaw.
suggestion: We use to have iframe by the past, and we have a legacy component to post message to a parent window: iframNotifier.
A property must be set by the parent to make the iframe elligible to communication.
However, I'm wondering if this module is not deprecated.
Even if you cannot control the parent window, couldn't you still declare a valid context from the config?
nitpick: Moreover, please always use window to namespace global context objects.
| //hide highlighter menu by default | ||
| hider.hide(this.$highlighterTray); | ||
|
|
||
| testRunner.after('renderitem', function () { |
There was a problem hiding this comment.
nitpick: A namespace should be used here. So that you can unregister the handler upon destroy.
3 participants
Pull request summary
Related to MS-447.
Scorer's Highlight plugin
How to test
On MS side: