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

Table insertion and manipulation now works in lists #404

Merged
merged 28 commits into from Jun 19, 2013

Conversation

NickMcL
Copy link
Contributor

@NickMcL NickMcL commented Jun 12, 2013

This pull request addresses issue #403 and partially addresses issue #139 by allowing tables to be inserted and manipulated within lists.

Previously, table insertion did not work at all for lists, and if you tried to insert a table with a selection in a list, the table would be inserted at the end of the document instead. This pull request fixes this issue by now allowing tables to be inserted at any point within a list or sublist. In addition, once a table is inserted into a list, it will properly indent and outdent with the list when the indent and outdent tools are used. This will work even if multiple tables are inserted at the same or at different points in one list.

A reoccurring issue I found with tables in a list was that when a table was inserted at the end of a list or sublist, the user could not click under the table to continue to add points to the list after the list item containing the table. To fix this, I put in checks when a table is inserted, indented, or outdented in a list to ensure that space exists under the table that the user can click in so that the list can always be continued.

I also added ten tests to the unit tests for table insertion, indention, and outdention in a list, including tests for the issue of continuing a list after a table that I mentioned in the paragraph above. I have confirmed that these tests all pass in FF 21, Chrome 27, and IE 6-10.

.removeAttr('_moz_editor_bogus_node')
.removeAttr('_moz_dirty');
});
$body.find('caption').each(function () {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? So we're forcing caption elements to the start of the table? Is this not normally the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In IE8 and IE9, the tests were failing because the caption was added after the table body. I added this to ensure the caption was always at the start.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Could you wrap this in a browser detection check and add a comment to that effect, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added browser detection and a comment for it.

@NickMcL
Copy link
Contributor Author

NickMcL commented Jun 12, 2013

I think I addressed all of your problems with the tests. I'll start working on fixing the issues you pointed out with the editor code now.

@NickMcL
Copy link
Contributor Author

NickMcL commented Jun 13, 2013

I reimplemented the table spacing to use spaceBlockingElements. I confirmed that the tests for it are now all passing in FF 21, Chrome 27, and IE 6-10.

return normalizeHtml($body.get(0).firstChild);
}

function setupTable(wymeditor, html, selection, rows, cols, caption) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment here explaining that this creates a table whose cells have the X_Y index has the id and as the text. Should also have a mini example in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@NickMcL
Copy link
Contributor Author

NickMcL commented Jun 14, 2013

I rewrote a decent amount of how table insertion and spacing is handled with these most recent changes. From my testing, the insertion and spacing should now be more consistent and behave more predictably. I also made a greater effort to try to follow the existing model for spacing blocking elements, and I tried to make it easy to support adding different blocking elements besides tables inline with list items in the future in case it would ever be desired.

!(jQuery.browser.msie &&
attr.nodeName === '_wym_visited' ||
attr.nodeName === 'sizcache' ||
attr.nodeName === 'sizset')
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 sizcache and sizset attributes specified here were only showing up in IE 6 and 7 for my table in lists tests. I just had the normalizeHtml function always remove them for now, but I'm not sure if there are cases where these attributes should be preserved.

htmlEquals(wymeditor, sublistThreeTablesNoBR);
});

module("table-indent_in_list", {setup: setupWym});
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this, it would make more sense for both the indent_in_list and outdent_in_list tests to live next to the list module. So list-indent_outdent_with_table, I would say. If there are going to be bugs there, the fixes will be in the list indent module, unless the bug was caused by invalid HTML created as a result of the insert (in which case the table-FOO tests should cover it).

Does that make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense.

I moved the tests in the indent_in_list and outdent_in_list modules to a list-indent_outdent_with_table module that lives next to the other list modules as you suggested.

…tems after the last blocking element and instead added a check to prevent adding extra line breaks at that point if one is already there.
@winhamwr
Copy link
Member

The code looks good, but I ran in to some problems while trying this out.

All of these were found using IE8

1. New tables now default to the first list item

Start with just a list, like what happens at the end of this unit test run.

  1. Clear your selection by clicking outside of the editor iframe
  2. Click the dialog to insert a table

The correct behavior would be to insert a table after the list, at the end of the document. Now, it's inserting it in the first list item.

2. Sometimes inserting a table in an li that already has a table nests the tables

In IE8, there are consistently two different br spacers that you can select after a table (two rows for your cursor). They cause weird things when there are multiple tables. If your cursor is in the space immediately following a table (not the second one), doing a table insertion does really weird nesting of the two tables.

  1. Start by running this unit test
  2. Put your cursor right after the 8.
  3. Insert a table
  4. Click to move your cursor to the second "row" of space, right below where it ends up.
  5. Click to move your cursor back to the first "row" of space, right below the table.
  6. Insert a table

The tables get nested together in a really strange way.

3. Attempting to insert a table at the end of the last item in a list inserts the table after the list

I think this is related to the two rows of spacer issue.

  1. Start with that same unit test
  2. Put your cursor right after the 8.
  3. Insert a table
  4. Insert another table
  5. Move your cursor to the second "row" of space after the last table
  6. Insert a table

Instead of being inside the li, this new table gets put entirely outside the list.

@NickMcL
Copy link
Contributor Author

NickMcL commented Jun 18, 2013

In trying to respond to your issues, I've been having some problems.

For the first issue, I am not getting the behavior you described. Following your steps on the unit test you linked inserts the table after the list at the end of the document for me in IE8 as well as in the other browsers I tested.

For the second issue and third issues, these problems seem to be almost exclusive to IE8. This double spacer phenomenon does not occur at all in IE6, IE7, Firefox 21, or Chrome 27 because they only have a single row to click in after a table in a list. The same double spacer phenomenon does occur in IE9 and IE10, but in IE9, it does not cause either of the two issues described, and in IE10, it does not cause the table nesting issue described in your second point, but strangely enough, it does still cause the table insertion outside of a list issue described in your third point.

From my analysis, there is definitely still only one spacer <br /> after the tables in IE8, IE9, and IE10 just like in the other browsers, so I'm not sure why it creates two clickable rows in those browsers. I didn't find any irregular behavior in the execution of the table insertion in those browsers either, so it just seems like the <br /> acts differently in those browsers once it is inserted. I tried mingling around with the CSS quite a bit to see if I could get the <br />s or the spacing in general in IE8-10 to act like they do in the other browsers, but I couldn't get their behavior to change through that method either. Do you have any theories on the source of this problem or a solution to fix it?

It's not related to the problems you described in your comment, but it might also be of note that I noticed there seems to be some odd behavior with table spacing in some situations in IE6 and IE7 that don't occur in the other browsers as well. Particularly, I had an issue with not being able to continue a list after a list item with several tables in it because pressing the enter key would not create a new bullet point after the tables.

@winhamwr
Copy link
Member

Looks great! Thanks so much, Nick.

winhamwr added a commit that referenced this pull request Jun 19, 2013
Fixes #403 Table insertion and manipulation is now possible inside lists
@winhamwr winhamwr merged commit 63a3e2d into wymeditor:master Jun 19, 2013
NickMcL added a commit to NickMcL/wymeditor that referenced this pull request Jul 1, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants