Skip to content
This repository has been archived by the owner on Apr 2, 2021. It is now read-only.

Image selection #641

Merged
merged 46 commits into from Dec 30, 2014
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
e34c686
Select images on mousedown using Rangy
mightyiam Dec 16, 2014
7780bdc
Add test for unlinking image.
mightyiam Dec 16, 2014
2da645b
TridentPre7's _keyup: remove unused assignment
mightyiam Dec 16, 2014
c523284
Image selection test.
mightyiam Dec 16, 2014
0dcea99
Image selection test: assert only one node selected
mightyiam Dec 16, 2014
4a47c58
Use `manipulationFunc` in image selection test
mightyiam Dec 16, 2014
75ce430
Merge branch 'classToggling' into selectImagesWithRangy
mightyiam Dec 17, 2014
5f6e1fb
Make JSHint happy.
mightyiam Dec 17, 2014
1e7dcd8
Merge branch 'master' into selectImagesWithRangy
mightyiam Dec 17, 2014
bf090a9
`editor.getSelectedImage` including api doc and tests
mightyiam Dec 18, 2014
8b918a2
Fix toggleClass
mightyiam Dec 18, 2014
5458c49
Use IMG_SRC in a few places in tests
mightyiam Dec 18, 2014
580db7b
_selectSingleNode
mightyiam Dec 18, 2014
92a1af5
Merge branch 'image-dialog-focus-before-insert' into selectImagesWith…
mightyiam Dec 19, 2014
eab4b0e
Merge branch 'qunit-hide-passed' into selectImagesWithRangy
mightyiam Dec 20, 2014
cf02d83
_selectSingleNode: return statement is supposed to be throw
mightyiam Dec 20, 2014
9a5525f
Fix image dragging and dropping
mightyiam Dec 20, 2014
545c3fb
Docstring for _selectSingleNode
mightyiam Dec 20, 2014
d9a4160
Tests with image selection: skip in PhantomJS
mightyiam Dec 20, 2014
97d1753
Make JSHint happy
mightyiam Dec 20, 2014
d7e9f1b
Some code comments for #641
mightyiam Dec 20, 2014
bb09bdb
Fix code comments for #641
mightyiam Dec 20, 2014
b936084
Changelog for #641.
mightyiam Dec 20, 2014
d581582
Fix Trident 7's docEventQuirks
mightyiam Dec 22, 2014
73a9760
Fix some image related tests
mightyiam Dec 22, 2014
149f898
Remove old image selection test
mightyiam Dec 22, 2014
4388986
Merge branch 'master' into selectImagesWithRangy
mightyiam Dec 22, 2014
1f56d9c
Don't use sinon fake timers in QUnit.
mightyiam Dec 22, 2014
be5fcfd
manipulationTestHelper: async feature
mightyiam Dec 22, 2014
b7d405b
Async image selection test for pre-7 Trident
mightyiam Dec 22, 2014
82715fe
Fix some image related tests for pre-7 Trident
mightyiam Dec 22, 2014
686aac6
Pre-7 trident's image selection test: some comments
mightyiam Dec 22, 2014
37d06d4
Fix some code comment
mightyiam Dec 22, 2014
ec95b71
Test for image selection via dragend in IE
mightyiam Dec 23, 2014
0560db2
Deselect before image selection tests.
mightyiam Dec 23, 2014
6e4b123
Fix an image selection test had extra async: true
mightyiam Dec 23, 2014
306fad5
Added missing `async: true` to test helper example
mightyiam Dec 24, 2014
cb38bce
manipulationTestHelper: more readable async example
mightyiam Dec 24, 2014
99cbd40
manipulationTestHelper: semantic change in returning asyncResumeFunc
mightyiam Dec 24, 2014
e1aed3a
manipulationTestHelper: fail tests that specify async but do not call
mightyiam Dec 24, 2014
6c88b05
Image selection test: more readable.
mightyiam Dec 24, 2014
f25a80a
Unit tests: explain why we do not use fake timers.
mightyiam Dec 24, 2014
c6d0553
Comment on pre-7 Triden'ts `mouseup` on an image.
mightyiam Dec 24, 2014
3c1604f
Better comment for pre-7 Trident image selection
mightyiam Dec 24, 2014
f1921f8
manipulationTestHelper: add comma in docstring
mightyiam Dec 30, 2014
7d90d19
manipulationTestHelper: return asyncResumeFunc only if async
mightyiam Dec 30, 2014
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 24 additions & 35 deletions src/test/unit/specific_feature_tests/images.js
Expand Up @@ -4,8 +4,7 @@
prepareUnitTestModule,
test,
expect,
equal,
deepEqual
strictEqual
*/
"use strict";

Expand Down Expand Up @@ -45,39 +44,29 @@ test("Inserts image into the body", function () {
});
});

test("._selectedImage is saved on mousedown", function () {
var initHtml = [""
, '<p id="noimage">Images? We dont need no silly images</p>'
, '<p>'
, '<img id="google" src="http://example.com/example.jpg" />'
, '</p>'
].join(''),
wymeditor = jQuery.wymeditors(0),
$body,
$noimage,
$google;
module("images-selection", {setup: prepareUnitTestModule});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A module for testing that images are being selected on mousedown.


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not how this PR rolls.

expect(3);
test("Image is selected on mousedown", function () {
var noChangeHtml = [""
, "<p>"
, "A "
, "<img alt=\"Pen\" src=\"http://goo.gl/N9nqUc\" />"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised you're not using your IMG_SRC global.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between all of these different branches... It is like juggling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed.

, "</p>"
].join("");

wymeditor.rawHtml(initHtml);
$body = wymeditor.$body();

// Editor starts with no selected image. Use equal instead of strictEqual
// because wymeditor._selectedImage intermittently changes between being
// undefined and null, but either value should be acceptable for this test.
equal(
wymeditor._selectedImage,
null
);

// Clicking on a non-image doesn't change that
$noimage = $body.find('#noimage');
$noimage.mousedown();
deepEqual(wymeditor._selectedImage, null);


// Clicking an image does update the selected image
$google = $body.find('#google');
$google.mousedown();
deepEqual(wymeditor._selectedImage, $google[0]);
manipulationTestHelper({
startHtml: noChangeHtml,
prepareFunc: function (wymeditor) {
wymeditor.$body().find("img").mousedown();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
expectedResultHtml: noChangeHtml,
additionalAssertionsFunc: function (wymeditor) {
expect(expect() + 1);
strictEqual(
wymeditor._getSelectedNodes()[0].tagName.toLowerCase(),
"img",
"Image is the only selected node"
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One assertion tests that only one node is selected.
The other assertion tests that it is an image.

}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
72 changes: 72 additions & 0 deletions src/test/unit/specific_feature_tests/links.js
Expand Up @@ -196,3 +196,75 @@ test("IE unlinks when collapsed selection inside link", function () {
}
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests for linking/unlinking images.

test("Links selected unlinked image", function () {
manipulationTestHelper({
startHtml: "<p>A <img alt=\"pen\" src=\"http://goo.gl/N9nqUc\" /></p>",
prepareFunc: function (wymeditor) {
wymeditor.$body().find("img").mousedown();
},
manipulationFunc: function (wymeditor) {
wymeditor.link({href: "http://example.com/"});
},
expectedResultHtml: [""
, "<p>"
, "A "
, "<a href=\"http://example.com/\">"
, "<img alt=\"pen\" src=\"http://goo.gl/N9nqUc\" />"
, "</a>"
, "</p>"
].join("")
});
});

test("Modifies linked image", function () {
manipulationTestHelper({
startHtml: [""
, "<p>"
, "<a href=\"http://example.com/\">"
, "A "
, "<img alt=\"pen\" src=\"http://goo.gl/N9nqUc\" />"
, "</a>"
, "</p>"
].join(""),
prepareFunc: function (wymeditor) {
wymeditor.$body().find("img").mousedown();
},
manipulationFunc: function (wymeditor) {
wymeditor.link({href: "http://example.com/foo"});
},
expectedResultHtml: [""
, "<p>"
, "<a href=\"http://example.com/foo\">"
, "A "
, "<img alt=\"pen\" src=\"http://goo.gl/N9nqUc\" />"
, "</a>"
, "</p>"
].join("")
});
});

test("Unlinks linked image", function () {
manipulationTestHelper({
startHtml: [""
, "<p>"
, "A "
, "<a href=\"http://example.com/\">"
, "<img alt=\"pen\" src=\"http://goo.gl/N9nqUc\" />"
, "</a>"
, "</p>"
].join(""),
prepareFunc: function (wymeditor) {
wymeditor.$body().find("img").mousedown();
},
manipulationFunc: function (wymeditor) {
wymeditor.exec(WYMeditor.EXEC_COMMANDS.UNLINK);
},
expectedResultHtml: [""
, "<p>"
, "A "
, "<img alt=\"pen\" src=\"http://goo.gl/N9nqUc\" />"
, "</p>"
].join("")
});
});
18 changes: 0 additions & 18 deletions src/wymeditor/core.js
Expand Up @@ -1181,11 +1181,6 @@ WYMeditor._initDialog = function (index) {
selected.tagName.toLowerCase !== WYMeditor.A) {
selected = jQuery(selected).parentsOrSelf(WYMeditor.A);
}

// fix MSIE selection if link image has been clicked
if (!selected && wym._selectedImage) {
selected = jQuery(wym._selectedImage).parentsOrSelf(WYMeditor.A);
}
}

// pre-init functions
Expand All @@ -1202,19 +1197,6 @@ WYMeditor._initDialog = function (index) {
jQuery(wym._options.altSelector).val(jQuery(selected).attr(WYMeditor.ALT));
}

// auto populate image fields if selected image
if (wym._selectedImage) {
jQuery(
wym._options.dialogImageSelector + " " + wym._options.srcSelector
).val(jQuery(wym._selectedImage).attr(WYMeditor.SRC));
jQuery(
wym._options.dialogImageSelector + " " + wym._options.titleSelector
).val(jQuery(wym._selectedImage).attr(WYMeditor.TITLE));
jQuery(
wym._options.dialogImageSelector + " " + wym._options.altSelector
).val(jQuery(wym._selectedImage).attr(WYMeditor.ALT));
}

jQuery(wym._options.dialogLinkSelector + " " + wym._options.submitSelector)
.submit(function () {
wym.link({
Expand Down
19 changes: 9 additions & 10 deletions src/wymeditor/editor/base.js
Expand Up @@ -658,7 +658,7 @@ WYMeditor.editor.prototype.exec = function (cmd) {

case WYMeditor.EXEC_COMMANDS.CREATE_LINK:
container = wym.getRootContainer();
if (container || wym._selectedImage) {
if (container) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic of when dialogs pop up or not is in the works in #639 anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it is pretty messed up as it is and without tests. So I don't worry about it because that is the next PR, it seems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

wym.dialog(WYMeditor.DIALOG_LINK);
}
break;
Expand Down Expand Up @@ -1226,12 +1226,7 @@ WYMeditor.editor.prototype.keyCanCreateBlockElement = function (keyCode) {
*/
WYMeditor.editor.prototype.toggleClass = function (sClass, jqexpr) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modify toggleClass to work with new image selection behavior. See tests in this PR.

var wym = this,
$container;
if (wym._selectedImage) {
$container = jQuery(wym._selectedImage);
} else {
$container = jQuery(wym.selectedContainer());
}
$container = $container.parentsOrSelf(jqexpr);
$container.toggleClass(sClass);

Expand Down Expand Up @@ -3955,11 +3950,15 @@ WYMeditor.editor.prototype._handlePasteEvent = function () {
};

WYMeditor.editor.prototype._mousedown = function (evt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crux of this PR. We don't store _selectedImage any more.

Instead, upon mousedown on an image, we set selection to encompass it, exclusively.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. I hadn't thought of that. It seems to perform well in old IE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Seems so, yes.

var wym = this;
// Store the selected image if we clicked an <img> tag
wym._selectedImage = null;
var wym = this,
selection,
imageRange;

if (evt.target.tagName.toLowerCase() === WYMeditor.IMG) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using mousedown any more.

wym._selectedImage = evt.target;
selection = wym.selection();
imageRange = rangy.createRangyRange();
imageRange.selectNode(evt.target);
selection.setSingleRange(imageRange);
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crux of this PR.

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

No more unsetting & setting wym._selectedImage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


Expand Down
1 change: 0 additions & 1 deletion src/wymeditor/editor/gecko.js
Expand Up @@ -64,7 +64,6 @@ WYMeditor.WymClassGecko.prototype._keyup = function (evt) {
wym.documentStructureManager.structureRules.notValidRootContainers;
defaultRootContainer =
wym.documentStructureManager.structureRules.defaultRootContainer;
wym._selectedImage = null;
container = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gecko's keyup handler. Unsets _selectedImage. No need for this action any more.


// If the inputted key cannont create a block element and is not a command,
Expand Down
2 changes: 0 additions & 2 deletions src/wymeditor/editor/trident-pre-7.js
Expand Up @@ -163,7 +163,6 @@ WYMeditor.WymClassTridentPre7.prototype.unwrap = function () {

WYMeditor.WymClassTridentPre7.prototype._keyup = function (evt) {
var wym = this,
doc = wym._doc,
container,
defaultRootContainer,
notValidRootContainers,
Expand All @@ -176,7 +175,6 @@ WYMeditor.WymClassTridentPre7.prototype._keyup = function (evt) {
wym.documentStructureManager.structureRules.notValidRootContainers;
defaultRootContainer =
wym.documentStructureManager.structureRules.defaultRootContainer;
doc._selectedImage = null;

// If the pressed key can't create a block element and is not a command,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like in Gecko.

// check to make sure the selection is properly wrapped in a container
Expand Down
1 change: 0 additions & 1 deletion src/wymeditor/editor/webkit.js
Expand Up @@ -161,7 +161,6 @@ WYMeditor.WymClassWebKit.prototype._keyup = function (evt) {
wym.documentStructureManager.structureRules.notValidRootContainers;
defaultRootContainer =
wym.documentStructureManager.structureRules.defaultRootContainer;
wym._selectedImage = null;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like in Gecko.

// Fix to allow shift + return to insert a line break in older safari
if (jQuery.browser.version < 534.1) {
Expand Down