Enabled functions of editing commands should check FrameSelection::SelectionHasFocus() when necessary |
||||||
Issue descriptionEditing command may or may not work when the selection doesn't have focus. For example: - JS-triggered commands always work even when the selection doesn't have focus - 'unselect', 'undo', 'redo' always work even when the selection doesn't have focus - User-triggered 'insertText', 'copy', 'cut', 'paste', ... shouldn't edit the selection when it doesn't have focus Hence, when necessary, the enabled functions of editing commands should take FrameSelection::SelectionHasFocus() into consideration
,
May 16 2017
This still doesn't fix it for the following commands: - copy - cut - paste - pasteAndMatchStyle - pasteGlobalSelection because they are set to "allow execution when disabled" yosin@: Could you check why these commands are allowed even when disabled? Thanks!
,
May 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e91966df2e6a57cafb584bfa72e19bc2e9bb620 commit 2e91966df2e6a57cafb584bfa72e19bc2e9bb620 Author: xiaochengh <xiaochengh@chromium.org> Date: Wed May 17 02:06:07 2017 Make EnabledInEditableText return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledInEditableText to fix the behavior of these commands: BackwardDelete DeleteBackward DeleteBackwardByDecomposingPreviousCharacter DeleteForward DeleteToBeginningOfLine DeleteToBeginningOfParagraph DeleteToEndOfLine DeleteToEndOfParagraph DeleteToMark DeleteWordBackward DeleteWordForward ForwardDelete IgnoreSpelling InsertBacktab InsertHTML InsertLineBreak InsertNewline InsertParagraph InsertTab InsertText MoveBackward MoveDown MoveForward MoveLeft MovePageDown MovePageUp MoveParagraphBackward MoveParagraphForward MoveRight MoveToBeginningOfDocument MoveToBeginningOfLine MoveToBeginningOfParagraph MoveToBeginningOfSentence MoveToEndOfDocument MoveToEndOfLine MoveToEndOfParagraph MoveToEndOfSentence MoveToLeftEndOfLine MoveToLeftEndOfLineAndModifySelection MoveToRightEndOfLine MoveToRightEndOfLineAndModifySelection MoveUp MoveWordBackward MoveWordForward MoveWordLeft MoveWordRight Yank YankAndSelect BUG= 713607 , 722925 TEST=editing/selection/arrow_key_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2886933002 Cr-Commit-Position: refs/heads/master@{#472284} [add] https://crrev.com/2e91966df2e6a57cafb584bfa72e19bc2e9bb620/third_party/WebKit/LayoutTests/editing/selection/arrow_key_with_unfocused_selection.html [modify] https://crrev.com/2e91966df2e6a57cafb584bfa72e19bc2e9bb620/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 17 2017
>This still doesn't fix it for the following commands:
>- copy
>- cut
>- paste
>- pasteAndMatchStyle
>- pasteGlobalSelection
>because they are set to "allow execution when disabled"
They are used for issuing clipboard event.
Note: These commands must be executed as response of user action for security reason
to prevent clipboard access.
It seems WebKit uses function for allowExecutionWhenDisabled()
1504 static bool allowExecutionWhenDisabledCopyCut(EditorCommandSource source)
1505 {
1506 switch (source) {
1507 case CommandFromMenuOrKeyBinding:
1508 return true;
1509 case CommandFromDOM:
1510 case CommandFromDOMWithUserInterface:
1511 return false;
1512 }
1513
1514 ASSERT_NOT_REACHED();
1515 return false;
1516 }
Note: I believe Blink does this in another place, before WebKit does so.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/editing/EditorCommand.cpp?annotate=blame
,
May 17 2017
Lower to Pri-2, usage of execCommand, except for "Past" is low. We care only about Arrow keys, as mentioned in issue 715889 .
,
May 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d93e3784b84a368cae59ba1b57dd4dfbe26ade24 commit d93e3784b84a368cae59ba1b57dd4dfbe26ade24 Author: xiaochengh <xiaochengh@chromium.org> Date: Wed May 17 04:04:51 2017 Introduce EnabledUnselect for 'unselect' command Among user-triggered editing commands using EnabledVisibleSelection, 'unselect' is the only one that should be enabled when the selection doesn't have focus. Hence, this patch adds EnabledUnselect as its enabled function, so that a follow up patch can disable the remaining commands when the selection doesn't have focus. BUG= 722925 TEST=n/a; no behavioral changes Review-Url: https://codereview.chromium.org/2891493003 Cr-Commit-Position: refs/heads/master@{#472315} [modify] https://crrev.com/d93e3784b84a368cae59ba1b57dd4dfbe26ade24/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb67b5730de46d4dbf550ca0496e27326b0a77b1 commit bb67b5730de46d4dbf550ca0496e27326b0a77b1 Author: xiaochengh <xiaochengh@chromium.org> Date: Wed May 17 05:15:36 2017 Make EnabledInRichlyEditableText return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledInRichlyEditableText to fix the behavior of these commands: AlignJustified AlignLeft AlignRight BackColor Bold CreateLink FontName FontSize FontSizeDelta ForeColor FormatBlock HiliteColor Indent InsertHorizontalRule InsertImage InsertNewlineInQuotedContent InsertOrderedList InsertUnorderedList Italic JustifyCenter JustifyFull JustifyLeft JustifyNone JustifyRight MakeTextWritingDirectionLeftToRight MakeTextWritingDirectionNatural MakeTextWritingDirectionRightToLeft Outdent OverWrite Strikethrough Subscript Superscript ToggleBold ToggleItalic ToggleUnderline Underline Unscript AlignCenter BUG= 722925 TEST=editing/style/apply_style_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2883383003 Cr-Commit-Position: refs/heads/master@{#472335} [add] https://crrev.com/bb67b5730de46d4dbf550ca0496e27326b0a77b1/third_party/WebKit/LayoutTests/editing/style/apply_style_with_unfocused_selection.html [modify] https://crrev.com/bb67b5730de46d4dbf550ca0496e27326b0a77b1/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9d6632785a0f1e94b4a15d3c2b3b54251162bc0 commit e9d6632785a0f1e94b4a15d3c2b3b54251162bc0 Author: xiaochengh <xiaochengh@chromium.org> Date: Wed May 17 07:20:31 2017 Make EnabledVisibleSelection return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledVisibleSelection() to fix the behavior of these commands: MoveBackwardAndModifySelection MoveDownAndModifySelection MoveForwardAndModifySelection MoveLeftAndModifySelection MovePageDownAndModifySelection MovePageUpAndModifySelection MoveParagraphBackwardAndModifySelection MoveParagraphForwardAndModifySelection MoveRightAndModifySelection MoveToBeginningOfDocumentAndModifySelection MoveToBeginningOfLineAndModifySelection MoveToBeginningOfParagraphAndModifySelection MoveToBeginningOfSentenceAndModifySelection MoveToEndOfDocumentAndModifySelection MoveToEndOfLineAndModifySelection MoveToEndOfParagraphAndModifySelection MoveToEndOfSentenceAndModifySelection MoveUpAndModifySelection MoveWordBackwardAndModifySelection MoveWordForwardAndModifySelection MoveWordLeftAndModifySelection MoveWordRightAndModifySelection SelectLine SelectParagraph SelectSentence SelectWord SetMark BUG= 722925 TEST=editing/selection/modify_extend/extend_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2886983002 Cr-Commit-Position: refs/heads/master@{#472366} [modify] https://crrev.com/e9d6632785a0f1e94b4a15d3c2b3b54251162bc0/third_party/WebKit/LayoutTests/editing/execCommand/script-tests/enabling-and-selection-2.js [add] https://crrev.com/e9d6632785a0f1e94b4a15d3c2b3b54251162bc0/third_party/WebKit/LayoutTests/editing/selection/modify_extend/extend_with_unfocused_selection.html [modify] https://crrev.com/e9d6632785a0f1e94b4a15d3c2b3b54251162bc0/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 18 2017
Note: Menu command, e.g. browser's [Edit] menu, is executed by following path: - Editor::Command::Execute() - Editor::Command::CreateCommand() w/ kCommandFromMenuOrKeyBinding, - WebLocalFrameImpl::ExecuteCommand()
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8aaa656f06ce8379a55bf81fa4cdca123ff43485 commit 8aaa656f06ce8379a55bf81fa4cdca123ff43485 Author: xiaochengh <xiaochengh@chromium.org> Date: Thu May 18 03:29:05 2017 Make EnabledRangeInEditableText return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledRangeInEditableText() to fix the behavior of the RemoveFormat command. BUG= 722925 TEST=editing/execCommand/remove_format_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2893713003 Cr-Commit-Position: refs/heads/master@{#472635} [add] https://crrev.com/8aaa656f06ce8379a55bf81fa4cdca123ff43485/third_party/WebKit/LayoutTests/editing/execCommand/remove_format_with_unfocused_selection.html [modify] https://crrev.com/8aaa656f06ce8379a55bf81fa4cdca123ff43485/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1eefca818771900ae2977d86952d42cb3b7fcb2 commit f1eefca818771900ae2977d86952d42cb3b7fcb2 Author: xiaochengh <xiaochengh@chromium.org> Date: Thu May 18 03:42:59 2017 Make EnabledDelete return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledDelete() to fix the behavior of the 'Delete' command. BUG= 722925 TEST=editing/deleting/delete_with_unfocsed_selection.html Review-Url: https://codereview.chromium.org/2887213002 Cr-Commit-Position: refs/heads/master@{#472644} [add] https://crrev.com/f1eefca818771900ae2977d86952d42cb3b7fcb2/third_party/WebKit/LayoutTests/editing/deleting/delete_with_unfocused_selection.html [modify] https://crrev.com/f1eefca818771900ae2977d86952d42cb3b7fcb2/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f462b4e66919d4104355e3883bf81b541f8754e commit 1f462b4e66919d4104355e3883bf81b541f8754e Author: xiaochengh <xiaochengh@chromium.org> Date: Thu May 18 04:45:00 2017 Make EnableCaretInEditableText return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledRangeInEditableText() to fix the behavior of the Transpose command. BUG= 722925 TEST=editing/execCommand/transpose_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2887213003 Cr-Commit-Position: refs/heads/master@{#472668} [add] https://crrev.com/1f462b4e66919d4104355e3883bf81b541f8754e/third_party/WebKit/LayoutTests/editing/execCommand/transpose_with_unfocused_selection.html [modify] https://crrev.com/1f462b4e66919d4104355e3883bf81b541f8754e/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2dde8c244c58dcf6d38d812ca064f911b1b0fe6 commit b2dde8c244c58dcf6d38d812ca064f911b1b0fe6 Author: xiaochengh <xiaochengh@chromium.org> Date: Thu May 18 06:04:40 2017 Make EnabledRangeInRichlyEditableText return false for user-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledRangeInEditableText() to fix the behavior of the Unlink command. BUG= 722925 TEST=editing/execCommand/unlink_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2889963003 Cr-Commit-Position: refs/heads/master@{#472696} [add] https://crrev.com/b2dde8c244c58dcf6d38d812ca064f911b1b0fe6/third_party/WebKit/LayoutTests/editing/execCommand/unlink_with_unfocused_selection.html [modify] https://crrev.com/b2dde8c244c58dcf6d38d812ca064f911b1b0fe6/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1 commit f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1 Author: xiaochengh <xiaochengh@chromium.org> Date: Fri May 19 00:12:15 2017 Fix behavior of clipboard commands when there is unfocused selection User-triggered clipboard commands should be disabled when selection doesn't have focus. Hence, this patch checks whether selection has focus in EnabledCopy/Cut/Paste. However, this is a quirk that allows clipboard commands to run even their enabled functions return false, so that clipboard events are still fired regardless of the enabled state. To work with this quirk, this patch also checks whether selection has focus in Editor::Copy/Cut/Paste/PasteAsPlainText, and if not, stops the command. BUG= 722925 TEST=editing/pasteboard/pasteboard_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2894473003 Cr-Commit-Position: refs/heads/master@{#472985} [add] https://crrev.com/f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1/third_party/WebKit/LayoutTests/editing/pasteboard/pasteboard_with_unfocused_selection.html [modify] https://crrev.com/f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1/third_party/WebKit/Source/core/editing/Editor.cpp [modify] https://crrev.com/f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1/third_party/WebKit/Source/core/editing/Editor.h [modify] https://crrev.com/f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp [modify] https://crrev.com/f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1/third_party/WebKit/Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd5cd151162312c124b976bc822bbd0e6f4f2d39 commit bd5cd151162312c124b976bc822bbd0e6f4f2d39 Author: xiaochengh <xiaochengh@chromium.org> Date: Fri May 19 05:58:34 2017 Make user-triggered SelectAll act as if there is no selection for hidden selection This patch ensures that a user-triggered SelectAll selects the entire document when there is a hidden selection. This is for consistency in user experience because a hidden selection appears as no selection to a user. Behavior of Javascript-triggered SelectAll remains the same. BUG= 722925 TEST=editing/selection/select_all/select_all_with_unfocused_selection.html Review-Url: https://codereview.chromium.org/2891203002 Cr-Commit-Position: refs/heads/master@{#473103} [add] https://crrev.com/bd5cd151162312c124b976bc822bbd0e6f4f2d39/third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_with_unfocused_selection.html [modify] https://crrev.com/bd5cd151162312c124b976bc822bbd0e6f4f2d39/third_party/WebKit/Source/core/editing/FrameSelection.cpp [modify] https://crrev.com/bd5cd151162312c124b976bc822bbd0e6f4f2d39/third_party/WebKit/Source/core/editing/FrameSelection.h [modify] https://crrev.com/bd5cd151162312c124b976bc822bbd0e6f4f2d39/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e08fc6e66a78c7db92ec9769da85b4dde4dd772e commit e08fc6e66a78c7db92ec9769da85b4dde4dd772e Author: xiaochengh <xiaochengh@chromium.org> Date: Fri May 19 21:39:34 2017 Fix a layout test case description TBR=yosin@chromium.org BUG= 722925 Review-Url: https://codereview.chromium.org/2892233002 Cr-Commit-Position: refs/heads/master@{#473335} [modify] https://crrev.com/e08fc6e66a78c7db92ec9769da85b4dde4dd772e/third_party/WebKit/LayoutTests/editing/selection/select_all/select_all_with_unfocused_selection.html
,
May 23 2017
Only two commands are not covered yet, both of which have very low usage: SelectToMark SwapWithMark
,
May 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5172eedc999c1828d7051d3a9c8a210d8370f0ad commit 5172eedc999c1828d7051d3a9c8a210d8370f0ad Author: Xiaocheng Hu <xiaochengh@chromium.org> Date: Thu May 25 06:14:31 2017 Make EnabledVisibleSelectionAndMark return false for use-triggered command and unfocused selection Certain editing commands shouldn't be enabled when they are user-triggered and the selection doesn't have focus. This patch adds checking of FrameSelection::SelectionHasFocus() in EnabledVisibleSelectionAndMark() to fix the behavior of the following commands: - SelectToMark - SwapWithMark Bug: 722925 Change-Id: Ib5e2c9fde9fec1f73421166d04d40daeecfd46d6 Reviewed-on: https://chromium-review.googlesource.com/514164 Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#474581} [add] https://crrev.com/5172eedc999c1828d7051d3a9c8a210d8370f0ad/third_party/WebKit/LayoutTests/editing/selection/mark_with_unfocused_selection.html [modify] https://crrev.com/5172eedc999c1828d7051d3a9c8a210d8370f0ad/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp
,
May 25 2017
,
Aug 1 2017
Disabling copy/cut actions for unfocused selections broke the clipboard management in our app for Chrome 60, and made this article no longer valid: https://developers.google.com/web/updates/2015/04/cut-and-copy-commands In fact, does this make document.execCommand('copy') unusable (except for the case you'd prevent the selection from blurring when pressing the UI control to trigger the copy command)?
,
Aug 1 2017
danburzo@: Could you file a new issue with a link to your app and repro steps of broken functionality? JS triggered operations (document.execCommand) are unaffected. Copy/Cut/Paste are disabled only when they are directly triggered by the user (via Ctrl+C/X/V, context menu, etc). The content of the article you gave is still valid.
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef6a22fba8fcf2ab5f17e4ac6cad171efb7b6700 commit ef6a22fba8fcf2ab5f17e4ac6cad171efb7b6700 Author: Alexandre Elias <aelias@chromium.org> Date: Thu Aug 10 00:13:41 2017 Revert "Fix behavior of clipboard commands when there is unfocused selection" on M60. This reverts commit f8ff4f535ca35569ff840d9ffb71bb3e9e5d3ee1 on M60 branch. Causes Android copy/paste regression http://crbug.com/741968 only on M60. BUG= 741968 , 722925 TBR=xiaochengh@chromium.org TEST=Long-press on links, focus on textboxes, then select "normal" text outside textboxes and try to copy/paste it. Change-Id: I48a6a80e56cb4e5bb7ea69564303a4beab43741a Reviewed-on: https://chromium-review.googlesource.com/609375 Reviewed-by: Alexandre Elias <aelias@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#718} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [delete] https://crrev.com/35e343db69f214e4ad8f741febf8c2dccc54fd10/third_party/WebKit/LayoutTests/editing/pasteboard/pasteboard_with_unfocused_selection.html [modify] https://crrev.com/ef6a22fba8fcf2ab5f17e4ac6cad171efb7b6700/third_party/WebKit/Source/core/editing/Editor.cpp [modify] https://crrev.com/ef6a22fba8fcf2ab5f17e4ac6cad171efb7b6700/third_party/WebKit/Source/core/editing/Editor.h [modify] https://crrev.com/ef6a22fba8fcf2ab5f17e4ac6cad171efb7b6700/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp [modify] https://crrev.com/ef6a22fba8fcf2ab5f17e4ac6cad171efb7b6700/third_party/WebKit/Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp
,
Aug 12 2017
Thanks for the reply. We have managed to work with the current implementation on our web application by altering our code. However, I have logged a simplified test case based on the article I linked to in the previous comment, exhibiting what seems to (but may not) be focus-dependent execCommand('copy') behavior: https://bugs.chromium.org/p/chromium/issues/detail?id=754965
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by xiaoche...@chromium.org
, May 16 2017