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)
Core
DOM: Editor
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
firefox55 | --- | affected |
People
(Reporter: masayuki, Unassigned)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [h2review-noted])
Attachments
(2 files)
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.
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
Thanks, I just think that redoing in this case should select the range which has styled.
Comment 3•7 years ago
|
||
True, in this case it should re-select the whole thing.
Comment 4•7 years ago
|
||
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)
Reporter | ||
Comment 5•7 years ago
|
||
I just guessed that each transaction's Redo() should do that. http://searchfox.org/mozilla-central/rev/990055a4902952e038cc350c9ffe1ac426d1c943/editor/libeditor/CreateElementTransaction.cpp#122 http://searchfox.org/mozilla-central/rev/990055a4902952e038cc350c9ffe1ac426d1c943/editor/libeditor/ChangeStyleTransaction.cpp#247
Flags: needinfo?(masayuki)
Comment 6•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
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?
Reporter | ||
Comment 10•7 years ago
|
||
(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)
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-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().
Updated•7 years ago
|
Priority: -- → P3
Comment 13•4 years ago
|
||
This seems to be working now, at least on macOS. Does it still reproduce for you Masayuki?
Flags: needinfo?(masayuki)
Whiteboard: [h2review-noted]
Reporter | ||
Comment 14•4 years ago
|
||
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.
Description
•