1671556 - Redesign `nsITableEditor.getFirstSelectedCell()` and relatives

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

For fixing bug 1669479 easier, I'd like to redesign nsITableEditor.getFirstSelectedCell() and relatives.

Currently, they are designed as iterator methods to retrieve selected table cells one by one, and returns the selected range for the cell element.

So, they are stateful and they may behave odd if selection or the DOM tree changed during the calls.

Additionally, nobody users the returned range.

• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/editor/ui/dialogs/content/EdTableProps.js#1320-1321,1323,1354
• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/mail/components/compose/content/dialogs/EdTableProps.js#1317,1320,1351
• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/editor/ui/composer/content/editor.js#2947
• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/editor/ui/composer/content/editor.js#2982
• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/mail/components/compose/content/editor.js#2308
• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/mail/components/compose/content/editor.js#2343

And surprisingly, these callers always causes an exception for due to few arguments.

• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/editor/ui/dialogs/content/EdInsertTable.js#221
• https://searchfox.org/comm-central/rev/10eba65ee28fe96fc58eb49eeb2b3af80a45c5bc/mail/components/compose/content/dialogs/EdInsertTable.js#221

So, we can make the API return array of selected cell elements instead.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 1: Make `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` stop returning selected range with out argument r=m_kato!

These APIs are used only in comm-central (not in BlueGriffon) and nobody
refers the out argument which is selected range of the cell selection.
Therefore, we should drop them to make the APIs simpler.

Depends on D94225

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 2: Replace `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` with `nsITableEditor.getSelectedCells()` r=m_kato!

nsITableEditor.getFirstSelectedCell() and
nsITableEditor.getNextSelectedCell() use HTMLEditor::mSelectedCellIndex
via their internal methods. That makes these API unstable if mutation happens
between the calls. Therefore, it's really simpler and stabler that there is
an API to return selected table cell elements once.

Therefore this patch creates new API, nsITableEditor.getSelectedCells(),
which returns array of selected table cell elements. And it stops taking
inconsistent behavior of the old API:

1. Even if there is no selection. it does not throw an exception.
   https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/editor/libeditor/HTMLTableEditor.cpp#3956-3961
2. Even if it meets selection range in a text node (i.e.,
   nsRange::GetChildAtStartOffset() returns nullptr), this just ignores the
   range.
   https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/editor/libeditor/HTMLTableEditor.cpp#4041-4047

The following patches will remove their internal methods too.

Depends on D94226

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 3: Make `HTMLEditor::SetHTMLBackgroundColorWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

Depends on D94228

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 4: Make `HTMLEditor::HTMLWithContextInserter::Run()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!

Although it's not tested yet, but I have no idea how to test it.
https://coverage.moz.tools/#revision=latest&path=editor%2Flibeditor%2FHTMLEditorDataTransfer.cpp&view=file&line=545

Depends on D94229

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 5: Make `HTMLEditor::HandleDeleteSelection()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!

Depends on D94230

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 6: Make `HTMLEditor::DeleteTableCellWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

This method is tested by test_nsITableEditor_deleteTableCell.html.

Depends on D94231

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 7: Make `HTMLEditor::DeleteTableCellContentsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

This is tested by test_nsITableEditor_deleteTableCellContents.html.

Depends on D94232

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 8: Make `HTMLEditor::DeleteSelectedTableColumnWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

This is tested by test_nsITableEditor_deleteTableColumn.html.

Depends on D94233

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 9: Make `HTMLEditor::DeleteSelectedTableRowsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

This is tested by test_nsITableEditor_deleteTableRow.html.

Depends on D94234

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 10: Make `HTMLEditor::JoinTableCells()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

Unfortunately, there is no automated tests for nsITableEditor.joinTableCells()
and it'll take a couple of days to write its testcase because of too big
method. And it's not so important for Firefox (although it's used in
comm-central). Therefore, this patch skips to check it.

Note that CellAndIndexes uses HTMLEditor::GetFirstSelectedTableCellElement()
to retrieve first selected cell element and then, it retrieves its indexes.
https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/editor/libeditor/HTMLTableEditor.cpp#4090,4098,4109
Therefore, this patch makes it stop using this simple structure too.

Depends on D94235

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 11: Make `HTMLEditor::GetSelectedCellsType()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!

Depends on D94236

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 12: Make `HTMLEditor::GetSelectedOrParentTableElement()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!

This is tested by test_nsITableEditor_getSelectedOrParentTableElement.html.

Depends on D94238

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 13: Make `HTMLEditor::GetFirstSelectedCellInTable()` stop using `CellAndIndexes` which uses `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!

The method is tested by test_nsITableEditor_getFirstSelectedCellInTable.html.

And now, nobody uses CellAndIndexes so that this patch removes it.

Depends on D94239

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1671556 - part 14: Get rid of `HTMLEditor::GetFirstSelectedTableCellElement()`, `HTMLEditor::GetNextSelectedTableCellElement()` and `HTMLEditor::mSelectedCellIndex` r=m_kato!

Depends on D94240

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/585df60d18f9
part 1: Make `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` stop returning selected range with out argument r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fe3735c5aac9
part 2: Replace `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` with `nsITableEditor.getSelectedCells()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/17405d242d1b
part 3: Make `HTMLEditor::SetHTMLBackgroundColorWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 16

[Alexandru Michis [:malexandru]]

Backed out 4 changesets (Bug 1671556, Bug 1669479) for causing bustages in HTMLEditUtils.h

Backout link: https://hg.mozilla.org/integration/autoland/rev/2221accc694f206651cccf4adfa53344c193b636

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&tochange=2221accc694f206651cccf4adfa53344c193b636&fromchange=315d9195557b184a0915e31c1566ace9e5582960&selectedTaskRun=LV79rWHbSQibClGoYfNDFA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319535344&repo=autoland&lineNumber=76667

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2f73fb6876b
part 1: Make `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` stop returning selected range with out argument r=m_kato
https://hg.mozilla.org/integration/autoland/rev/aefb7fc23dc0
part 2: Replace `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` with `nsITableEditor.getSelectedCells()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2a1f19d58fa5
part 3: Make `HTMLEditor::SetHTMLBackgroundColorWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3008d2467ea
part 4: Make `HTMLEditor::HTMLWithContextInserter::Run()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef35e6316c77
part 5: Make `HTMLEditor::HandleDeleteSelection()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26271 for changes under testing/web-platform/tests

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78130fed97b7
part 6: Make `HTMLEditor::DeleteTableCellWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba7aed374703
part 7: Make `HTMLEditor::DeleteTableCellContentsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 23

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b2f73fb6876b
https://hg.mozilla.org/mozilla-central/rev/aefb7fc23dc0
https://hg.mozilla.org/mozilla-central/rev/2a1f19d58fa5
https://hg.mozilla.org/mozilla-central/rev/b3008d2467ea
https://hg.mozilla.org/mozilla-central/rev/ef35e6316c77
https://hg.mozilla.org/mozilla-central/rev/78130fed97b7

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f5ec513f9d8
part 8: Make `HTMLEditor::DeleteSelectedTableColumnWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4de0c383896c
part 9: Make `HTMLEditor::DeleteSelectedTableRowsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 26

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ba7aed374703
https://hg.mozilla.org/mozilla-central/rev/7f5ec513f9d8
https://hg.mozilla.org/mozilla-central/rev/4de0c383896c

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed2cf77cd4fa
part 10: Make `HTMLEditor::JoinTableCells()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c57cdfde03a
part 11: Make `HTMLEditor::GetSelectedCellsType()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/535228b4464a
part 12: Make `HTMLEditor::GetSelectedOrParentTableElement()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6bbb4ed602e
part 13: Make `HTMLEditor::GetFirstSelectedCellInTable()` stop using `CellAndIndexes` which uses `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57bccf71d4f4
part 14: Get rid of `HTMLEditor::GetFirstSelectedTableCellElement()`, `HTMLEditor::GetNextSelectedTableCellElement()` and `HTMLEditor::mSelectedCellIndex` r=m_kato

Comment 33

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ed2cf77cd4fa
https://hg.mozilla.org/mozilla-central/rev/3c57cdfde03a
https://hg.mozilla.org/mozilla-central/rev/535228b4464a
https://hg.mozilla.org/mozilla-central/rev/a6bbb4ed602e
https://hg.mozilla.org/mozilla-central/rev/57bccf71d4f4