As I was profiling 1346723, I spent quite a bit of time to understand to understand why the transaction needs to manipulate the selections like this. It is very strange for subtransactions to be manipulating the selections directly like this, because then the editor will just overwrite what they do inside TextEditRules::AfterEdit() (from the CollapseSelectionToTrailingBRIfNeeded() call.) I finally realized the reason seems to be a forgotten AutoTransactionsConserveSelection in TextEditRules::WillSetText(), not sure if this is quite intentional, but as far as I can tell there's no tests that currently depend on the inner transaction manipulating the selection, and I'm not 100% convinced that this selection manipulation is visible to the outside world at this point. I suggest making this change at the very least to make this transaction behave like all the rest. BTW I also added the old school comment to keep things consistent with existing code. :-)

This makes the SetTextTransaction transaction behave more similarly to the rest of the editor transactions, by making sure the inner transactions don't manipulate the selections themselves and leave it up to the AfterEdit() method to take care of adjusting the selection when the entire editing operation is finished.

The one thing we need to ensure is that .value sets on an input with an editor still move the selection to the end of the text. Is that still true?

Comment on attachment 8891575 [details] [diff] [review] Make SetTextTransaction a selection preserving transaction Okay, but please add automated tests like this: https://jsfiddle.net/d_toybox/7qau36am/ > dontSpazMySelection BTW, what's "spaz"? According to my dictionary, the word is not a good word and I don't understand why it is a good name for this case. In this case, it prevents unnecessary selection changes which will be overwritten.

(In reply to Boris Zbarsky [:bz] from comment #2) > The one thing we need to ensure is that .value sets on an input with an > editor still move the selection to the end of the text. Is that still true? Sure, see https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/editor/libeditor/TextEditRules.cpp#255. (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3) > Comment on attachment 8891575 [details] [diff] [review] > Make SetTextTransaction a selection preserving transaction > > Okay, but please add automated tests like this: > https://jsfiddle.net/d_toybox/7qau36am/ > > > dontSpazMySelection > > BTW, what's "spaz"? According to my dictionary, the word is not a good word > and I don't understand why it is a good name for this case. In this case, it > prevents unnecessary selection changes which will be overwritten. Huh, I didn't know this is an offensive word. I think it means "change" here, I'll land another change to replace the occurrences of "spaz" with "change".

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/91637b17112c Part 1: Make SetTextTransaction a selection preserving transaction; r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/c579ac37ac11 Part 2: Replace 'spaz' with 'change' in the editor code

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3) > Comment on attachment 8891575 [details] [diff] [review] > Make SetTextTransaction a selection preserving transaction > > Okay, but please add automated tests like this: > https://jsfiddle.net/d_toybox/7qau36am/ Oh, sorry, I didn't see this part. I'm 99.99% sure we do have tests for this elsewhere in the tree, but will add some explicit tests for this in bug 1385926.

https://hg.mozilla.org/mozilla-central/rev/91637b17112c https://hg.mozilla.org/mozilla-central/rev/c579ac37ac11

Backed out from Fx56 for causing bug 1399722. https://hg.mozilla.org/releases/mozilla-beta/rev/678bea28fe03 (FIREFOX_56b13_RELBRANCH) https://hg.mozilla.org/releases/mozilla-release/rev/2bb9e6526ce9

This was effectively backed out in bug 1399722.