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

The caret divides the marker with inline styles #502

Closed
Mgsy opened this issue Jul 11, 2017 · 4 comments
Closed

The caret divides the marker with inline styles #502

Mgsy opened this issue Jul 11, 2017 · 4 comments
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Mgsy
Copy link
Member

Mgsy commented Jul 11, 2017

Steps to reproduce

  1. Load the Link plugin into the markers manual test.
  2. Go to http://localhost:8125/ckeditor5-engine/tests/manual/markers.html
  3. Type 2 lines of text (or copy/paste it with the Clipboard plugin).
  4. Select whole first line and part o the second line.
  5. Apply bold, italic and link.
  6. Apply the marker.
  7. Click on different spots at the marker.

Current result

The marker divides on every click.

GIF

bug_cke5

Other information

OS: Windows 10, MacOS X
Browser: Chrome, Safari, Firefox, Opera, Edge
Release: 0.10.0

@Mgsy Mgsy added package:engine type:bug This issue reports a buggy (incorrect) behavior. labels Jul 11, 2017
@Reinmar
Copy link
Member

Reinmar commented Jul 11, 2017

May be the same what I described in #500 (comment). A filler splitting the element.

@scofalik
Copy link
Contributor

Caret creating a filler element and splitting words/elements is a known bug.

@Reinmar Reinmar added this to the iteration 11 milestone Jul 11, 2017
@Reinmar
Copy link
Member

Reinmar commented Jul 13, 2017

The easiest way to reproduce it is to load such a content into the markers test:

<div id="editor">
	<p><a href="1"><i><strong>Lorem ipsum dolor sit amet.</strong></i></a></p>
	<p>Foobar.</p>
</div>

(you also need to load the link feature)

And place selection in the marker. The DOM gets messy. The order of attribute elements on the left side of the selection is different than on the right-hand side so they don't get merged nicely and hence, the filler is inserted between them.

The problem here is that we have 3 attributes with the same priority - strong, i and marker and after some operations the order gets messy because we only compare priorties (so the sort isn't stable at all).

I made a quick fix like this and it seems to fix the issue:

--- a/src/view/writer.js
+++ b/src/view/writer.js
@@ -914,6 +914,16 @@ function unwrapChildren( parent, startOffset, endOffset, attribute ) {
     return Range.createFromParentsAndOffsets( parent, startOffset, parent, endOffset );
 }

+function shouldABeOutsideB( a, b ) {
+    if ( a.priority < b.priority ) {
+        return true;
+    } else if ( a.priority > b.priority ) {
+        return false;
+    } else {
+        return a.name < b.name;
+    }
+}
+
 // Wraps children with provided `attribute`. Only children contained in `parent` element between
 // `startOffset` and `endOffset` will be wrapped.
 //
@@ -933,7 +943,7 @@ function wrapChildren( parent, startOffset, endOffset, attribute ) {
         const isUI = child.is( 'uiElement' );

         // Wrap text, empty elements, ui elements or attributes with higher or equal priority.
-        if ( isText || isEmpty || isUI || ( isAttribute && attribute.priority <= child.priority ) ) {
+        if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( attribute, child ) ) ) {
             // Clone attribute.
             const newAttribute = attribute.clone();

However, this won't be stable still because you may have two different attributes which have the same element name. So, we need some way to reliably compare entire attributes. I don't know the rules where elements are considered identical (and are merged) but assuming that classes names and attribute names (but not values) count to being identical, we could compare not element names alone but elementName|attrName1|attrName2|attrName3|className1|className2 strings (attrs and classes need to be sorted). view.Element#getIdentity() could be introduced for that.

@Reinmar Reinmar assigned pomek and szymonkups and unassigned pomek Jul 28, 2017
@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

5 participants