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

slotFor() cannot handle text nodes #11149

Closed
Reinmar opened this issue Jan 21, 2022 · 5 comments
Closed

slotFor() cannot handle text nodes #11149

Reinmar opened this issue Jan 21, 2022 · 5 comments
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 21, 2022

    editor.model.schema.register( 'myElement', {
        inheritAllFrom: '$block'
    } );

    editor.conversion.for( 'downcast' ).elementToStructure( {
        model: 'myElement',
        view: ( modelElement, { writer, slotFor } ) => {
            return writer.createContainerElement( 'div', { class: 'wrapper' }, [
                writer.createContainerElement( 'p', null, [
                    slotFor( 'children' )
                ] )
            ] );
        }
    } );

    setData( editor.model, '<myElement>foo[]</myElement>' );

I'd expect this in the view:

<$root>
    <div class=wrapper>
        <p>foo[]</p>
    </div>
</$root>

Instead, we get this:

<$root>
    <div class=wrapper>
        <p>foo</p>[]
    </div>
</$root>

NOTE: Exactly the same problem will occur when trying to use slots to handle <pre><code> in the code block feature.

@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. squad:core Issue to be handled by the Core team. labels Jan 21, 2022
@Reinmar
Copy link
Member Author

Reinmar commented Jan 25, 2022

Ideas:

  • A: Model to view selection conversion: pass a flag that we're looking for an innermost position.
    • A[<myElement>]B[</myElement>]C
    • A[<div class=wrapper><p><ui/>]B[<ui/></p></div>]C
    • Algorithm:
      • When slotFor() is used, remember on which index of its parent it was defined
      • Map children/positions of <myElement> relatively to that memorized position (<p> + index)
    • Cons: If we need to memorize such things then why remove slot elements in the first place?
  • B: Introduce support for single model element to two view elements mapping (the outermost and innermost).
    • What about multiple slots?
    • What about <p><x>XYZ</x>slotFor('children')<x>XYZ</x></p>?
      • There's no innermost element!
    • This should be forbidden: <myElement><$text a=1>X</$text><$text b=1>Y</text></myElement> 😱
      • In this case, multiple slots should be forbidden.
  • C: What if slots weren't removed on conversion?
    • Will it cause the same problems but on the view<->DOM step instead of on the model<->view?
    • What kind of element this would need to be?
    • It makes sense to research this.
  • D: Make slots handle only elements, not text nodes.
    • Reasonable for now.
    • Throw when a text node was encountered by slotFor().

@Reinmar
Copy link
Member Author

Reinmar commented Jan 26, 2022

D: Make slots handle only elements, not text nodes.

  • Reasonable for now.
  • Throw when a text node was encountered by slotFor().

Extracted to: #11163.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 2, 2022

I've recently stumbled upon #11044 where the person wants to turn <p>s into <p><span>...</span></p>. The purpose, as far as I know., is the ability to use overflow-wrap.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Oct 31, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

2 participants