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.

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

EZP-28078: Embedding a Content Object in the RichText Editor makes page scroll up #920

Conversation

4 participants
@adamwojs
Copy link
Member

commented Nov 15, 2017

JIRA: https://jira.ez.no/browse/EZP-28078

Description

This PR contains the workaround patch for unwanted page scroll to the top when user embeds a content/image or choose link location from UDW. The solution is to save and restore scroll position after command execution.

The direct cause of this issue is focus call in https://github.com/ckeditor/ckeditor-dev/blob/release/4.5.x/core/editable.js#L90 (in case of embedding content).

@adamwojs adamwojs requested review from andrerom, sunpietro and dew326 Nov 15, 2017

@adamwojs adamwojs changed the base branch from master to 1.11 Nov 15, 2017

@adamwojs adamwojs changed the title [WIP] EZP-28078: Embedding a Content Object in the RichText Editor makes page scroll up EZP-28078: Embedding a Content Object in the RichText Editor makes page scroll up Nov 16, 2017

@@ -75,9 +75,14 @@ YUI.add('ez-alloyeditor-button-embed', function (Y) {
*/
_addEmbed: function (e) {
var contentInfo = e.selection.contentInfo,
scrollX = window.pageXOffset || document.documentElement.scrollLeft,

This comment has been minimized.

Copy link
@dew326

dew326 Nov 16, 2017

Member

As far as I know document.documentElement.scrollLeft and document.documentElement.scrollTop is for IE 8 and lower. We dropped support a long time ago for IE, so I think you can omit this.

@dew326

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

The same issue is on v2 and I wouldn't mind if you could create a patch for v2 ;)

@dew326

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

What about updating image and embed? It also fires UDW.

@andrerom

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

The same issue is on v2 and I wouldn't mind if you could create a patch for v2 ;)

lets finish this one first, no customers on v2 yet :P

@andrerom

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

@adamwojs / @dew326 I see the issue is fixed / improved in CKEditor 4.7.x: https://github.com/ckeditor/ckeditor-dev/blob/release/4.7.x/core/editable.js#L90 Any chance we could get Alloy folks to update CKeditor? At a minimum for v2 which will use newer version right?

EDIT: Reported this here: liferay/alloy-editor#797

@andrerom

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

@adamwojs Can you perhaps align with the fix in CKEditor? As 1. Consider to use scrollTop, 2. only apply on chome if that is only browser affected by this

@adamwojs

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2017

@adamwojs Can you perhaps align with the fix in CKEditor? As 1. Consider to use scrollTop, 2. only apply on chome if that is only browser affected by this

@andrerom Good idea :-)

adamwojs added some commits Nov 16, 2017

@ezsystems ezsystems deleted a comment from ezrobot Nov 16, 2017

@adamwojs

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2017

PR updated according to suggestions.

@andrerom
Copy link
Member

left a comment

+1

but lets try to change branch to 1.12 to see if we can get it to pass

@andrerom andrerom changed the base branch from 1.11 to 1.12 Nov 16, 2017

@andrerom andrerom closed this Nov 16, 2017

@andrerom andrerom reopened this Nov 16, 2017

@dew326

dew326 approved these changes Nov 17, 2017

@micszo

micszo approved these changes Nov 17, 2017

Copy link
Member

left a comment

Retest OK on v1.12.0 with patch! 👍

@andrerom andrerom merged commit 6d835b6 into ezsystems:1.12 Nov 17, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ezrobot/csslint Code review by ezrobot
Details
ezrobot/jshint Code review by ezrobot
Details
ezrobot/phpcsfixer Code review by ezrobot
Details
ezrobot/yuidoc Code review by ezrobot
Details

@adamwojs adamwojs referenced this pull request Jan 9, 2018

Merged

1.13 merge into 2.0 #938

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.