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

Jump to bottom

LPS-117468 Fix unexpected background and border in Page Editor editables #157

Closed
wants to merge 1 commit into from
Closed

LPS-117468 Fix unexpected background and border in Page Editor editables #157

wants to merge 1 commit into from

Conversation

mateomustapic
Copy link

This PR is a followup on liferay/liferay-ckeditor#99 , it is a UI fix specific for page editor.
It removes “Unexpected background and border in Page Editor editables” described in https://issues.liferay.com/browse/LPS-117468

Steps to reproduce

  1. Edit Hello World page (or create a content page with a heading fragment)
  2. Double click on a text editable to start editing.

@liferay-continuous-integration
Copy link
Collaborator

To conserve resources, the PR Tester does not automatically run for every pull.

If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed.

If your pull was never tested, comment "ci:test" to run the PR Tester for this pull.

@mateomustapic
Copy link
Author

ci:test:sf

@mateomustapic
Copy link
Author

ci:test:relevant

@mateomustapic
Copy link
Author

88190122-2f2eff00-cc3a-11ea-8201-838d68ac991a

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 4d667230e673a0ba7f0b09b45a43282655cae6b3

Sender Branch:

Branch Name: LPS-117468_2
Branch GIT ID: 6fe9b736cf6e7b0e9f473e95fc65173a5f143732

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

Jenkins Build:test-portal-source-format#4407
Jenkins Report:jenkins-report.html
Jenkins Suite: sf
Pull Request:liferay-frontend#157
Spira Release:/Liferay DXP 7.3/Pull Request/ci:test:sf
Spira Release Build:master - mateomustapic > liferay-frontend - PR#157 - 2020-07-23[07:45:24]
Spira Jenkins Build:publish-spira-report#1282

@jbalsas
Copy link

jbalsas commented Jul 23, 2020 •
edited

Hey @mateomustapic, thanks for sending this over and for providing the context of the other conversation!

In order to make sure this is the proper fix, I'd expect to see the following information:

  • How was it working before as described by @p2kmgcl?
  • What specific change did we make that broke it?
  • Why did that change alter the existing behaviour?
  • What are the pros/cons of fixing this in general (AlloyEditor) rather than for the particular case of PageEditor

I'd expect to see this reasoning before we even attempted code to fix it. In general, if something was working before and now it isn't, we should not only fix it but also understand how it broke in the first place.

Can you please provide this additional information so we can evaluate how to proceed?

Thanks!

/cc @p2kmgcl, please keep an eye on this. Correct me if my assumption that it was working fine before but now it isn't is not correct. I wrote this based on your other comment here

@mateomustapic
Copy link
Author

mateomustapic commented Jul 23, 2020 •
edited

Hey @mateomustapic, thanks for sending this over and for providing the context of the other conversation!

In order to make sure this is the proper fix, I'd expect to see the following information:

  • How was it working before as described by @p2kmgcl?
  • What specific change did we make that broke it?
  • Why did that change alter the existing behaviour?
  • What are the pros/cons of fixing this in general (AlloyEditor) rather than for the particular case of PageEditor

I'd expect to see this reasoning before we even attempted code to fix it. In general, if something was working before and now it isn't, we should not only fix it but also understand how it broke in the first place.

Can you please provide this additional information so we can evaluate how to proceed?

Thanks!

/cc @p2kmgcl, please keep an eye on this. Correct me if my assumption that it was working fine before but now it isn't is not correct. I wrote this based on your other comment here

Hi @jbalsas , thanks for the feedback :).

Since the css class cke_editable, which is obviously causing this altered appearance is located in moono-lexicon skin in liferay-ckeditor, I presumed it was the last week skin changes that caused this.
The background-color and padding, modified in this PR, cke_editable are present ONLY in moono-lexicon skin.
I was suspicious why would the changes in this skin affect AlloyEditor, but it is obvious that it does.

However, this altered the existing appearance of all inline editors, but Page Editor (especially Hello World app on startup) is specific because its text color, which is white, so this added background-color alongside with padding makes it look ugly .
Due to this, I found it more reasonable to add these changes directly into layout-content-page-editor-web.

Edit:
I am referring to this file https://github.com/liferay/liferay-ckeditor/blob/3582464bdfb43a68a0c39bc5bc21b83cd6a0b853/skins/moono-lexicon/mainui.css#L187

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 21 out of 21 jobs passed

❌ ci:test:relevant - 75 out of 78 jobs passed in 8 hours 14 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 4d667230e673a0ba7f0b09b45a43282655cae6b3

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 9df5a717f14278f29e7460d7055a941d93e0b735

ci:test:stable - 21 out of 21 jobs PASSED
21 Successful Jobs:
ci:test:relevant - 75 out of 78 jobs PASSED
75 Successful Jobs:
For more details click here.

Failures unique to this pull:

Failures in common with acceptance upstream results at 4d66723:

@liferay-continuous-integration
Copy link
Collaborator

Jenkins Build:test-portal-acceptance-pullrequest(master)#6058
Jenkins Report:jenkins-report.html
Jenkins Suite: relevant
Pull Request:liferay-frontend#157
Spira Release:/Liferay DXP 7.3/Pull Request/ci:test:relevant
Spira Release Build:master - mateomustapic > liferay-frontend - PR#157 - 2020-07-23[09:09:03]
Spira Jenkins Build:publish-spira-report#4967

@jbalsas
Copy link

jbalsas commented Jul 24, 2020

I was suspicious why would the changes in this skin affect AlloyEditor, but it is obvious that it does.

While it is obvious that this is affecting AlloyEditor, it's not clear why. That's what I'm asking. When possible, we need to have a clear view of what's causing the issue. If the issue has been introduced close in time, then it's also good to actually trace back the origin to analyze it, find the best solution and prevent issues like that in the future.

As I pointed out at fix: remove padding and background-color in page editor .cke_editable #99:

AFAIK AlloyEditor uses whatever the version of CKEditor is in portal, and in portal we have a version with moono-lexicon... At least that's what I found out with @julien some weeks ago 🤷

Keep digging in your memory, I know this is somewhere around there: LPS-116201 Use moono-lisa skin in frontend-editor-alloyeditor-web because you thanked @julien for it! :)

There's no reason for AlloyEditor to use moono-lexicon which is why we did LPS-116201 Use moono-lisa skin in frontend-editor-alloyeditor-web.

Since PageEditor is instantiating AlloyEditor directly, it's either not picking the right configuration or picking one that doesn't set moono-lisa as its skin and the original fix was incomplete.

@mateomustapic
Copy link
Author

There's no reason for AlloyEditor to use moono-lexicon which is why we did LPS-116201 Use moono-lisa skin in frontend-editor-alloyeditor-web.

Since PageEditor is instantiating AlloyEditor directly, it's either not picking the right configuration or picking one that doesn't set moono-lisa as its skin and the original fix was incomplete.

I am digging into this. I think the mentioned commit above didn't even set moono-lisa skin correctly in the first place.
I found that when deploying frontend-editor-alloyeditor-web, the only skin present in tmp is moono :/ .

I will continue to look into this.

Screenshot at Jul 24 13-04-31

@mateomustapic
Copy link
Author

mateomustapic commented Jul 24, 2020 •
edited

hey @jbalsas and @carloslancha

After analysis on this I came to following information.
I found that this class cke_editableis shared between both Alloy Editor and CKEditor.
This caused this similar issue liferay/liferay-ckeditor#95.
Since cke_editable is shared class and there are no styles for it in moono-lisa, these specific styles which are defined only in moono-lexicon override and prevail at the end. This will happen to any shared css class between skins used in portal.

There are 2 solutions for this issue would be:

  1. To add this fix in layout-content-page-editor-web - as it is in this PR
    I think this is the one we should go with because this is the issue only in Page Editor

  2. To modify moono-lisa skin and modify cke_editable class there and completely remove background-color and padding from all AlloyEditor instances

What do you think?

@jbalsas
Copy link

jbalsas commented Jul 24, 2020 •
edited

There are 2 solutions for this issue would be:

I think there are more and that's what I was hinting when I specifically said:

Since PageEditor is instantiating AlloyEditor directly, it's either not picking the right configuration or picking one that doesn't set moono-lisa as its skin and the original fix was incomplete.

If you check PageEditor, it is using moono-lexicon and not moono-lisa.

So, the first step would be to identify where this can be changed so the editor is using the correct skin. This can be done:

  • Following LPS-116201 Use moono-lisa skin in frontend-editor-alloyeditor-web in the proper ConfigContributors for this case (there are 3, afaik).
  • In PageEditor js before instantiating AlloyEditor hardcoding the skin
  • In AlloyEditor, always forcing moono-lisa. This would be the more general way and probably safe enough since we don't support skinning the editor through regular ckeditor skins...

Once that's done, everything should be back to normal for this editor instances.

Now, there is a clear additional problem which should occur when we have 2 editors with different skins together in the same page. This would happen in Web Content where we have both an AlloyEditor for the content and a CKEditor for the Summary field. In there, the moono-lexicon styles will clearly bleed through to the AlloyEditor instance. For that, I feel we should want to make sure moono-lexicon styles remain in those editor instances. I'm not 100% sure if CKEditor has already some kind of API for this, but something like namespacing our css to include something like cke_lexicon should be enough to distinguish one skin from another.

Just to summarize:

  1. The issue described by @p2kmgcl could be fixed by making sure those editors have the proper skin in one of the described ways (or any other)
  2. The bleeding styles from skin to skin could be fixed by properly namespacing our styles.

@carloslancha
Copy link

Keep digging in your memory, I know this is somewhere around there: LPS-116201 Use moono-lisa skin in frontend-editor-alloyeditor-web because you thanked @julien for it! :)

Well, I'll talk to my doctor about this alzheimer guy that seems to be visiting me...

I'm not 100% sure if CKEditor has already some kind of API for this, but something like namespacing our css to include something like cke_lexicon should be enough to distinguish one skin from another.

I couldn't find anything in the API about namespacing the skin, maybe you'll be luckier than me.

Besides that, I found this in the docs: https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR.html#cfg-skinName

The skin to load for all created instances, it may be the name of the skin folder inside the editor installation path, or the name and the path separated by a comma.
Note: This is a global configuration that applies to all instances.

Don't know if Alloy Editor has some magic that would allow us to use different skins...

@jbalsas
Copy link

jbalsas commented Jul 27, 2020

I'm not 100% sure if CKEditor has already some kind of API for this, but something like namespacing our css to include something like cke_lexicon should be enough to distinguish one skin from another.

I couldn't find anything in the API about namespacing the skin, maybe you'll be luckier than me.

If there isn't, we could simply patch our fork to add the .cke_${skinName} class to the different roots that need it so we could then namespace our skin as .cke_moono-lexicon { ... }.

Besides that, I found this in the docs: https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR.html#cfg-skinName

The skin to load for all created instances, it may be the name of the skin folder inside the editor installation path, or the name and the path separated by a comma.
Note: This is a global configuration that applies to all instances.

Don't know if Alloy Editor has some magic that would allow us to use different skins...

AlloyEditor has no magic.

What I've briefly seen here makes me think that this is either outdated or misleading. Maybe the fact that the -lexicon skin is css and has no plugin.js has something to do with it, but both skins seem to indeed be taking effect.

@jbalsas
Copy link

jbalsas commented Jul 28, 2020

Sent a different PR to #175

@jbalsas jbalsas closed this Jul 28, 2020
@jbalsas jbalsas mentioned this pull request Jul 28, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carloslancha carloslancha Awaiting requested review from carloslancha

Assignees
No one assigned
Labels
ci:test:relevant - failure ci:test:sf - success ci:test:stable - success needs update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mateomustapic @liferay-continuous-integration @jbalsas @carloslancha