Closed Bug 1671556 Opened 4 years ago Closed 4 years ago

Redesign `nsITableEditor.getFirstSelectedCell()` and relatives

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
84 Branch
Tracking Flags:
Tracking Status
firefox84 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 3 open bugs)

Details

Attachments

(14 files)

Bug 1671556 - part 1: Make `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` stop returning selected range with out argument r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 2: Replace `nsITableEditor.getFirstSelectedCell()` and `nsITableEditor.getNextSelectedCell()` with `nsITableEditor.getSelectedCells()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 3: Make `HTMLEditor::SetHTMLBackgroundColorWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 4: Make `HTMLEditor::HTMLWithContextInserter::Run()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 5: Make `HTMLEditor::HandleDeleteSelection()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 6: Make `HTMLEditor::DeleteTableCellWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 7: Make `HTMLEditor::DeleteTableCellContentsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 8: Make `HTMLEditor::DeleteSelectedTableColumnWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 9: Make `HTMLEditor::DeleteSelectedTableRowsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 10: Make `HTMLEditor::JoinTableCells()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 11: Make `HTMLEditor::GetSelectedCellsType()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 12: Make `HTMLEditor::GetSelectedOrParentTableElement()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 13: Make `HTMLEditor::GetFirstSelectedCellInTable()` stop using `CellAndIndexes` which uses `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1671556 - part 14: Get rid of `HTMLEditor::GetFirstSelectedTableCellElement()`, `HTMLEditor::GetNextSelectedTableCellElement()` and `HTMLEditor::mSelectedCellIndex` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review

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.

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

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

Severity: -- → S3
Priority: -- → P3

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

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

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

The method is tested by test_nsITableEditor_getFirstSelectedCellInTable.html.

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

Depends on D94239

Pushed by masayuki@d-toybox.com:
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
Pushed by masayuki@d-toybox.com:
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
Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b3008d2467ea
part 4: Make `HTMLEditor::HTMLWithContextInserter::Run()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ef35e6316c77
part 5: Make `HTMLEditor::HandleDeleteSelection()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26271 for changes under testing/web-platform/tests
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/78130fed97b7
part 6: Make `HTMLEditor::DeleteTableCellWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ba7aed374703
part 7: Make `HTMLEditor::DeleteTableCellContentsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7f5ec513f9d8
part 8: Make `HTMLEditor::DeleteSelectedTableColumnWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4de0c383896c
part 9: Make `HTMLEditor::DeleteSelectedTableRowsWithTransaction()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato
Upstream PR merged by moz-wptsync-bot
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ed2cf77cd4fa
part 10: Make `HTMLEditor::JoinTableCells()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3c57cdfde03a
part 11: Make `HTMLEditor::GetSelectedCellsType()` stop using `HTMLEditor::Get(First|Next)SelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/535228b4464a
part 12: Make `HTMLEditor::GetSelectedOrParentTableElement()` stop using `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a6bbb4ed602e
part 13: Make `HTMLEditor::GetFirstSelectedCellInTable()` stop using `CellAndIndexes` which uses `HTMLEditor::GetFirstSelectedTableCellElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/57bccf71d4f4
part 14: Get rid of `HTMLEditor::GetFirstSelectedTableCellElement()`, `HTMLEditor::GetNextSelectedTableCellElement()` and `HTMLEditor::mSelectedCellIndex` r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox84: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: