Get rid of nsIEditor.setShouldTxnSetSelection, instead insertNode and deleteNode should have optional argument for equivalent feature
Categories
(Core :: DOM: Editor, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox122 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 3 open bugs)
Details
Attachments
(3 files)
nsIEditor.setShouldTxnSetSelection is used only with nsIEditor.insertNode and nsIEditor.deleteNode in comm-central. So, it's enough to make insertNode and deleteNode have new optional argument to do same thing.
The motivation of this is, I want to centralize selection updates instead of each DOM tree updater including transactions do it multiple times because of the performance reason of DOM Selection and making editor implementation simpler.
Additionally, comm-central does not guarantee the clean up of calling nsIEditor.setShouldTxnSetSelection(false) with try-finally. From point of view of this, this API style is a bad design.
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Updated•2 years ago
|
| Comment hidden (off-topic) |
|
||
Comment 2•2 years ago
|
||
Sorry, there was a problem with the detection of inactive users. I'm reverting the change.
|
|
Assignee | |
Comment 3•7 months ago
|
||
nsIEditor.setShouldTxnSetSelection can preserve selection across multiple
editing. Therefore, we cannot manage the state only in the stack.
It's used only in comm-central, and used only with insertNode and
deleteNode. Therefore, adding new param to them to preserve selection
must be enough.
While I'm writing this patch, I realized that input event is not fired
by these methods because nobody set a placeholder transaction. That may
lead Thunderbird only IME crash bugs due to IMEContentObserver is not
notified editor properly. Therefore, this may fix some Thunderbird only
crashes.
Note that deleteNode should not update selection. However, I'm not 100%
sure that. Therefore, I add new param to deleteNode too. However,
some reviewers think it's unnecessary, I'll remove it before landing.
Finally, beforeinput and input caused by the method calls start updating
selection. However, I think that it should be better behavior. If Thunderbird
needs to guarantee that selection is set to whether the user expected when
it calls these methods with preserving selection.
|
|
Assignee | |
Comment 4•7 months ago
|
||
It allows multiple edit action preserves Selection when updating the DOM tree.
However, most callers do not use finally to reset the state, and this makes
edit action handling complicated in some edge cases. Therefore, let's stop
supporting this feature. If Thunderbird requires the feature, the developers
should request additional param for preserving Selection like the preceding
patch.
Depends on D196004
|
|
Assignee | |
Comment 5•7 months ago
|
||
It'll be removed and nsIEditor.insertNode() and nsIEditor.deleteNode()
have new optional parameter which if you set to true, selection won't be
modified by the editor (except beforeinput and input event listeners).
|
|
Assignee | |
Updated•7 months ago
|
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/ac096e9798c4 part 1: Make `nsIEditor.insertNode` and `nsIEditor.deleteNode` take an optional parameter to preserve selection r=m_kato
|
||
Comment 7•7 months ago
|
||
| bugherder | ||
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/12707a2d0153 part 2: Make comm-central stop using `nsIEditor.setShouldTxnSetSelection` r=mkmelin https://hg.mozilla.org/comm-central/rev/a2942a799f19 Fix build bustage on comm-central. rs=bustage-fix
|
|
Assignee | |
Comment 9•7 months ago
|
||
(In reply to Pulsebot from comment #8)
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/12707a2d0153
part 2: Make comm-central stop usingnsIEditor.setShouldTxnSetSelection
r=mkmelin
https://hg.mozilla.org/comm-central/rev/a2942a799f19
Fix build bustage on comm-central. rs=bustage-fix
Thank you for the landing and fixing the bustage. I don't know why my try build succeeded to build...
|
||
Comment 10•7 months ago
|
||
(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)
I don't know why my try build succeeded to build...
It built from what was the mozilla-central tip at the time. For it to have used your part 1 patch, you would have had to do a special Try build pointing to it (I think you knew that but I linked it anway).
|
|
Assignee | |
Comment 11•7 months ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #10)
(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)
I don't know why my try build succeeded to build...
It built from what was the mozilla-central tip at the time. For it to have used your part 1 patch, you would have had to do a special Try build pointing to it (I think you knew that but I linked it anway).
Oh, thanks. Oddly, I failed to update to the changeset which changed .gecko_rev.yml only when I pushed it.
FYI: The reason why I tried multiple times is, the document may be to old about pushing try builds. The decision task failed except when I push via Lando. Could you notify the document owner of updating required?
|
||
Comment 12•7 months ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/2b83013fcfc8 part 3: Get rid of `nsIEditor.setShouldTxnSetSelection` r=m_kato DONTBUILD
|
|
||
Comment 13•7 months ago
|
||
I see your try run is based on mozilla-central from 9 days ago, but your try-comm-central run is based on comm-central from 5 days ago. In the meantime all of this happened (including some taskcluster config changes) on mozilla-central and comm-central has been updated to match. That's where the decision task failure has come from.
Ideally you'd push to try and try-cc at the same time, using the tip of both trees (assuming comm-central is in a good state, which isn't always the case). I'll add a note to that effect in the documentation.
|
||
Comment 14•7 months ago
|
||
| bugherder | ||
Description
•