Closed Bug 1350553 Opened 7 years ago Closed 4 years ago

Redoing selected text's style change moves caret to outside of the new style

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox55 --- affected

People

(Reporter: masayuki, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [h2review-noted])

Attachments

(2 files)

Bug 1350553 - Move mEditorBase into EditTransactionBase
(deleted), text/x-review-board-request
masayuki
: review+
Details
Bug 1350553 - Change selection on redo only if it was changed originally
(deleted), text/x-review-board-request
Details 
STR:
1. Load https://jsfiddle.net/d_toybox/60s13tp4/
2. Select some text in the editor
3. Click the button
4. Type Ctrl+Z, then, type Ctrl+Y or Ctrl+Shift+Z to redo
5. Type something.

Expected result:

Typed text is bold.

Actual result:

Typed text is not bold.


Gecko collapsed selection after the selected text after redo. However, Chrome and Edge keeps select the range and typing ArrowRight does not move after the <b> element.
I think Chrome just always places the selection at the left of tag boundaries.  So it only allows

  foo[]<b>bar</b>
  <b>foo[]</b>bar

but not

  foo<b>[]bar</b>
  <b>foo</b>[]bar

You can test this:

data:text/html,<!doctype html>
<div contenteditable>foo<b>bar</b></div>
<script>
getSelection().collapse(document.querySelector("b").firstChild, 0);
document.body.textContent = getSelection().focusNode.nodeValue;
</script>

In Chrome it outputs "foo", even though we put the selection in "bar" via script.  We probably don't want to do this, and if we do it would be a huge deal to change, but I think this is the behavior we should target in specific cases.

I once studied this in some detail, but I can't remember where I wrote up the findings.  My conclusion was that Chrome makes the most sense.  We do weird stuff where which side of the tag you're on depends on how you get there, which I don't think makes sense from a user standpoint.
Thanks, I just think that redoing in this case should select the range which has styled.
True, in this case it should re-select the whole thing.
Do you have any ideas on how to fix this?  I tried putting an AutoTransactionsConserveSelection in EditorBase::Redo, but it didn't seem to help.  (I'm not sure it would be correct anyway.)
Flags: needinfo?(masayuki)
That will be wrong in the case where the original caller of DoTransaction actually wants the selection to be modified, won't it?  Shouldn't we record the value of GetShouldTxnSetSelection in DoTransaction and reuse the same value in RedoTransaction?
Flags: needinfo?(masayuki)
So I spent a bunch of time on this.  With the patches, in the test case, it no longer collapses the selection, but now the selection vanishes entirely on redo (getSelection().focusNode is null).  I have no idea why.  I triggered a try run just to see if any test results change.  I tried attaching a debugger but it didn't work.  Do you have any ideas?
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #9)
> So I spent a bunch of time on this.  With the patches, in the test case, it
> no longer collapses the selection, but now the selection vanishes entirely
> on redo (getSelection().focusNode is null).

Sound like that Selection.removeAllRanges() are called but not added new range?

I'll check the patch soon.
Flags: needinfo?(masayuki)
Comment on attachment 8854457 [details]
Bug 1350553 - Move mEditorBase into EditTransactionBase

https://reviewboard.mozilla.org/r/126392/#review129270

::: editor/libeditor/EditAggregateTransaction.cpp:16
(Diff revision 1)
> -EditAggregateTransaction::EditAggregateTransaction()
> +EditAggregateTransaction::EditAggregateTransaction(EditorBase& mEditorBase)
> +  : EditTransactionBase(mEditorBase)

Although, container of other transactions may not need editor instance actually, though.

::: editor/libeditor/PlaceholderTransaction.cpp:19
(Diff revision 1)
>  PlaceholderTransaction::PlaceholderTransaction(
>                            EditorBase& aEditorBase,
>                            nsIAtom* aName,
>                            UniquePtr<SelectionState> aSelState)
> -  : mAbsorb(true)
> +  : EditAggregateTransaction(aEditorBase)

Although, container of other transactions may not need editor instance actually, though.
Attachment #8854457 - Flags: review+
Comment on attachment 8854458 [details]
Bug 1350553 - Change selection on redo only if it was changed originally

https://reviewboard.mozilla.org/r/126394/#review129276

I don't think that this is right approach. First, caching GetShouldTxnSetSelection() is really odd. It may be prevented by editor at |DoTransaction()| but not so at |RedoTransaction()|. For example, when changing the style, a word may be selected, then, the selection may not be changed (I don't know actual our implementation, though). However, *before* redoing, the selection may be any ranges, therefore, |RedoTransaction()| clearly needs to set selection explicitly.

Additionally, there are no guarantee DOM tree is not modified without transactions. For example, JS can modify editor contents without transactions. Therefore, original selection range shouldn't be cached. I think that new selection after redo should be computed by itself.

I think that some transactions need to override RedoTransaction() and set selection explicitly after DoTransaction().
Priority: -- → P3

This seems to be working now, at least on macOS. Does it still reproduce for you Masayuki?

Flags: needinfo?(masayuki)
Whiteboard: [h2review-noted]

Oh, me too, but I have no idea which fix affected this.

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(masayuki)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: