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

Impossible to manage the editable's classes #798

Closed
Reinmar opened this issue Jan 30, 2018 · 6 comments
Closed

Impossible to manage the editable's classes #798

Reinmar opened this issue Jan 30, 2018 · 6 comments
Labels
package:engine package:ui type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 30, 2018

Try this:

editor.ui.view.editable.element.classList.add( 'foo' )

And this class will be immediately removed by the engine on next focus.

Try this:

editor.editing.view.getRoot().addClass( 'foo' )

And after you focus the editable, only this class will be left.

Which means that external app (or even a plugin) has no chance to manage the editable's classes. I even wonder how it worked so far with the initial classes and focus/blur management. I guess we already have some mechanism but it's inefficient.

cc @pjasiun @oleq @oskarwrobel

@Reinmar
Copy link
Member Author

Reinmar commented Jan 30, 2018

Unfortunately, I can't see a workaround for this. The engine overrides everything as soon as you did viewRoot.addClass(). Theoretically, you could add all these missing classes, but then .ck-blurred/ck-focused is still gone.

@Reinmar Reinmar added this to the iteration 14 milestone Jan 30, 2018
@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed package:engine package:ui labels Jan 30, 2018
@Reinmar
Copy link
Member Author

Reinmar commented Jan 30, 2018

OK, one workaround would be to fork the editor package and add this class to the editor UI's template, so it's added together with the other classes (which are maintained). But that requires more work.

@pjasiun
Copy link

pjasiun commented Jan 31, 2018

You can always extend the existence template (even on the editor instance):

editor.ui.view.editable.extendTemplate( {
	attributes: {
		class: 'foo'
	}
} );

But it's a hack.

The problem is that both engine and UI manage this element(actually no, see the last paragraph). And both change the whole attribute value when it changes. It means that when the class attribute changes for the view (see https://github.com/ckeditor/ckeditor5-ui/blob/c5cea07703e1ab63b9cb2f950a253570a2fcc865/src/editableui/editableuiview.js#L38) the whole class attribute is refreshed what means that all custom classes are removed. Fortunately, the engine does nothing with the class attribute and do no break it.

To fix it, UI rendering should change only classes it knows about.

Also, note that because of above there is no problem if you set a custom attribute on that element (even when you do so directly on DOM).

But the way, I tested it, and it looks that root element is never changed by the model. A long time ago we introduced RootAttributeOperation. This is the special type of the operation introduced because attribute operation needs parent element to set range and can not be used on the root element. However, RootAttributeOperation is now never created (I set debugger in the constructor, created the editor, changed focus, changed readOnly mode, edit, it was never called). It makes sense to leave root attributes management to the UI library. It meant that this operation and all related code should be removed.

@oleq
Copy link
Member

oleq commented Feb 1, 2018

To fix it, UI rendering should change only classes it knows about.

That's a good plan, actually. And quite easy to implement IMO.

@Reinmar Reinmar modified the milestones: iteration 14, iteration 15 Feb 6, 2018
@Reinmar Reinmar modified the milestones: iteration 15, backlog, next Mar 30, 2018
@oleq
Copy link
Member

oleq commented Jan 11, 2019

I stumbled on this issue when implementing #479.

I experienced an engine vs. UI collision when implementing root–level placeholders; setting a data-placeholder attribute in a view post–fixer, re–renders the root view and nukes all the attributes set by the UI.

@oleq
Copy link
Member

oleq commented Feb 7, 2019

#479 moves (dynamic) root attribute management from UI to the engine.

It will make adding/removing classes much easier. All it requires is

view.change( writer => writer.addClass( 'new-class', view.document.getRoot() ) );

pjasiun pushed a commit to ckeditor/ckeditor5-ui that referenced this issue Feb 7, 2019
Internal: Moved the `EditableUIView` and  `InlineEditableUIView` attributes management to the engine (see ckeditor/ckeditor5#479). Closes ckeditor/ckeditor5#798.

BREAKING CHANGE: The `EditableUIView#isReadOnly` property has been removed. Use the engine `EditableElement#isReadOnly` instead.

BREAKING CHANGE: The second argument of `EditableUIView.constructor()` and `InlineEditableUIView.constructor()` became the instance of the editing view (previously an optional editable element reference).
@Reinmar Reinmar modified the milestones: next, iteration 22 Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine package:ui type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

3 participants