1385514 - Consider making SetTextTransaction a selection preserving transaction

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

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.  :-)

Comment 1

[(no longer active)]

Attachment: Make SetTextTransaction a selection preserving transaction

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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.

Comment 4

[(no longer active)]

(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".

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[(no longer active)]

(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.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[(no longer active)]

This was effectively backed out in bug 1399722.