Closed Bug 1671557 Opened 11 months ago Closed 11 months ago

Some APIs in `nsITableEditor` will be changed by bug 1671556

Categories

(Thunderbird :: Upstream Synchronization, task)

Tracking

(thunderbird_esr78 unaffected, thunderbird83 unaffected)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird83 --- unaffected

People

(Reporter: masayuki, Assigned: mkmelin)

References

Details

Attachments

(1 file)

See bug 1671556 comment 0 for the detail. I'll get rid of the stateful API to make table editor simpler and create new API which is easier to use. The new design will be posted here (not fixed yet in my brain).

Yes, it is. Nobody uses the selecred range for a selected cell element, and it can know really easy, the range is same as the result of Range.selectNode(). And the new API simply returns array of selected cells at once. It ignores selection ranges whose index is 2 or more if a range does not select only one cell element. And different from the old API, it does not throw exception unless editor has already destroyed.

I think this covers it.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9183632 - Flags: review?(khushil324)
Comment on attachment 9183632 [details] [diff] [review]
bug1671557_editor_changes.patch

Review of attachment 9183632 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the quick fix. Getting rid of the stateful API makes `HTMLEditor` much simpler.

::: mail/components/compose/content/dialogs/EdTableProps.js
@@ +1319,4 @@
>      return;
>    }
>  
>    if (gSelectedCellCount == 1) {

In strictly speaking, this is not safe. If `selectedCells.length` is not 1, the selection and/or the DOM tree has been changed after `gSelectedCellCount` is updated.

::: mail/components/compose/content/editor.js
@@ +2302,5 @@
> +  // We have at least one row
> +  rows++;
> +
> +  var lastIndex = rowObj.value;
> +  for (let cell of editor.getSelectedCells()) {

I'm not familiar with JS though, is this `getSelectedCells()` called once?

@@ +2333,5 @@
> +  // We have at least one column
> +  columns++;
> +
> +  var lastIndex = colObj.value;
> +  for (let cell of editor.getSelectedCells()) {

Ditto.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

In strictly speaking, this is not safe. If selectedCells.length is not 1,
the selection and/or the DOM tree has been changed after
gSelectedCellCount is updated.

gSelectedCellCount is updated every time we select anything in the table. But I guess we can use editor.getSelectedCells().length now and remove the usage of the gSelectedCellCount.

I'm not familiar with JS though, is this getSelectedCells() called once?

Yes, it gets called only once.

Comment on attachment 9183632 [details] [diff] [review]
bug1671557_editor_changes.patch

Review of attachment 9183632 [details] [diff] [review]:
-----------------------------------------------------------------

All the changes look good to me. I have sent it for a try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=77d838f347c5cc7c4a85382e379dd1bdba96703b
Check once on ``gSelectedCellCount``.
Attachment #9183632 - Flags: review?(khushil324) → review+

I'll change this occurrence to use getSelectedCells().length instead of gSelectedCellCount, but leave other places using it since doesn't look like I can get rid of it very trivially atm.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/dd200251aa68
port editor.getFirstSelectedCell and editor.getNextSelectedCell changes from bug 1671556. r=khushil DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED

Reopening as I suppose there is more coming.

Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Target Milestone: --- → 84 Branch

Looks like we're done.

Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.