Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 7 years ago

As far as my investigation, there are 4 active classes which implements nsIEditActionLisstener. 3 of them are our internal classes, HTMLEditRules, nsTextServicesDocument and mozInlineSpellChecker. The other is, ThunderHTMLedit addon. (Other addons are not available even on Thunderbird 52. So, we can ignore them.)

Nobody implements WillCreateNode, WillInsertNode, WillSplitNode, WillInsertText and WillDeleteText actually. So, we can just drop them simply.

ThunderHTMLedit implements all Did* methods. So, if we keep supporting it, we cannot remove any Did* methods. Otherwise, nobody implements DidDeleteSelection, so, we can drop this too. And if we can make some our classes register EditorBase directly, we can get rid of more methods.

	Jorg K (CEST = GMT+2)
	Comment 1 • 7 years ago

I'm the author of ThunderHTMLedit and that's still working at TB 59 and I intend to maintain it.

Basically I'm listening to any changes in the document so that when I switch to the HTML view I know to regenerate the HTML. This is the add-on code:

  // nsIEditActionListener
  DidCreateNode:       function(tag, node, parent, position ) { this.Dirty(); },
  DidDeleteNode:       function(child)                        { this.Dirty(); },
  DidDeleteSelection:  function(selection)                    { this.Dirty(); this.Action(); },
  DidDeleteText:       function(textNode, offset, length)     { this.Dirty(); this.Action(); },
  DidInsertNode:       function(node, parent, position)       { this.Dirty(); },
  DidInsertText:       function(textNode, offset, string)     { this.Dirty(); this.Action(); },
  DidJoinNodes:        function(leftNode, rightNode, parent)  { this.Dirty(); },
  DidSplitNode:        function(existingRightNode, offset, newLeftNode) { this.Dirty(); },
  WillCreateNode:      function(tag, parent, position)        { },
  WillDeleteNode:      function(child)                        { },
  WillDeleteSelection: function(selection )                   { },
  WillDeleteText:      function(textNode, offset, length)     { },
  WillInsertNode:      function(node, parent, position)       { },
  WillInsertText:      function(textNode, offset, string)     { },
  WillJoinNodes:       function(leftNode,  rightNode, parent) { },
  WillSplitNode:       function(existingRightNode, offset)    { },

So is there any other or better way to get notified? Why do you think it's a good idea to remove a feature from a *toolbox*?

Note that the add-on where the ThunderHTMLcode was forked from, Stationery 0.8.6.2, has removed their home-grown HTML editor using nsIEditActionListener and switched to integrating Ace (https://ace.c9.io/) in their version 0.9.0.2. Ace doesn't use nsIEditActionListener, so perhaps it's about time to fork again and integrate Ace into ThunderHTMLedit as well.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 7 years ago

(In reply to Jorg K (GMT+1) from comment #1)
> I'm the author of ThunderHTMLedit and that's still working at TB 59 and I
> intend to maintain it.

Yeah, therefore, I cced you.

> So is there any other or better way to get notified?

First of all, we still don't decide to remove all of them. If somebody still need them, we can keep only necessary methods.

On the other hand, your addon just observes any actions. Cannot use "input" event or mutation observer instead?

> Why do you think it's a
> good idea to remove a feature from a *toolbox*?

Currently, we call a set of Will and Did methods of edit action listeners every DOM change caused by edit action. Those methods are virtual call and we need to copy all listeners to local variable twice (one is for Will, the other is for Did) before loop to call a method and add all refcounters for guaranteeing the lifetime of the listeners. So these things really expensive. We want to reduce such cost for improving UI response.

	Jorg K (CEST = GMT+2)
	Comment 3 • 7 years ago

Thanks for keeping me in the loop. Please go ahead as you deem fit, even with a complete removal. I'll find a way of adapting the add-on.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 7 years ago

Oh, my investigation might not be enough. I found FinderHighlighter.jsm implements only 3 methods:
https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/toolkit/modules/FinderHighlighter.jsm#1495,1516,1542

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 7 years ago

After fixing bug 1430021, bug 1430785 and bug 1430982, EditorBase never calls methods of nsIEditActionListener when any installed addons don't listen to edit actions and user doesn't highlight findbar's result. So, these series of bugs must improve the performance in most cases.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 7 years ago

FYI: I'll land all patches after the merge for 60.

	Jorg K (CEST = GMT+2)
	Comment 7 • 7 years ago

Thanks!

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 8 • 7 years ago

I was going to reply on dev.platform, and see that what conversation there has been is here.  I support removing this interface and the callbacks entirely; if it's not used in mozilla-central, we shouldn't maintain it.  There are some things that we can help comm-central maintain, for a short amount of time; and we can definitely try to find replacements (as Masayuki did -- thanks!); but there are lots of things that must be removed from mozilla-central, and we should do that as quickly and efficiently as we can.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 7 years ago

(In reply to Nick Alexander :nalexander from comment #8)
> There are some things that we can help comm-central maintain, for a short
> amount of time; and we can definitely try to find replacements (as Masayuki
> did -- thanks!); but there are lots of things that must be removed from
> mozilla-central, and we should do that as quickly and efficiently as we can.

Yeah, but editor module is really important for both Thunderbird and Bluegriffon. Unless something blocks our improvements, we should keep to having such interfaces in editor.  Although we shouldn't use such slow path at runtime of Firefox as far as possible.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 10 • 7 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> (In reply to Nick Alexander :nalexander from comment #8)
> > There are some things that we can help comm-central maintain, for a short
> > amount of time; and we can definitely try to find replacements (as Masayuki
> > did -- thanks!); but there are lots of things that must be removed from
> > mozilla-central, and we should do that as quickly and efficiently as we can.
> 
> Yeah, but editor module is really important for both Thunderbird and
> Bluegriffon. Unless something blocks our improvements, we should keep to
> having such interfaces in editor.  Although we shouldn't use such slow path
> at runtime of Firefox as far as possible.

There are lots of things that are important for lots of XUL projects that consume Firefox.  They are going to have to find a way to not use XUL in the very near future.  In addition, Firefox shouldn't have the expense -- both at runtime but more importantly at maintenance time or creation time -- of two pathways.

In any case, you asked, and I responded as a voice on the extreme side of the continuum between "rip it out now" and "support it forever".  Somewhere in the middle, the right path for right now lies.  I'm sure you'll do a great job finding the right path :)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 11 • 7 years ago

Ah, this should be marked as FIXED. Unless user shows the findbar and is using its highlight mode, editor won't use nsIEditActionListener to notify ending an edit action.