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

Inconsistency in down-casting the UIElements #8959

Closed
niegowski opened this issue Jan 30, 2021 · 2 comments · Fixed by #8998
Closed

Inconsistency in down-casting the UIElements #8959

niegowski opened this issue Jan 30, 2021 · 2 comments · Fixed by #8998
Assignees
Labels
package:engine squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@niegowski
Copy link
Contributor

📝 Provide a description of the improvement

I was checking why putting an UIElement inside some text formatted as a code is moving following part of that text and I realized that it's because we split all AttributeElements while inserting any element into the view (this is causing the content movement because a <code> element has the padding so it has 2 additional paddings inside after split):

const insertionPosition = this._breakAttributes( position, true );
const length = container._insertChild( insertionPosition.offset, nodes );

On the other hand, if we already have an UIElement and we wrap some content with an AttributeElement then we allow a UIElement to be wrapped with it (also EmptyElement and RawElement are allowed):

// Wrap the child if it is not an attribute element or if it is an attribute element that should be inside
// `wrapElement` (due to priority).
//
// <p>abc</p> --> <p><span class="foo">abc</span></p>
// <p><strong>abc</strong></p> --> <p><span class="foo"><strong>abc</strong></span></p>
else if ( isText || isEmpty || isUI || isRaw || ( isAttribute && shouldABeOutsideB( wrapElement, child ) ) ) {


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@niegowski niegowski added type:improvement This issue reports a possible enhancement of an existing feature. package:engine squad:core Issue to be handled by the Core team. labels Jan 30, 2021
@niegowski
Copy link
Contributor Author

niegowski commented Jan 30, 2021

I created a branch with the simple solution (the full solution should probably be able to handle multiple UIElements, but it seems to be a virtual case that never happens in real-life scenarios).

While thinking about this I realized that we could extend it to handle inline elements (like inline images) so the generated link would not break before/on/after the inline image but would be created properly on the whole range (I tested it with a simple flag isInline on the view element and it works perfectly in both view and data pipeline). Also funny thing is that it already works correctly in the data pipeline without any changes (just because of isEmpty check in the mentioned code fragment because an inline image is an empty <img> element).

@niegowski niegowski changed the title Inconsequence in down-casting the UIElements Inconsistency in down-casting the UIElements Jan 31, 2021
@niegowski
Copy link
Contributor Author

I have some questions about how to proceed with this:

  1. Instead of checking isEmpty || isUI || isRaw while wrapping with an AttributeElement we could add a property to the view Element that would indicate that this element can be in-text (won't break an AttributeElement). This property would be exposed in the same way as getFillerOffset and priority. This seems like a very similar case to the priority property (and is used in the same context - DowncastWriter). And what would be the name of this property? allowInText?
  2. Should we handle multiple in-text nodes in DowncastWriter#insert() or we can assume that it's a virtual scenario so there is no point in complicating code for it?

cc @Reinmar @scofalik

@niegowski niegowski added this to the iteration 40 milestone Feb 6, 2021
@niegowski niegowski self-assigned this Feb 6, 2021
Reinmar added a commit that referenced this issue Feb 22, 2021
Fix (engine): `DowncastWriter` should handle `UIElements` consistently while wrapping with and inserting them into attribute elements. Closes #8959.

Feature (engine): `ContainerElement` can be marked as `isAllowedInsideAttributeElement` in order to allow wrapping it with attribute elements. Useful for instance for inline widgets. Other element types (UI, Raw, Empty) have this flag on by default but it can be changed via `options.isAllowedInsideAttributeElement` to `false`. Read more in `DowncastWriter#create*()` methods documentation. Closes #1633.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
1 participant