Closed Bug 1385514 Opened 2 years ago Closed 2 years ago

Consider making SetTextTransaction a selection preserving transaction

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

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.
Attachment #8891575 - Flags: review?(masayuki)
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.
Attachment #8891575 - Flags: review?(masayuki) → review+
(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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1399722
This was effectively backed out in bug 1399722.
You need to log in before you can comment on or make changes to this bug.