Поправил определение границ ссылки #1

Epsilonnnn · 2018-06-07T14:56:05Z

Убрал функции, приводящии к разбиению текстовых нод

Epsilonnnn · 2018-06-07T15:08:11Z

Проблема 1: Операции CKEDITOR.dom.range.checkStartOfBlock и CKEDITOR.dom.range.checkEndOfBlock меняют текстовые ноды и могут разделять одну тектовую ноду на 2 части, из-за этого при клике в любую тектовую ноду она разделяется на 2.

Проблема 2: Операции CKEDITOR.dom.range.checkStartOfBlock и CKEDITOR.dom.range.checkEndOfBlock не определяют корректно конец и начало ссылки, а определяют на самом деле конец и начало первого дива, в который обёрнута ссылка.

Решение: Заменил CKEDITOR.dom.range.checkStartOfBlock и CKEDITOR.dom.range.checkEndOfBlock на более точную функцию определения границ ссылки, которая не модифицирует DOM.

chestozo

вопросики

chestozo · 2018-06-07T16:01:01Z

plugin.js

+        abbr: 1, acronym: 1, b: 1, bdo: 1, big: 1, cite: 1, code: 1, del: 1,
+        dfn: 1, em: 1, font: 1, i: 1, ins: 1, label: 1, kbd: 1, q: 1, samp: 1, small: 1, span: 1, strike: 1,
+        strong: 1, sub: 1, sup: 1, tt: 1, u: 1, 'var': 1
+    };


ох )
а мы откуда-то позаимствовали этот список, надеюсь? )
может его его в конфиг можно положить?

Реализация функции linkStartEndEvaluator была inspired by вот этой функцией https://github.com/yandex-ui/ckeditor-dev/blob/master/core/dom/range.js#L525 , которая используется как evaluator для функций CKEDITOR.dom.range.checkStartOfBlock и CKEDITOR.dom.range.checkEndOfBlock.

Список взят отсюда - https://github.com/yandex-ui/ckeditor-dev/blob/master/core/dom/range.js#L517

может его его в конфиг можно положить?

в какой? зачем?

ну это же набор инлайн тэгов, похоже на статическую информацию
чтобы можно было реиспользовать

Ты говоришь про проектный конфиг? Тоесть вынести этот список из плагина?
я не думаю, что это нужно будет переиспользовать, так же тут теги не всех inline элементов
Кажется, имеет смысл выносить, только если это реально где-то понадобится, тоесть, если где-то ещё придётся писать свою функцию определения начала и/или конца блока

chestozo · 2018-06-07T16:07:34Z

plugin.js

+            }
+
+            var isFalseTextNode = node.type == ckeditorNamespace.NODE_TEXT &&
+                !node.hasAscendant('pre') && !ckeditorNamespace.tools.trim(node.getText()).length;


почему именно pre?

взято отсюда - https://github.com/yandex-ui/ckeditor-dev/blob/master/core/dom/range.js#L543
на сколько я понимаю pre должно передавать юзер-инпут без изменений даже если это пробелы, поэтому с ним считаются

кажется, замечание про === резонно, можно поправить

chestozo · 2018-06-07T16:09:06Z

plugin.js

+            var isFalseTextNode = node.type == ckeditorNamespace.NODE_TEXT &&
+                !node.hasAscendant('pre') && !ckeditorNamespace.tools.trim(node.getText()).length;
+
+            var isFalseInlineBlock = node.type == ckeditorNamespace.NODE_ELEMENT && node.is(inlineChildReqElements);


не хотим оптимизнуть и не вычислять node.is(), пока это не требуется?

поправил

chestozo · 2018-06-07T16:10:11Z

plugin.js

@@ -14,26 +49,29 @@
            this.editable().on('click', this.plugins.openlink.onClick, this, null, 0);
        },

+        _isInLinkBoundary: function(range, link) {
+            var prevNode = range.getPreviousNode(linkStartEndEvaluator(CKEDITOR), linkStartEndGuard(link), link);
+            var nextNode = range.getNextNode(linkStartEndEvaluator(CKEDITOR), linkStartEndGuard(link), link);


аналогично, можно оптимизнуть и не вычислять nextNode, если это не нужно

поправил

chestozo · 2018-06-07T16:10:47Z

plugin.js

                return;
            }

            var nativeEvent = event.data.$;
            var path = new CKEDITOR.dom.elementPath(new CKEDITOR.dom.element(nativeEvent.target), range.root);

            var link = path.contains('a', true);
-            if (!link) {
+            if (!link || this.plugins.openlink._isInLinkBoundary(range, link)) {


вызов приватного метода? )

метод принадлежит плагину, внутри которого вызывается

chestozo · 2018-06-09T08:30:57Z

plugin.js

-                !node.hasAscendant('pre') && !ckeditorNamespace.tools.trim(node.getText()).length;
-
-            var isFalseInlineBlock = node.type == ckeditorNamespace.NODE_ELEMENT && node.is(inlineChildReqElements);
+            // is false inline block


is not inline block
а вообще можно было бы сделать переменную и тогда коммент не нужен
!isInlineBlock

не все inline блоки, нет например img

разве img не inline?

да, inline, но тут его специально нет - https://github.com/yandex-ui/ckeditor-dev/blob/master/core/dom/range.js#L549

chestozo · 2018-06-09T08:31:39Z

plugin.js


-            if (isFalseTextNode || isFalseInlineBlock) {
+            // is false text node


вот тут совсем не ясно, о чём коммент

наверное стоит написать is empty text node

chestozo · 2018-06-09T08:32:55Z

plugin.js

+        abbr: 1, acronym: 1, b: 1, bdo: 1, big: 1, cite: 1, code: 1, del: 1,
+        dfn: 1, em: 1, font: 1, i: 1, ins: 1, label: 1, kbd: 1, q: 1, samp: 1, small: 1, span: 1, strike: 1,
+        strong: 1, sub: 1, sup: 1, tt: 1, u: 1, 'var': 1
+    };


может его его в конфиг можно положить?

chestozo · 2018-06-09T08:34:41Z

plugin.js

+            }
+
+            // is false inline block
+            if (node.type == ckeditorNamespace.NODE_ELEMENT && node.is(inlineChildReqElements)) {


проверка "это inline node-а"
а коммент как будто бы про другое ..

Имхо коммент выше не нужен, он почти полностью дублирует код.
Может стоит переименовать inlineChildReqElements во что-то более говорящее (не очень ясно, что значит req).

добавил коммент про inlineChildReqElements - https://github.com/yandex-ui/ckeditor-openlink/pull/1/files#diff-18e559ebd3cc204a52bc1dc3db2adf8bR6

req - require

chestozo · 2018-06-13T11:11:41Z

plugin.js

+            }
+
+            // is false inline block
+            if (node.type == ckeditorNamespace.NODE_ELEMENT && node.is(inlineChildReqElements)) {


Имхо коммент выше не нужен, он почти полностью дублирует код.
Может стоит переименовать inlineChildReqElements во что-то более говорящее (не очень ясно, что значит req).

chestozo · 2018-06-13T11:16:39Z

plugin.js

+            if (
+                node.type == ckeditorNamespace.NODE_TEXT && !node.hasAscendant('pre') &&
+                !ckeditorNamespace.tools.trim(node.getText()).length
+            ) {


имхо тут стоит

проверку node.hasAscendant() делать в самую последнюю очередь (как самую дорогую)

коммент, имхо, стоит переписать на что-то более human readable, сейчас я не очень понимаю вообще что делает linkStartEndEvaluator поэтому не могу предложить чего-то конкретного )

Поправил

linkStartEndEvaluator - это https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_dom_walker.html#property-evaluator

я всё ещё не доволен комментом
он нам говорит "это пустая текстовая нода"
а проверка на непустоту !ckeditorNamespace.tools.trim(node.getText()).length

эм, если ckeditorNamespace.tools.trim(node.getText()).length = 0, то !ckeditorNamespace.tools.trim(node.getText()).length = true ))

Убрал функции, приводящии к разбиению текстовых нод

chestozo reviewed Jun 7, 2018

View reviewed changes

chestozo reviewed Jun 9, 2018

View reviewed changes

chestozo reviewed Jun 13, 2018

View reviewed changes

chestozo approved these changes Jun 14, 2018

View reviewed changes

Поправил определение границ ссылки

7368d2a

Убрал функции, приводящии к разбиению текстовых нод

Epsilonnnn force-pushed the fix-detect-link-boundaries branch from 2441dcd to 7368d2a Compare June 15, 2018 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Поправил определение границ ссылки #1

Поправил определение границ ссылки #1

Epsilonnnn commented Jun 7, 2018

Epsilonnnn commented Jun 7, 2018 •

edited

Loading

chestozo left a comment

chestozo Jun 7, 2018

Epsilonnnn Jun 8, 2018

chestozo Jun 9, 2018

Epsilonnnn Jun 9, 2018

chestozo Jun 9, 2018

Epsilonnnn Jun 13, 2018 •

edited

Loading

chestozo Jun 7, 2018

chestozo Jun 7, 2018

Epsilonnnn Jun 8, 2018

chestozo Jun 7, 2018

Epsilonnnn Jun 8, 2018

chestozo Jun 7, 2018

Epsilonnnn Jun 8, 2018

chestozo Jun 7, 2018

Epsilonnnn Jun 8, 2018 •

edited

Loading

chestozo Jun 9, 2018

Epsilonnnn Jun 9, 2018

chestozo Jun 9, 2018

Epsilonnnn Jun 13, 2018

chestozo Jun 9, 2018

Epsilonnnn Jun 13, 2018 •

edited

Loading

chestozo Jun 9, 2018

chestozo Jun 9, 2018

chestozo Jun 13, 2018

Epsilonnnn Jun 14, 2018

chestozo Jun 13, 2018

chestozo Jun 13, 2018

Epsilonnnn Jun 14, 2018

chestozo Jun 14, 2018

Epsilonnnn Jun 14, 2018 •

edited

Loading


		if (isFalseTextNode \|\| isFalseInlineBlock) {
		// is false text node

Поправил определение границ ссылки #1

Are you sure you want to change the base?

Поправил определение границ ссылки #1

Conversation

Epsilonnnn commented Jun 7, 2018

Epsilonnnn commented Jun 7, 2018 • edited Loading

chestozo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Epsilonnnn Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Epsilonnnn Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Epsilonnnn Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Epsilonnnn Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Epsilonnnn commented Jun 7, 2018 •

edited

Loading

Epsilonnnn Jun 13, 2018 •

edited

Loading

Epsilonnnn Jun 8, 2018 •

edited

Loading

Epsilonnnn Jun 13, 2018 •

edited

Loading

Epsilonnnn Jun 14, 2018 •

edited

Loading