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

`rawHtml` shouldn't call `prepareDocForEditing` #620

Merged
merged 14 commits into from Nov 25, 2014

Conversation

Projects
None yet
2 participants
@mightyiam
Member

mightyiam commented Nov 22, 2014

No description provided.

@@ -525,7 +525,6 @@ WYMeditor.editor.prototype.rawHtml = function (html) {
if (typeof html === 'string') {
wym.$body().html(html);
wym.update();
wym.prepareDocForEditing();

This comment has been minimized.

@mightyiam

mightyiam Nov 22, 2014

Member

I would expect rawHtml to not call any modification. If required, that modification can be called via prepareDocForEditing().

This makes tests more predictable, as what you put in via rawHtml actually remains in the editor (usually—In Firefox, we turn off designMode before insertion and turn it back on afterwards. That actually causes some alteration, like insertion of br inside empty td) .

prepareDocForEditing() is getting called—and it should—inside html(), right after the call to rawHtml().

@@ -672,7 +672,9 @@ if (!WYMeditor.STRUCTURED_HEADINGS_POLYFILL_REQUIRED ||
'<p>Content</p>' +
'<h2 id="start_selection"><a href="http://google.com">Test</a></h2>' +
'<p>Content</p>' +
'<br class="wym-blocking-element-spacer wym-editor-only" />' +

This comment has been minimized.

@mightyiam

mightyiam Nov 22, 2014

Member

This is a start point HTML for a test.

This test, like most tests, uses rawHtml to insert content for testing on. Since the prepareDocForEditing call was removed from the rawHtml method, this start point HTML must contain these spacers, which are added as part of prepareDocForEditing.

This comment has been minimized.

@winhamwr
@@ -115,11 +115,12 @@ if (jQuery.browser.webkit || jQuery.browser.safari) {
// If there is no element in front of a table in FF or ie, it's not possible
// to put content in front of that table.
test("table has br spacers via .rawHtml()", function () {
test("table has br spacers via .prepareDocForEditing()", function () {

This comment has been minimized.

@mightyiam

mightyiam Nov 22, 2014

Member

This tests the adding of spacers.

Since rawHtml doesn't call for prepareDocForEditing, we must add that call here, after inserting the HTML that doesn't have the spacers.

expectedEndIn,
{assertionString: "Table indented at the end of a list with no " +
"line break"}
);

This comment has been minimized.

@mightyiam

mightyiam Nov 22, 2014

Member

Not sure why this part of this test is here. It seems useless, because we can expect a br to be there.

@@ -484,9 +484,9 @@ test("Body- Direct Paste", function () {
expect(2);
testPaste(
'', // No selector. Just the body
'', // No HTML to start
'<br />', // An empty document to start

This comment has been minimized.

@mightyiam

mightyiam Nov 22, 2014

Member

An empty document is actually br now.

@mightyiam mightyiam changed the title from `rawHtml` shouldn't call `prepareDocForEditing`. to `rawHtml` shouldn't call `prepareDocForEditing` Nov 22, 2014

@mightyiam mightyiam referenced this pull request Nov 22, 2014

Closed

`editor.emptyDocument` #621

@mightyiam

This comment has been minimized.

Member

mightyiam commented Nov 22, 2014

@winhamwr both #621 and #622 are branched off of this one so it would be easier to review them once this is merged. So you're welcome to press the merge button yourself.

@@ -159,6 +159,9 @@ test("Instantiate", function () {
testWymManipulation({
testName: "Empty document is a single `br`.",
startHtml: "",
prepareFunc: function (wymeditor) {
wymeditor.prepareDocForEditing();
},

This comment has been minimized.

@mightyiam

mightyiam Nov 22, 2014

Member

startHtml is set using rawHtml.

prepareDocForEditing is not called from rawHtml any more, so if we expect the empty document to have a single br, we must call prepareDocForEditing using prepareFunc, which is called in testWymManipulation after rawHtml.

@mightyiam

This comment has been minimized.

Member

mightyiam commented Nov 22, 2014

@winhamwr please review #623 before this.

@winhamwr

This comment has been minimized.

Member

winhamwr commented Nov 25, 2014

Looks good! I like moving in the more explicit test direction.

winhamwr added a commit that referenced this pull request Nov 25, 2014

Merge pull request #620 from wymeditor/noPrepareDocInRawHtml
`rawHtml` shouldn't call `prepareDocForEditing`

@winhamwr winhamwr merged commit 217bb85 into master Nov 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@winhamwr winhamwr deleted the noPrepareDocInRawHtml branch Nov 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment