[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: GIF File MGNLFE-26.gif    
Issue Links:
Relates
relates to MGNLFE-37 mgnlRefresh is not available when the... Closed
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.
Looks like Iframe loads faster than Magnolia.

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  It did happen to me few times already.
Since you do not check against mgnlRefresh what is inside iframe is rendering freely and my app calls mgnlRefresh but it is still not there so I have normal page without any green bars.
Probably very edge case, as usually mgnlRefersh will be already there before iframe does all it's fetching and rendering but it does happen, few times happened for me. Hence current check is not 100% legit
Just something to keep in mind.

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.
But for now ignore me. This 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.

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  But simple timeout with give up as you suggested will do the trick

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

cc: bstaryga, sang.ngo

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.
You'll not gonna reproduce it 100% times. Happened to me few times only, but it did happen. Hence question, do we wanna cover this case so it'll never come back to us, or we ignore it.
If the time is problem here, I am happy to add PR.

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.

https://jira.magnolia-cms.com/browse/MGNLFE-37

Generated at Mon Feb 12 05:43:30 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.