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

EZP-31321: Replaced zero-width space with oe-upcast-placeholder #88

Merged
merged 2 commits into from
Jan 22, 2020
Merged

EZP-31321: Replaced zero-width space with oe-upcast-placeholder #88

merged 2 commits into from
Jan 22, 2020

Conversation

SerheyDolgushev
Copy link
Contributor

@SerheyDolgushev SerheyDolgushev commented Dec 2, 2019 •
edited by alongosz
Loading

Question Answer
JIRA issue EZP-31321, related to EZP-29882
Required by ezsystems/ezplatform-admin-ui#1151
Bug/Improvement yes
New feature no
Target version 1.1
BC breaks no
Tests pass yes
Doc needed no

The original fix for this issue was ezsystems/ezplatform-admin-ui#746. Which fixed the symptoms, but introduced zero-width spaces. Which are causing serious issues in
ezsystems/ezplatform-admin-ui#1034, and seems to be a nice reason for some very strange bugs in the future.

Before posting this PR, we checked the original issue to understand the reasons why it was happening. So the case is:

  1. The user creates new content with a rich text field. That field contains just an inline embed.
  2. The content is published and rendered fine.
  3. But when the same content is edited - inline embed is not picked by the online editor.

It happens because, in this case, the HTML which is passed to OE contains just a paragraph and child's span element, without any child content. And OE skips such kind elements during widgets upcasting. Adding a zero-width space makes that paragraph a non-empty element so its child's span is checked for widgets upcasting. But zero-width space adds additional complexity and might cause more serious issues/bugs. That's why our solution seems to be more simple and predicted. It just adds "ao-upcast-placeholder" placeholder in the described scenario. So span element for inline embed is not ignored if its parent has no and text nodes, and it is upcasted.

Related PR: ezsystems/ezplatform-admin-ui#1151

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@SerheyDolgushev SerheyDolgushev changed the base branch from master to 1.1 December 2, 2019 17:14
@SerheyDolgushev SerheyDolgushev mentioned this pull request Dec 2, 2019
Merged
2 tasks
@alongosz alongosz requested a review from dew326 December 3, 2019 08:57
@andrerom andrerom changed the title Dont use zero width sapce for EZP-29882 Dont use zero width space for EZP-29882 Dec 12, 2019
alongosz
alongosz reviewed Dec 12, 2019
View reviewed changes
src/lib/eZ/RichText/Resources/stylesheets/docbook/xhtml5/edit/core.xsl Outdated
@@ -585,7 +585,7 @@
<xsl:call-template name="ezattribute"/>
<xsl:apply-templates/>
<xsl:if test="./docbook:ezattribute or not(node())">
<xsl:text> </xsl:text>
<xsl:text>ao-upcast-placeholder</xsl:text>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make much sense to me. What is ao-upcast-placeholder and why I would want to expose this in OE? Is this shown as attribute value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just choose this placeholder and made a type there. It should be ae-upcast-placeholder. Basically its value does not matter at all. It just needs to be some non-space characters. It is passed only to OE, and so AlloyEditor upcasts the widget for it.
Without this placeholder, there will be following HTML in the editor:

...
<p><span some-custom-tag-attributes /></p>
...

And AlloyEditor will ignore that custom tag span for widget upcasting. The point is to get some paceholder within the span:

...
<p><span some-custom-tag-attributes>ao-upcast-placeholder</span></p>
...

And in this case, the custom tag span will be upcasted and transformed to the widget. It's widget will not contain the placeholder text at all.

@alongosz should we change the placeholder text? Do you have any better suggestions for that?

@SerheyDolgushev
Copy link
Contributor Author

Zero width space seems to also cause some issues with Solr indexing:

org.apache.solr.common.SolrException: Illegal character ((CTRL-CHAR,&#8203; code 14))

@alongosz alongosz changed the title Dont use zero width space for EZP-29882 Replaced zero-width space with oe-upcast-placeholder Jan 21, 2020
@alongosz alongosz changed the title Replaced zero-width space with oe-upcast-placeholder EZP-31321: Replaced zero-width space with oe-upcast-placeholder Jan 21, 2020
@alongosz alongosz added the Improvement New feature or request label Jan 21, 2020
alongosz
alongosz reviewed Jan 21, 2020
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that oe-upcast-placeholder would be better, "ao" doesn't mean anything to me.
I'll commit it and rebase.

src/lib/eZ/RichText/Resources/stylesheets/docbook/xhtml5/edit/core.xsl Outdated Show resolved Hide resolved
tests/lib/eZ/RichText/Converter/Xslt/_fixtures/xhtml5/edit/023-embedInline.xml Outdated Show resolved Hide resolved
.../lib/eZ/RichText/Converter/Xslt/_fixtures/xhtml5/edit/035-itemizedListWithNestedElements.xml Outdated Show resolved Hide resolved
tests/lib/eZ/RichText/Converter/Xslt/_fixtures/xhtml5/edit/023-embedInline.xml Outdated Show resolved Hide resolved
@micszo micszo self-assigned this Jan 22, 2020
micszo
micszo approved these changes Jan 22, 2020
View reviewed changes
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with ezsystems/ezplatform-admin-ui#1151.

@micszo micszo removed their assignment Jan 22, 2020
@lserwatka lserwatka merged commit 637f5bb into ezsystems:1.1 Jan 22, 2020
@lserwatka
Copy link
Member

@alongosz could you merge it up?

@alongosz
Copy link
Member

Merged up to master via 280f8d7.

Thank you @SerheyDolgushev 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micszo micszo micszo approved these changes

@dew326 dew326 dew326 approved these changes

@alongosz alongosz alongosz approved these changes

@adamwojs adamwojs adamwojs approved these changes

Assignees
No one assigned
Labels
Improvement New feature or request QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SerheyDolgushev @lserwatka @alongosz @adamwojs @dew326 @micszo