Closed Bug 1730442 Opened 3 years ago Closed 3 years ago

Reduce the dependency for layout information of table editor #1

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details

The table editor (in HTMLTableEditor.cpp) is used by XPCOM API in comm-central or built-in table editor in Firefox which is disabled by default (web apps can enable it though). So it's a minor feature and perhaps we don't need to take care about backward compatibility so much at least for DOM mutation events. Once we fix bug 1710784, the table editor cannot access the latest layout information after changing the DOM. So, we should make it layout information free as far as possible with flushing pending notifications first.

I don't work on this bug actively in these 7 months, but the patches in my local repository required rebasing a lot. Therefore, I'd like to land the finished patches now.

Summary: Reduce the dependency for layout information of table editor → Reduce the dependency for layout information of table editor #1

Our table editor depends on layout information for getting raw/column position
of a cell, checking rawspan/colspan information and table size. Therefore,
they require the latest information, but they don't flush pending layout by
themselves. Therefore, this patch makes them do it by themselves and deleting
unnecessary hack from their tests.

It refers the layout information for getting row/column index of selected cell
element or a cell element which contains selection. However, it can stop
referring it if all callers can specify insertion point of new cells, and it's
possible. Therefore, we can make it free from layout information.

Note that this blocks legacy mutation events during inserting table cell
elements, but perhaps this does not cause problems in the wild.
DOMNodeInserted will be fired after all cell elements are inserted, so
from point of view of the event listeners, editor content is different from
traditional behavior, but this works only when user uses inline table editor
which is disabled by default. I.e., in the wild, this path is rarely run.

Note that the changes are tested by test_nsITableEditor_insertTableColumn.html
and test_nsITableEditor_insertTableCell.html.

Depends on D146360

This change is tested by test_nsITableEditor_getFirstRow.html and
test_nsITableEditor_insertTableColumn.html.

Depends on D146361

There is no direct test because of no corresponding XPCOM method, but this is
called only by HTMLEditor::InsertTableColumnsWithTransaction() which is
tested by test_nsITableEditor_insertTableColumn.html. Anyway, the chagne is
really simple.

Depends on D146362

It needs to work with the latest layout information to consider which cell
element is the insertion point due to rowspan and colspan. Therefore,
this patch makes it collects all cell data before touching the DOM except
the case that it needs to normalize the table to make it rectanble.

Note that the case requring the normalizer should be fixed in a later patch.

This method is corresponding to an XPCOM method. Therefore, this is tested
by test_nsITableEditor_insertTableColumn.html.

And also it's used by the inline table editor, but we don't have automated tests
for this because of no API to get the buttons. Therefore, I tested it by my
hand.

Depends on D146363

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b67c4035c12f part 1: Make all `nsITableEditor` features flush pending layout at start to handle their jobs r=m_kato https://hg.mozilla.org/integration/autoland/rev/ada0dda5bcde part 2: Rewrite `HTMLEditor::GetSelectedOrParentTableElement()` as returing `Result<RefPtr<Element>, nsresult>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/bd3324b635f4 part 3: Make `HTMLEditor::InsertTableCellsWithTransaction()` work without layout information r=m_kato https://hg.mozilla.org/integration/autoland/rev/943d4e4b0770 part 4: Rewrite `HTMLEditor::GetFirstTableRowElement()` as returing `Result<RefPtr<Element>, nsresult>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/e60a5cd69184 part 5: Rewrite `HTMLEditor::GetNextTableRowElement()` as returning `Result<RefPtr<Element>, nsresult>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/e5d1671b3bbc part 6: Make `HTMLEditor::InsertTableColumnsWithTransaction()` collects all necessary data before touching the DOM tree r=m_kato https://hg.mozilla.org/integration/autoland/rev/f6d7f162e57a part 7: Rewrite `HTMLEditor::InsertTableRowsWithTransaction()` to collect any cell information before touching the DOM tree r=m_kato

Backed out 7 changesets (bug 1730442) for causing reftest failures in editor/reftests/inline-table-editor-position-after-updating-table-size-from-input-event-listener.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/9c5d06519b170fd95fd257aecfb5f3c824aa23fd

Push with failures

Failure log

Flags: needinfo?(masayuki)

Oh, sorry. I've not checked the reftest result in tryserver.

Flags: needinfo?(masayuki)
Attachment #9276535 - Attachment description: Bug 1730442 - part 6: Make `HTMLEditor::InsertTableColumnsWithTransaction()` collects all necessary data before touching the DOM tree r=m_kato! → Bug 1730442 - part 6: Make `HTMLEditor::InsertTableColumnsWithTransaction()` collect all necessary data before touching the DOM tree r=m_kato!
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b4dcdb40970d part 1: Make all `nsITableEditor` features flush pending layout at start to handle their jobs r=m_kato https://hg.mozilla.org/integration/autoland/rev/76fac2be98e7 part 2: Rewrite `HTMLEditor::GetSelectedOrParentTableElement()` as returing `Result<RefPtr<Element>, nsresult>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/d51b14fb40ed part 3: Make `HTMLEditor::InsertTableCellsWithTransaction()` work without layout information r=m_kato https://hg.mozilla.org/integration/autoland/rev/0d370e2abf6d part 4: Rewrite `HTMLEditor::GetFirstTableRowElement()` as returing `Result<RefPtr<Element>, nsresult>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/e44e3784a8f2 part 5: Rewrite `HTMLEditor::GetNextTableRowElement()` as returning `Result<RefPtr<Element>, nsresult>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/9282a4bfd86f part 6: Make `HTMLEditor::InsertTableColumnsWithTransaction()` collect all necessary data before touching the DOM tree r=m_kato https://hg.mozilla.org/integration/autoland/rev/dafaccab50af part 7: Rewrite `HTMLEditor::InsertTableRowsWithTransaction()` to collect any cell information before touching the DOM tree r=m_kato
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: