[MGNLFE-26] inEditor context incorrect if mgnlRefresh has not been added to the window yet Created: 19/Feb/20 Updated: 03/Jun/20 Resolved: 23/Apr/20 |
|
| Status: | Closed |
| Project: | Magnolia Frontend Helpers |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 1.0.1 |
| Type: | Bug | Priority: | Major |
| Reporter: | Christopher Zimmermann | Assignee: | Canh Nguyen |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 3d | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| Acceptance criteria: |
Empty
|
||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||
| Date of First Response: | |||||||||
| Epic Link: | SPA Editor | ||||||||
| Sprint: | 6.2.1 Ramp-up 22 | ||||||||
| Story Points: | 2 | ||||||||
| Description |
|
Timebox for investigation: 2 SP There is an "inEditor" value on the RendererContext. If the mgnlRefresh JS function not being loaded yet on the frontend, the react (not sure about angular) FE library functions that should tell it if it is in the authoring environment are getting an incorrect result. |
| Comments |
| Comment by Bartosz Staryga [ 09/Apr/20 ] |
|
Angular also breaks as it uses the same `inEditor` from template annotation lib which checks against parent.mgnlRefresh. Maybe renderer could wait until mgnlRefresh is in place then add src to iframe? |
| Comment by Bartosz Staryga [ 16/Apr/20 ] |
|
canh.nguyen so we still have small race condition |
| Comment by Bartosz Staryga [ 20/Apr/20 ] |
|
Yo canh.nguyen EditorContextHelper.refresh() still uses the mgnlRefresh so it does not really solve the race condition issue. If you'd like to solve it with front end then maybe you'd have to check if mgnlRefresh is there and if not then add timeout and call the EditorContextHelper.refresh() again in let's say 300ms. |
| Comment by Christopher Zimmermann [ 20/Apr/20 ] |
|
Agree the timeout approach could be a goodenough fix. But it would also need to "give up" at some point (for example after 3 seconds).
But, disagree with "happens so little times that we can ignore it for now, and do sth about it later on when/if it really becomes bothering us" It consistantly happens the first time you try the page editor, which is the worst time for it not to work. bstaryga I know you have a workaround in the demo projects (thank you!) , but the library needs to be fixed as soon as possible. |
| Comment by Bartosz Staryga [ 20/Apr/20 ] |
|
czimmermann hehehe, I did not wanna sound to pushy |
| Comment by Canh Nguyen [ 20/Apr/20 ] |
|
Hi czimmermann, bstaryga, Please take a look at this code https://git.magnolia-cms.com/projects/MODULES/repos/frontend-helpers/pull-requests/23/diff#packages/react-editor/src/component/EditableArea/EditableArea.js?t=29 When an area is attached into the page, it will call refresh to update edit bars. I cannot reproduce the issue on my magnolia-react-sample repo, and I think I know why: In my sample app, it calls REST APIs, so it takes time to fetch the contents. If my assumtion is correct, this issue will never happen in the real world, because the app must call REST APIs to fetch data, and after fetching data finished, mgnlRefresh must be ready to be called. |
| Comment by Christopher Zimmermann [ 20/Apr/20 ] |
|
It can happen in the real world. Try this repo at this branch: https://git.magnolia-cms.com/projects/DEMOS/repos/minimal-headless-spa-demos/browse?at=refs%2Fheads%2Fadd-theme (Before bartosz changed it to work around this bug.) The library methods should be robust enough to handle any usage. |
| Comment by Canh Nguyen [ 20/Apr/20 ] |
|
Hi czimmermann With the improvemt from inEditor(), it will always render http comments if it's inside editor iframe. It needs to call refresh() when everything is finished. I suspect that your demo repo is rendered too fast when mgnlRefresh is not ready yet. You can add a timeout on your demo to call refresh function. |
| Comment by Christopher Zimmermann [ 20/Apr/20 ] |
|
Why should I as a developer using the library and magnolia, have to add a timeout in my code? How should I know that I might need to do it? Shouldn't the library "just work"? |
| Comment by Bartosz Staryga [ 20/Apr/20 ] |
|
Agree 100% with czimmermann. |
| Comment by Canh Nguyen [ 20/Apr/20 ] |
|
Hi czimmermann Let me try the demo repo. Could you tell me more about the environment and steps to reproduce the issue please? When I run magnolia-react-sample with the old react-editor before, I debuged the code and inEditor() returned false, but when the page finished loading, I could see all edit bars. I was running the sample on CE community/CE community demo/ dx-core demo. I've never seen this issue. When I picked this ticket, I thought the issue was that it didn't create HTML comments then page editor could not render green bars. I mention timeout for the demo repo only, because the demo may not fetch data, then it gets rendered supper fast. For other cases like my sample repo, it fetches data to render the page, and it works well. Actually, I don't like timeout, it should be rendered when everything is ready. I'll try the demo repo, if the issue is that spa-renderer on server side does not call the script to render green bars when the frame is ready, then I will fix it on server side. |
| Comment by Canh Nguyen [ 21/Apr/20 ] |
|
Hi czimmermann I've checked minimal-headless-spa-demos, but I couldn't reproduce the defect with the fix of my PR. It always showed green bars when I refreshed the page. Please check my repo https://git.magnolia-cms.com/users/canh.nguyen/repos/minimal-headless-spa-demos/compare/diff?targetBranch=refs%2Fheads%2Fadd-theme&sourceBranch=refs%2Fheads%2Fadd-theme&targetRepoId=4246
|
| Comment by Bartosz Staryga [ 21/Apr/20 ] |
|
canh.nguyen as I mentioned before it is sth that rarely happen. I am just pointing out that there is possibility there that site inside iframe fetches data and renders before parent prepared mgnlRefresh. |
| Comment by Christopher Zimmermann [ 21/Apr/20 ] |
|
I got permission denied on that above linke canh.nguyen |
| Comment by Canh Nguyen [ 21/Apr/20 ] |
|
Could you please check again czimmermann |
| Comment by Christopher Zimmermann [ 21/Apr/20 ] |
|
Thanks, I can access the link now. |
| Comment by Christopher Zimmermann [ 21/Apr/20 ] |
|
OK - I've reviewed again and this works for me, and addresses the issue stated in the description. The mgnlRefresh not being available is still an issue but is lower priority. I'll put that part of it in a new ticket, so that we can expidite bringing this important fix in. |