Image selection #641

mightyiam · 2014-12-16T14:33:57Z

Removes _selectedImage.

mightyiam · 2014-12-16T14:38:33Z

src/wymeditor/editor/base.js

+        selection = wym.selection();
+        imageRange = rangy.createRangyRange();
+        imageRange.selectNode(evt.target);
+        selection.setSingleRange(imageRange);
    }
 };


The crux of this PR.

Simply selects, using Rangy, the image that was clicked upon.

No more unsetting & setting wym._selectedImage.

This is a simple mechanism for selecting images. It could be more pretty, but it is simple and works in all browsers.

The previous mechanism didn't work well enough, because selection wasn't set to the image and there was otherwise no indication that the image was indeed selected. Users don't look at the debugger and put a watch on jQuery.wymeditors(0)._selectedImage, ya know?

mightyiam · 2014-12-16T16:03:49Z

src/wymeditor/editor/base.js

        $container = jQuery(wym.selectedContainer());
-    }


This means that toggling the class of an img isn't possible.

Does it seem important? Regression?

I notice that we don't have tests for toggleClass.

mightyiam · 2014-12-16T16:29:50Z

src/test/unit/specific_feature_tests/images.js

+                "Image is the only selected node"
+            );
+        }
+    });


A test for image selection, instead of the test for _selectedImage.

mightyiam · 2014-12-16T16:36:58Z

src/test/unit/specific_feature_tests/images.js

+    manipulationTestHelper({
+        startHtml: noChangeHtml,
+        manipulationFunc: function (wymeditor) {
+            wymeditor.$body().find("img").mousedown();


The manipulation in this test is a jQuery.fn.mousedown() on the image.

Conflicts: src/test/unit/specific_feature_tests/images.js src/wymeditor/editor/base.js

Conflicts: src/wymeditor/editor/base.js

mightyiam · 2014-12-18T12:43:13Z

src/wymeditor/editor/base.js

+    If selection encompasses exactly a single image, returns that image.
+    Otherwise returns `false`.
+*/
+WYMeditor.editor.prototype.getSelectedImage = function () {


winhamwr · 2014-12-23T19:39:52Z

src/test/unit/specific_feature_tests/images.js

+    wymeditor = jQuery.wymeditors(0);
+
+    // Stop QUnit from running the next test
+    stop();


Rather than the deprecated stop() and start(), why not use assert.async and done()? This line would be var done = assert.async(); and the start() line would be done() I think.

That would require an upgrade of QUnit, I believe.

mightyiam · 2014-12-23T20:05:28Z

src/test/unit/specific_feature_tests/images.js

+module("images-selection", {setup: prepareUnitTestModule});
+
+test("Image is selected via `mouseup` in non pre-7 Trident", function () {
+    manipulationTestHelper({


@winhamwr you're right. The async: true here was just left by me here. This serves to show that turning that switch on by mistake could result in a false passing test.

Hrm. What can we do about that? Seems like expect() is the thing designed to avoid those kind of false positives.

See fix in manipulationTestHelper.

mightyiam · 2014-12-24T14:31:37Z

src/test/unit/manipulation-test-helper.js

+ *         assertions. This can only be used when a single manipulation cause
+ *         is provided (for example, only `manipulationFunc`).
+ *         For example:
+ *         ```


@winhamwr replaced resume with resumeManipulationTestHelper in the example below. Not runAssertions because it is more than assertions—it is also undo/redo operations. And also, by the time it returns, some assertions were already executed.

mightyiam · 2014-12-24T14:36:04Z

src/test/unit/manipulation-test-helper.js

    }

+    return asyncResumeFunc;


@winhamwr you're right. This is much cleaner. I thought about writing it like this, at first, but setting it inside a while loop statement didn't seem reasonable, until now that I realized that in the case when a.async === true, that loop will execute once.

While this code functions correctly, it reads poorly. There's no reason to return here if we're not using async (even though the caller should just ignore the return result). Let's put it inside an if (a.async === true) block.

mightyiam · 2014-12-24T14:56:59Z

src/test/unit/manipulation-test-helper.js

-            return;
+        // Expectancy incremented here in order to fail tests that specify
+        // `async` but do not call the `asyncResumeFunc`.
+        expect(expect() + 1);


Here's the fix for the case where a test specifies async: true but does not call the returned function.

Clever fix!

mightyiam · 2014-12-24T15:25:01Z

src/test/unit/specific_feature_tests/images.js

+        // Unwrap
+        wymeditor._selectSingleNode = _selectSingleNode;
+        // Resume `manipulationTestHelper`
+        resumeManipulationTestHelper();


The resume() function when called in your wrapped _selectSingleNode isn't yet set to the return value of manipulationTestHelper because that function hasn't yet returned. Having a hard time making heads or tails of this.

The scope of this call is a function definition, an assignment to wymeditor._selectSingleNode. wymeditor._selectSingleNode will be called later, triggered by pre–7 Trident's mouseup.

It is ok that resumeManipulationTestHelper(), which is not defined (undefined) at the time of this function's definition, is included inside, because it is currently set to undefined. If you try to call it now, it would be like doing undefined() and that will probably yield something like "undefined does not have property call" or something like that.

Since the call to wymeditor._selectSingleNode is inside a setTimeout (with zero time) it will run only (and probably immediately) after resumeManipulationTestHelper had returned. Thus, in the scope of the anonymous function of this test, by that time, when it does run, in this scope resumeManipulationTestHelper would have already been the return value of manipulationTestHelper.

I hope this is clear enough :)

@winhamwr ^

mightyiam · 2014-12-24T15:30:43Z

src/test/unit/test.js

+// we test things that have to do with the browser's DOM API. Removing this
+// or setting it to `true` will almost certainly cause all tests of
+// asynchronous things to fail or the test runner to stop and not continue.
+sinon.config.useFakeTimers = false;


@winhamwr this explains it, right?

mightyiam · 2014-12-24T15:42:24Z

src/wymeditor/editor/trident-pre-7.js

+    // undesired resize handles.
+    // Wrapping the selection call in an immediate `setTimeout` makes
+    // reasonably certain that the "control selection" will be very quickly
+    // replaced by our desired, regular selection.


@winhamwr at your request, a comment here.

mightyiam · 2014-12-24T15:59:24Z

src/wymeditor/editor/trident-pre-7.js

+    // https://github.com/wymeditor/wymeditor/pull/641
+    window.setTimeout(function () {
+        wym._selectSingleNode(evt.target);
+    }, 0);


@winhamwr regarding zero–time setTimeout, I highly recommend this talk.

In short, the browser holds a sequential stack of calls and it runs them ASAP, sequentially. When one call ends, the next can start.

setTimeout adds a call on top of that stack, after the specified time. A zero time means that it gets throw on that stack immediately.

In our case, when an image gets a mouse event, some various named and unnamed events occur. Amongst them is mouseup, which we have a handler for. In pre–7 Trident, that handler throws a selection action on top of the call queue stack, with setTimeout with zero time. After that, the controlselect is triggered and the resize handles appear. After that, synchronous code had finished running and the browser's event loop executes the next call in the queue stack, which is almost certainly the selection call which we had previously thrown there from mouseup.

Man, that sounds like bad design. But it seems solid.

winhamwr · 2014-12-29T17:00:24Z

Other than the two minor tweaks I suggested, this looks ready to merge!

mightyiam · 2014-12-30T14:18:09Z

src/test/unit/manipulation-test-helper.js

@@ -64,6 +64,33 @@
 *         ```
 *         This example uses the `jquery.browser` plugin
 *         https://github.com/gabceb/jquery-browser-plugin
+ *     `async`
+ *         Optional; If this is `true` then after the manipulation is
+ *         performed, assertions regarding the results are not synchronously


Added comma here.

mightyiam · 2014-12-30T14:23:20Z

src/test/unit/manipulation-test-helper.js

+    }
+
+    if (a.async === true) {
+        return asyncResumeFunc;
    }


@winhamwr you asked for this return to be conditional. Here it is. Thanks.

Image selection

mightyiam · 2014-12-30T14:30:16Z

Great! Thank you!

Select images on mousedown using Rangy

e34c686

Removes _selectedImage.

mightyiam reviewed Dec 16, 2014
View reviewed changes

Add test for unlinking image.

7780bdc

mightyiam reviewed Dec 16, 2014
View reviewed changes

mightyiam added 2 commits December 16, 2014 18:28

TridentPre7's _keyup: remove unused assignment

2da645b

Image selection test.

c523284

mightyiam reviewed Dec 16, 2014
View reviewed changes

mightyiam added 2 commits December 16, 2014 18:34

Image selection test: assert only one node selected

0dcea99

Use manipulationFunc in image selection test

4a47c58

mightyiam reviewed Dec 16, 2014
View reviewed changes

mightyiam added 5 commits December 17, 2014 21:06

Merge branch 'classToggling' into selectImagesWithRangy

75ce430

Conflicts: src/test/unit/specific_feature_tests/images.js src/wymeditor/editor/base.js

Make JSHint happy.

5f6e1fb

Merge branch 'master' into selectImagesWithRangy

1e7dcd8

Conflicts: src/wymeditor/editor/base.js

editor.getSelectedImage including api doc and tests

bf090a9

Fix toggleClass

8b918a2

mightyiam reviewed Dec 18, 2014
View reviewed changes

winhamwr reviewed Dec 23, 2014
View reviewed changes

Fix an image selection test had extra async: true

6e4b123

mightyiam reviewed Dec 23, 2014
View reviewed changes

mightyiam added 2 commits December 24, 2014 16:20

Added missing async: true to test helper example

306fad5

manipulationTestHelper: more readable async example

cb38bce