Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Added a possibility to insert column or row with multiple selected cells #258

Merged
merged 6 commits into from
Mar 3, 2020

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Feb 28, 2020

Suggested merge commit message (convention)

Other: Added a possibility to insert column or row with multiple selected cells. Closes ckeditor/ckeditor5#6125.


Additional information

As we're going to work with multi-range more, I believe it is a good idea to put a similar method to getRangeContainedElement in the Range type at some point.

I was unable to quickly find better assertions to asses model selection and data in the same call, so I used two separate method for both.


As expected, there are weird scenarios cellspan/rowspan, but at the same time it doesn't look that bad - so I didn't put too much energy into any adjustments in this area.

@mlewand mlewand marked this pull request as ready for review February 28, 2020 16:11
@coveralls
Copy link

coveralls commented Feb 28, 2020

Coverage Status

Coverage increased (+0.07%) to 100.0% when pulling a8da6a9 on i/6125 into c9326ee on master.

@mlewand mlewand changed the title Added a possibility to insert columns before and after multiple selected cells Added a possibility to insert column/row before and after multiple selected cells Feb 28, 2020
@mlewand mlewand changed the title Added a possibility to insert column/row before and after multiple selected cells Added a possibility to insert column or row with multiple selected cells Feb 28, 2020
* @param {module:engine/model/range~Range} range
* @returns {module:engine/model/element~Element|null}
*/
export function getRangeContainedElement( range ) {
Copy link
Member

Choose a reason for hiding this comment

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

@Reinmar
Copy link
Member

Reinmar commented Mar 2, 2020

cc @ckeditor/qa-team 

The code looks good. I reported one followup.

@FilipTokarski
Copy link
Member

FilipTokarski commented Mar 3, 2020

  1. Go to table selection test
  2. Go to A "geometry" test editor
  3. Make selection as on gif and click insert column right
    ( I'm clicking two times on this gif, but one is enough )

Expected: an empty column inserted between d and e

Actual: text content jumps from cell to cell, some cells are merged automatically ( like cells from e, f and g in this example ) and additional columns appear ( 9 at the beginning, 11 after first insertion ):

table_merge_multiple1

@FilipTokarski
Copy link
Member

  1. Go to table selection test
  2. Go to A "geometry" test editor
  3. Select this biggest merged cell ( as on gif )
  4. Click insert row below

Expected: row gets inserted below the whole cell
Actual: new row gets inserted as third from the top, while the big cell merges it's cells automatically

  1. Click insert column left

Expected: an empty column gets inserted between d and e
Actual: a column gets inserted between d and e with content from g. Generally geometry is lost and 04 in the big cell is rendered incorrectly

table_merge_multiple2

@Mgsy
Copy link
Member

Mgsy commented Mar 3, 2020

Steps to reproduce

  1. Select two cells.
  2. Open the cell properties balloon.

Current result

The editor crashes.

Error

domconverter.js:198 Uncaught TypeError: Cannot read property 'is' of undefined
    at DomConverter.viewToDom (domconverter.js:198)
    at getBalloonCellPositionData (utils.js:81)
    at TableCellPropertiesUI._showView (tablecellpropertiesui.js:247)
    at ButtonView.<anonymous> (tablecellpropertiesui.js:97)
    at ButtonView.fire (emittermixin.js:209)
    at TemplateToBinding.<anonymous> (buttonview.js:162)
    at ProxyEmitter.callback (template.js:956)
    at ProxyEmitter.fire (emittermixin.js:209)
    at HTMLButtonElement.domListener (emittermixin.js:235)

@FilipTokarski
Copy link
Member

FilipTokarski commented Mar 3, 2020

Steps to reproduce

  1. Add a new table
  2. Select some cells and add a row ( or just select some cells horizontally )
  3. Start typing

Editor crashes:

table_merge_multiple4

ckeditorerror.js:66 Uncaught CKEditorError: model-selection-range-intersects: Trying to add a range that intersects with another range in the selection. Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-model-selection-range-intersects
 {"addedRange":{"start":{"root":"main","path":[0,1,0],"stickiness":"toNext"},"end":{"root":"main","path":[0,1,1],"stickiness":"toPrevious"}},"intersectingRange":{"start":{"root":"main","path":[0,1,0],"stickiness":"toNext"},"end":{"root":"main","path":[0,1,1],"stickiness":"toPrevious"}}}
    at LiveSelection._checkRange (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:64801:11)
    at LiveSelection._prepareRange (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:51649:8)
    at LiveSelection._fixGraveyardSelection (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:51971:26)
    at Model.LiveSelection.listenTo.priority (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:51463:10)
    at Model.fire (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:124298:30)
    at Model.<computed> [as applyOperation] (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:126435:16)
    at applyRemoveOperation (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:69028:8)
    at Writer.remove (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:68013:4)
    at http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:65925:11
    at Model.change (http://localhost:8125/ckeditor5-table/tests/manual/tableselection.js:53798:12)
tableutils.js:59 Uncaught TypeError: Cannot read property 'getChildIndex' of null
    at TableUtils.getCellLocation (tableutils.js:59)
    at TableSelection.getSelectedTableCells (tableselection.js:210)
    at getSelectedTableCells.next (<anonymous>)
    at TableSelection._updateModelSelection (tableselection.js:261)
    at TableSelection.stopSelection (tableselection.js:177)
    at MouseSelectionHandler._handleMouseMove (mouseselectionhandler.js:99)
    at Document.<anonymous> (mouseselectionhandler.js:63)
    at Document.fire (emittermixin.js:209)
    at MouseEventsObserver.fire (domeventobserver.js:96)
    at MouseEventsObserver.onDomEvent (mouseeventsobserver.js:42)

Other info:

  • Cannot reproduce it on master when inserting rows and typing.
  • There's no crash when inserting a column and typing. The behaviour is quite different, which you can check on table with some content inside:

table_merge_multiple5

@Reinmar
Copy link
Member

Reinmar commented Mar 3, 2020

Thanks guys for the reports. I analysed them to understand which are related to the scope of this PR:

  1. Added a possibility to insert column or row with multiple selected cells #258 (comment) – also happens on v17.0.0 with single cell selections – please report a new ticket for this.
  2. Added a possibility to insert column or row with multiple selected cells #258 (comment) – also happens on v17.0.0 since it's related to single cell selections – please report a new ticker for this. In fact, it's strongly related to the previous TC, so you can open one ticket for those cases.
  3. Added a possibility to insert column or row with multiple selected cells #258 (comment) – I can't reproduce this. Works like a charm. Maybe you tested this PR before we merged support for the cell props dialog + multi-cell selections. Could you retest?
  4. Added a possibility to insert column or row with multiple selected cells #258 (comment) – I can't reproduce this. Probably the same story as above. Could you retest?

Since I'm quite confident that the issues that you found are either also reproducible on master/v17.0.0 or were covered by other PRs, I'm merging this.

@Reinmar Reinmar merged commit f3d1dee into master Mar 3, 2020
@Reinmar Reinmar deleted the i/6125 branch March 3, 2020 21:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants