Ignore text transform for plain text copy [chromium/src : main]

1 view
Skip to first unread message

Siye Liu (Gerrit)

unread,
Mar 27, 2024, 12:14:53 PMMar 27
to Sambamurthy Bandaru, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar and Sambamurthy Bandaru

Siye Liu added 7 comments

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Siye Liu . resolved

Thank you!

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1 (Latest):String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . unresolved

Do we also need to set `ignores_css_transforms` to `true` here?

File third_party/blink/renderer/core/editing/iterators/text_iterator_behavior.cc
Line 136, Patchset 1 (Latest):TextIteratorBehavior::Builder::SetIgnoresCssTransforms(bool value) {
Siye Liu . unresolved

Please add some test coverage for this new behavior.

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
Line 66, Patchset 1 (Latest): if (behavior.EmitsOriginalText() || behavior.IgnoresCssTransforms()) {
Siye Liu . unresolved

Can you add the code flow in the description from `FrameSelection::SelectedTextForClipboard` to here? It would be easier to relate copy text with this function.

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1 (Latest): TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Same here. Can you add the code flow in the description from `FrameSelection::SelectedHTMLForClipboard` to here? It would be easier to relate copy html with this function.

Line 233, Patchset 1 (Latest): TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

There are a few unit test failures in the CQ dry run. Please address those failures.

Line 233, Patchset 1 (Latest): TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sambamurthy Bandaru
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 1
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Comment-Date: Wed, 27 Mar 2024 16:14:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Sanket Joshi (Gerrit)

unread,
Apr 2, 2024, 12:23:31 AMApr 2
to Sambamurthy Bandaru, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar and Sambamurthy Bandaru

Sanket Joshi added 1 comment

Patchset-level comments
Sanket Joshi . unresolved

Given that this changing the behavior of plain text copy, this would be considered a web-facing change and we should send a web-facing change PSA to blink-dev after this CL lands.
https://www.chromium.org/blink/launching-features/#change-to-existing-code

In addition, we should make this change behind a feature flag. In this case there is an unexpected regression, the feature flag can simply be turned off (remotely), which is much faster than a code change.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sambamurthy Bandaru
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 1
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Comment-Date: Tue, 02 Apr 2024 04:23:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Sanket Joshi (Gerrit)

unread,
Apr 2, 2024, 12:27:16 AMApr 2
to Sambamurthy Bandaru, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar and Sambamurthy Bandaru

Sanket Joshi added 1 comment

Patchset-level comments
Sanket Joshi . unresolved

Given that this is changing the behavior of plain text copy, this would be considered a web-facing change and we should send a web-facing change PSA to blink-dev after this CL lands.
https://www.chromium.org/blink/launching-features/#change-to-existing-code

In addition, we should make this change behind a feature flag. In case there is an unexpected regression, the feature flag can simply be turned off (remotely), which is much faster than a code change.
https://chromium.googlesource.com/chromium/src/+/HEAD/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md#overview

Gerrit-Comment-Date: Tue, 02 Apr 2024 04:27:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Dominique Fauteux-Chapleau (Gerrit)

unread,
Apr 19, 2024, 9:18:48 AMApr 19
to Sambamurthy Bandaru, dpr...@google.com, AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar

Dominique Fauteux-Chapleau removed dpr...@google.com from this change

Related details

Attention is currently required from:
  • Rakesh Goulikar
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: deleteReviewer
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 6
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Sambamurthy Bandaru (Gerrit)

unread,
Apr 19, 2024, 1:59:41 PMApr 19
to AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Siye Liu

Sambamurthy Bandaru added 8 comments

Patchset-level comments
File-level comment, Patchset 1:
Sanket Joshi . unresolved

Given that this changing the behavior of plain text copy, this would be considered a web-facing change and we should send a web-facing change PSA to blink-dev after this CL lands.
https://www.chromium.org/blink/launching-features/#change-to-existing-code

In addition, we should make this change behind a feature flag. In this case there is an unexpected regression, the feature flag can simply be turned off (remotely), which is much faster than a code change.

Sambamurthy Bandaru

Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'

Sanket Joshi . unresolved

Given that this is changing the behavior of plain text copy, this would be considered a web-facing change and we should send a web-facing change PSA to blink-dev after this CL lands.
https://www.chromium.org/blink/launching-features/#change-to-existing-code

In addition, we should make this change behind a feature flag. In case there is an unexpected regression, the feature flag can simply be turned off (remotely), which is much faster than a code change.
https://chromium.googlesource.com/chromium/src/+/HEAD/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md#overview

Sambamurthy Bandaru

Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1:String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . unresolved

Do we also need to set `ignores_css_transforms` to `true` here?

Sambamurthy Bandaru

`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.

File third_party/blink/renderer/core/editing/iterators/text_iterator_behavior.cc
Line 136, Patchset 1:TextIteratorBehavior::Builder::SetIgnoresCssTransforms(bool value) {
Siye Liu . unresolved

Please add some test coverage for this new behavior.

Sambamurthy Bandaru

Added web tests

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
Line 66, Patchset 1: if (behavior.EmitsOriginalText() || behavior.IgnoresCssTransforms()) {
Siye Liu . resolved

Can you add the code flow in the description from `FrameSelection::SelectedTextForClipboard` to here? It would be easier to relate copy text with this function.

Sambamurthy Bandaru

Acknowledged

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Same here. Can you add the code flow in the description from `FrameSelection::SelectedHTMLForClipboard` to here? It would be easier to relate copy html with this function.

Sambamurthy Bandaru

Updated commit description with code flow

Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Sambamurthy Bandaru

LayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.

Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . resolved

There are a few unit test failures in the CQ dry run. Please address those failures.

Sambamurthy Bandaru

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 6
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Fri, 19 Apr 2024 17:59:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sanket Joshi <sa...@microsoft.com>
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 19, 2024, 5:06:11 PMApr 19
to Sambamurthy Bandaru, AyeAye, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sambamurthy Bandaru and Sanket Joshi

Siye Liu added 5 comments

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1:String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . unresolved

Do we also need to set `ignores_css_transforms` to `true` here?

Sambamurthy Bandaru

`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.

Siye Liu

Since we set `ignores_css_transforms` to `true` in |StyledMarkupAccumulator::RenderedText|, we need to make sure that all the callers of this method expect the behavior (ignore css transform).

Can you check if it's the case?

File third_party/blink/renderer/core/editing/iterators/text_iterator_behavior.cc
Line 136, Patchset 1:TextIteratorBehavior::Builder::SetIgnoresCssTransforms(bool value) {
Siye Liu . resolved

Please add some test coverage for this new behavior.

Sambamurthy Bandaru

Added web tests

Siye Liu

Done

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Sambamurthy Bandaru

LayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.

Siye Liu

What I mean is that after our change, `StyledMarkupAccumulator::RenderedText` will return result that ignores css transforms property. Have you verified that all the callers of `StyledMarkupAccumulator::RenderedText` expect this behavior?

Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . resolved

Same here. Can you add the code flow in the description from `FrameSelection::SelectedHTMLForClipboard` to here? It would be easier to relate copy html with this function.

Sambamurthy Bandaru

Updated commit description with code flow

Siye Liu

Done

File third_party/blink/web_tests/editing/pasteboard/paste_text_with_text_transform_applied.html
Line 11, Patchset 7 (Latest): internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;
Siye Liu . unresolved

I think we may not need this case because the runtime feature flag is `true` by default.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sambamurthy Bandaru
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 7
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Comment-Date: Fri, 19 Apr 2024 21:06:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Sambamurthy Bandaru (Gerrit)

unread,
Apr 22, 2024, 4:32:59 AMApr 22
to AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Siye Liu

Sambamurthy Bandaru added 3 comments

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1:String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . unresolved

Do we also need to set `ignores_css_transforms` to `true` here?

Sambamurthy Bandaru

`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.

Siye Liu

Since we set `ignores_css_transforms` to `true` in |StyledMarkupAccumulator::RenderedText|, we need to make sure that all the callers of this method expect the behavior (ignore css transform).

Can you check if it's the case?

Sambamurthy Bandaru

Yes, that looks safe. SelectedHTMLForClipboard used only for copy, text drag and print renderer. And the selected markup's style still has the text-transform.

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Sambamurthy Bandaru

LayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.

Siye Liu

What I mean is that after our change, `StyledMarkupAccumulator::RenderedText` will return result that ignores css transforms property. Have you verified that all the callers of `StyledMarkupAccumulator::RenderedText` expect this behavior?

Sambamurthy Bandaru

Text iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true

File third_party/blink/web_tests/editing/pasteboard/paste_text_with_text_transform_applied.html
Line 11, Patchset 7 (Latest): internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;
Siye Liu . unresolved

I think we may not need this case because the runtime feature flag is `true` by default.

Sambamurthy Bandaru

Thought we may need this in case we disable the runtime feature flag

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 7
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 08:32:42 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 22, 2024, 1:09:56 PMApr 22
to Sambamurthy Bandaru, AyeAye, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sambamurthy Bandaru and Sanket Joshi

Siye Liu added 3 comments

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1:String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . unresolved

Do we also need to set `ignores_css_transforms` to `true` here?

Sambamurthy Bandaru

`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.

Siye Liu

Since we set `ignores_css_transforms` to `true` in |StyledMarkupAccumulator::RenderedText|, we need to make sure that all the callers of this method expect the behavior (ignore css transform).

Can you check if it's the case?

Sambamurthy Bandaru

Yes, that looks safe. SelectedHTMLForClipboard used only for copy, text drag and print renderer. And the selected markup's style still has the text-transform.

Siye Liu

Even though |StyledMarkupAccumulator::RenderedText| is only used in |FrameSelection::SelectedHTMLForClipboard|, we should still follow the pattern to set `ignores_css_transforms` as an option in `CreateMarkupOptions`(add `.SetIgnoresCssTransforms(true)` after line 1051).

It's clearer that `FrameSelection::SelectedHTMLForClipboard` expects result without css transform. It's also safer because the default result produced by |StyledMarkupAccumulator::RenderedText| remain the same as today.

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Sambamurthy Bandaru

LayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.

Siye Liu

What I mean is that after our change, `StyledMarkupAccumulator::RenderedText` will return result that ignores css transforms property. Have you verified that all the callers of `StyledMarkupAccumulator::RenderedText` expect this behavior?

Sambamurthy Bandaru

Text iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true

Siye Liu

I see. I still think we should pass in the `ignores_css_transforms` as an option instead of setting it directly to `true`.

Another option is to rename the method something like `RenderedTextWithoutCSSTransforms` so that it's clearer and self-explanatory.

File third_party/blink/web_tests/editing/pasteboard/paste_text_with_text_transform_applied.html
Line 11, Patchset 7 (Latest): internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;
Siye Liu . unresolved

I think we may not need this case because the runtime feature flag is `true` by default.

Sambamurthy Bandaru

Thought we may need this in case we disable the runtime feature flag

Siye Liu

Good consideration. It's okay to only have the default runtime feature flag case since it's turned on by default.

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sambamurthy Bandaru
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 7
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 17:09:44 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Sambamurthy Bandaru (Gerrit)

unread,
Apr 23, 2024, 5:49:15 AMApr 23
to AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sanket Joshi and Siye Liu

Sambamurthy Bandaru added 3 comments

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1:String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . unresolved

Do we also need to set `ignores_css_transforms` to `true` here?

Sambamurthy Bandaru

`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.

Siye Liu

Since we set `ignores_css_transforms` to `true` in |StyledMarkupAccumulator::RenderedText|, we need to make sure that all the callers of this method expect the behavior (ignore css transform).

Can you check if it's the case?

Sambamurthy Bandaru

Yes, that looks safe. SelectedHTMLForClipboard used only for copy, text drag and print renderer. And the selected markup's style still has the text-transform.

Siye Liu

Even though |StyledMarkupAccumulator::RenderedText| is only used in |FrameSelection::SelectedHTMLForClipboard|, we should still follow the pattern to set `ignores_css_transforms` as an option in `CreateMarkupOptions`(add `.SetIgnoresCssTransforms(true)` after line 1051).

It's clearer that `FrameSelection::SelectedHTMLForClipboard` expects result without css transform. It's also safer because the default result produced by |StyledMarkupAccumulator::RenderedText| remain the same as today.

Sambamurthy Bandaru

Agreed. Added a flag 'SetRenderTextWithoutCSSTransforms' in CreateMarkupOptions. Named it that way because the markup will still have text-transform in Style.

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . unresolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Sambamurthy Bandaru

LayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.

Siye Liu

What I mean is that after our change, `StyledMarkupAccumulator::RenderedText` will return result that ignores css transforms property. Have you verified that all the callers of `StyledMarkupAccumulator::RenderedText` expect this behavior?

Sambamurthy Bandaru

Text iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true

Siye Liu

I see. I still think we should pass in the `ignores_css_transforms` as an option instead of setting it directly to `true`.

Another option is to rename the method something like `RenderedTextWithoutCSSTransforms` so that it's clearer and self-explanatory.

Sambamurthy Bandaru

Done. Added a flag in CreateMarkupOptions and this is followed from there.
See latest patchset

File third_party/blink/web_tests/editing/pasteboard/paste_text_with_text_transform_applied.html
Line 11, Patchset 7: internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;
Siye Liu . unresolved

I think we may not need this case because the runtime feature flag is `true` by default.

Sambamurthy Bandaru

Thought we may need this in case we disable the runtime feature flag

Siye Liu

Good consideration. It's okay to only have the default runtime feature flag case since it's turned on by default.

Sambamurthy Bandaru

Removed

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 8
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Tue, 23 Apr 2024 09:49:01 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Prashant Nevase (Gerrit)

unread,
Apr 23, 2024, 10:24:34 AMApr 23
to Sambamurthy Bandaru, AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Rakesh Goulikar, Sambamurthy Bandaru, Sanket Joshi and Siye Liu

Prashant Nevase added 1 comment

Commit Message
Line 14, Patchset 8 (Latest):FrameSelection::SelectedHTMLForClipboard()
  |
  +-- Serialisation.cc -> CreateMarkupAlgorithm<Strategy>::CreateMarkup()
      |
      +-- StyledMarkupSerializer<Strategy>::CreateMarkup()
          |
          +-- StyledMarkupTraverser<Strategy>::Traverse(Node* start_node, Node* past_end)
              |
              +-- StyledMarkupTraverser<Strategy>::AppendStartMarkup(Node& node)
                  |
                  +-- StyledMarkupAccumulator::AppendTextWithInlineStyle(Text& text, EditingStyle* inline_style)
                      |
                      +-- StyledMarkupAccumulator::RenderedText(Text& text_node)
                          |
                          +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)

FrameSelection::SelectedTextForClipboard()
  |
  +-- FrameSelection::ExtractSelectedText(const FrameSelection& selection, TextIteratorBehavior behavior)
      |
      +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)
Prashant Nevase . unresolved

With this cl description looks unnecessarily lengthy. Is it possible to summarize this?

Open in Gerrit

Related details

Attention is currently required from:
  • Rakesh Goulikar
  • Sambamurthy Bandaru
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 8
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Prashant Nevase <pne...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Tue, 23 Apr 2024 14:24:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Sanket Joshi (Gerrit)

unread,
Apr 23, 2024, 5:00:48 PMApr 23
to Sambamurthy Bandaru, Prashant Nevase, AyeAye, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Prashant Nevase, Rakesh Goulikar, Sambamurthy Bandaru and Siye Liu

Sanket Joshi added 3 comments

Commit Message
Line 14, Patchset 8 (Latest):FrameSelection::SelectedHTMLForClipboard()
  |
  +-- Serialisation.cc -> CreateMarkupAlgorithm<Strategy>::CreateMarkup()
      |
      +-- StyledMarkupSerializer<Strategy>::CreateMarkup()
          |
          +-- StyledMarkupTraverser<Strategy>::Traverse(Node* start_node, Node* past_end)
              |
              +-- StyledMarkupTraverser<Strategy>::AppendStartMarkup(Node& node)
                  |
                  +-- StyledMarkupAccumulator::AppendTextWithInlineStyle(Text& text, EditingStyle* inline_style)
                      |
                      +-- StyledMarkupAccumulator::RenderedText(Text& text_node)
                          |
                          +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)

FrameSelection::SelectedTextForClipboard()
  |
  +-- FrameSelection::ExtractSelectedText(const FrameSelection& selection, TextIteratorBehavior behavior)
      |
      +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)
Prashant Nevase . unresolved

With this cl description looks unnecessarily lengthy. Is it possible to summarize this?

Sanket Joshi

+1

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1052, Patchset 8 (Latest): .SetRenderTextWithoutCSSTransforms(true)
Sanket Joshi . unresolved

Shouldn't this change also be made behind the feature flag?

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
Line 65, Patchset 8 (Latest): const LayoutObject& layout_object = unit.GetLayoutObject();
// This is ensured because |unit.GetLayoutObject()| must be the
// LayoutObject for TextIteratorTextNodeHandler's |text_node_|.
DCHECK(IsA<LayoutText>(layout_object));

bool isSecure =
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;
// Gets underlying text from node which is not desired
// when the text security is set
if (!isSecure && behavior.IgnoresCSSTransforms()) {
String text =
To<LayoutText>(layout_object)
.OriginalText()
.Substring(unit.DOMStart(), unit.DOMEnd() - unit.DOMStart());
if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
text.Replace(kNewlineCharacter, kSpaceCharacter);
}
result.string = text;
result.start = 0;
result.end = result.string.length();
}

if (behavior.EmitsOriginalText()) {
result.string =
To<LayoutText>(layout_object)
.OriginalText()
Sanket Joshi . unresolved

Shouldn't these changes also be made behind the feature flag?

Open in Gerrit

Related details

Attention is currently required from:
  • Prashant Nevase
  • Rakesh Goulikar
  • Sambamurthy Bandaru
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 8
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Prashant Nevase <pne...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Attention: Prashant Nevase <pne...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Tue, 23 Apr 2024 21:00:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Prashant Nevase <pne...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Sambamurthy Bandaru (Gerrit)

unread,
Apr 24, 2024, 12:21:03 AMApr 24
to Prashant Nevase, AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Prashant Nevase, Rakesh Goulikar, Sanket Joshi and Siye Liu

Sambamurthy Bandaru added 7 comments

Patchset-level comments
File-level comment, Patchset 1:
Sanket Joshi . resolved

Given that this is changing the behavior of plain text copy, this would be considered a web-facing change and we should send a web-facing change PSA to blink-dev after this CL lands.
https://www.chromium.org/blink/launching-features/#change-to-existing-code

In addition, we should make this change behind a feature flag. In case there is an unexpected regression, the feature flag can simply be turned off (remotely), which is much faster than a code change.
https://chromium.googlesource.com/chromium/src/+/HEAD/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md#overview

Sambamurthy Bandaru

Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'

Sambamurthy Bandaru

Done

File-level comment, Patchset 1:
Sanket Joshi . resolved

Given that this changing the behavior of plain text copy, this would be considered a web-facing change and we should send a web-facing change PSA to blink-dev after this CL lands.
https://www.chromium.org/blink/launching-features/#change-to-existing-code

In addition, we should make this change behind a feature flag. In this case there is an unexpected regression, the feature flag can simply be turned off (remotely), which is much faster than a code change.

Sambamurthy Bandaru

Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'

Sambamurthy Bandaru

Done

Commit Message
Line 14, Patchset 8:FrameSelection::SelectedHTMLForClipboard()

  |
  +-- Serialisation.cc -> CreateMarkupAlgorithm<Strategy>::CreateMarkup()
      |
      +-- StyledMarkupSerializer<Strategy>::CreateMarkup()
          |
          +-- StyledMarkupTraverser<Strategy>::Traverse(Node* start_node, Node* past_end)
              |
              +-- StyledMarkupTraverser<Strategy>::AppendStartMarkup(Node& node)
                  |
                  +-- StyledMarkupAccumulator::AppendTextWithInlineStyle(Text& text, EditingStyle* inline_style)
                      |
                      +-- StyledMarkupAccumulator::RenderedText(Text& text_node)
                          |
                          +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)

FrameSelection::SelectedTextForClipboard()
  |
  +-- FrameSelection::ExtractSelectedText(const FrameSelection& selection, TextIteratorBehavior behavior)
      |
      +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)
Prashant Nevase . unresolved

With this cl description looks unnecessarily lengthy. Is it possible to summarize this?

Sanket Joshi

+1

Sambamurthy Bandaru

Updated description

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1045, Patchset 1:String FrameSelection::SelectedHTMLForClipboard() const {
Siye Liu . resolved

Do we also need to set `ignores_css_transforms` to `true` here?

Sambamurthy Bandaru

`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.

Siye Liu

Since we set `ignores_css_transforms` to `true` in |StyledMarkupAccumulator::RenderedText|, we need to make sure that all the callers of this method expect the behavior (ignore css transform).

Can you check if it's the case?

Sambamurthy Bandaru

Yes, that looks safe. SelectedHTMLForClipboard used only for copy, text drag and print renderer. And the selected markup's style still has the text-transform.

Siye Liu

Even though |StyledMarkupAccumulator::RenderedText| is only used in |FrameSelection::SelectedHTMLForClipboard|, we should still follow the pattern to set `ignores_css_transforms` as an option in `CreateMarkupOptions`(add `.SetIgnoresCssTransforms(true)` after line 1051).

It's clearer that `FrameSelection::SelectedHTMLForClipboard` expects result without css transform. It's also safer because the default result produced by |StyledMarkupAccumulator::RenderedText| remain the same as today.

Sambamurthy Bandaru

Agreed. Added a flag 'SetRenderTextWithoutCSSTransforms' in CreateMarkupOptions. Named it that way because the markup will still have text-transform in Style.

Sambamurthy Bandaru

Done

Line 1052, Patchset 8: .SetRenderTextWithoutCSSTransforms(true)
Sanket Joshi . unresolved

Shouldn't this change also be made behind the feature flag?

Sambamurthy Bandaru

The options are used in RenderedText() method in styled accumulator. There it was behind the feature flag

File third_party/blink/renderer/core/editing/serializers/styled_markup_accumulator.cc
Line 233, Patchset 1: TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());
Siye Liu . resolved

Are there any other callers of this function that want the CSS transformed text? In that case should we add a param to set `ignores_css_transforms` property? We can default the param to `false`.

Sambamurthy Bandaru

LayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.

Siye Liu

What I mean is that after our change, `StyledMarkupAccumulator::RenderedText` will return result that ignores css transforms property. Have you verified that all the callers of `StyledMarkupAccumulator::RenderedText` expect this behavior?

Sambamurthy Bandaru

Text iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true

Siye Liu

I see. I still think we should pass in the `ignores_css_transforms` as an option instead of setting it directly to `true`.

Another option is to rename the method something like `RenderedTextWithoutCSSTransforms` so that it's clearer and self-explanatory.

Sambamurthy Bandaru

Done. Added a flag in CreateMarkupOptions and this is followed from there.
See latest patchset

Sambamurthy Bandaru

Done

File third_party/blink/web_tests/editing/pasteboard/paste_text_with_text_transform_applied.html
Line 11, Patchset 7: internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;
Siye Liu . resolved

I think we may not need this case because the runtime feature flag is `true` by default.

Sambamurthy Bandaru

Thought we may need this in case we disable the runtime feature flag

Siye Liu

Good consideration. It's okay to only have the default runtime feature flag case since it's turned on by default.

Sambamurthy Bandaru

Removed

Sambamurthy Bandaru

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Prashant Nevase
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 9
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Prashant Nevase <pne...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Prashant Nevase <pne...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 04:20:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sanket Joshi <sa...@microsoft.com>
Comment-In-Reply-To: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Comment-In-Reply-To: Prashant Nevase <pne...@microsoft.com>
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Sambamurthy Bandaru (Gerrit)

unread,
Apr 24, 2024, 12:23:09 AMApr 24
to Prashant Nevase, AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Prashant Nevase, Rakesh Goulikar, Sanket Joshi and Siye Liu

Sambamurthy Bandaru added 3 comments

Commit Message
Line 14, Patchset 8:FrameSelection::SelectedHTMLForClipboard()
  |
  +-- Serialisation.cc -> CreateMarkupAlgorithm<Strategy>::CreateMarkup()
      |
      +-- StyledMarkupSerializer<Strategy>::CreateMarkup()
          |
          +-- StyledMarkupTraverser<Strategy>::Traverse(Node* start_node, Node* past_end)
              |
              +-- StyledMarkupTraverser<Strategy>::AppendStartMarkup(Node& node)
                  |
                  +-- StyledMarkupAccumulator::AppendTextWithInlineStyle(Text& text, EditingStyle* inline_style)
                      |
                      +-- StyledMarkupAccumulator::RenderedText(Text& text_node)
                          |
                          +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)

FrameSelection::SelectedTextForClipboard()
  |
  +-- FrameSelection::ExtractSelectedText(const FrameSelection& selection, TextIteratorBehavior behavior)
      |
      +-- TextIterator::PlainText(const EphemeralRange& range, const TextIteratorBehavior& behavior)
Prashant Nevase . resolved

With this cl description looks unnecessarily lengthy. Is it possible to summarize this?

Sanket Joshi

+1

Sambamurthy Bandaru

Updated description

Sambamurthy Bandaru

Done

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1052, Patchset 8: .SetRenderTextWithoutCSSTransforms(true)
Sanket Joshi . resolved

Shouldn't this change also be made behind the feature flag?

Sambamurthy Bandaru

The options are used in RenderedText() method in styled accumulator. There it was behind the feature flag

Sambamurthy Bandaru

Done

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
Line 65, Patchset 8: const LayoutObject& layout_object = unit.GetLayoutObject();

// This is ensured because |unit.GetLayoutObject()| must be the
// LayoutObject for TextIteratorTextNodeHandler's |text_node_|.
DCHECK(IsA<LayoutText>(layout_object));

bool isSecure =
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;
// Gets underlying text from node which is not desired
// when the text security is set
if (!isSecure && behavior.IgnoresCSSTransforms()) {
String text =
To<LayoutText>(layout_object)
.OriginalText()
.Substring(unit.DOMStart(), unit.DOMEnd() - unit.DOMStart());
if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
text.Replace(kNewlineCharacter, kSpaceCharacter);
}
result.string = text;
result.start = 0;
result.end = result.string.length();
}

if (behavior.EmitsOriginalText()) {
result.string =
To<LayoutText>(layout_object)
.OriginalText()
Sanket Joshi . resolved

Shouldn't these changes also be made behind the feature flag?

Sambamurthy Bandaru

behavior.SetIgnoresCSSTransforms() is behind feature flag

Open in Gerrit

Related details

Attention is currently required from:
  • Prashant Nevase
  • Rakesh Goulikar
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
Gerrit-Change-Number: 5399744
Gerrit-PatchSet: 9
Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Prashant Nevase <pne...@microsoft.com>
Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Prashant Nevase <pne...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 04:22:59 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 24, 2024, 11:30:10 AMApr 24
to Sambamurthy Bandaru, Prashant Nevase, AyeAye, Sanket Joshi, Rakesh Goulikar, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
Attention needed from Prashant Nevase, Rakesh Goulikar, Sambamurthy Bandaru and Sanket Joshi

Siye Liu added 4 comments

File third_party/blink/renderer/core/editing/frame_selection.cc
Line 1068, Patchset 9 (Latest): builder.SetIgnoresCSSTransforms(true);
Siye Liu . unresolved

Nit: `builder.SetIgnoresCSSTransforms(RuntimeEnabledFeatures::IgnoresCSSTransformsForPlainTextCopyEnabled());`

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
Line 65, Patchset 9 (Latest): const LayoutObject& layout_object = unit.GetLayoutObject();

// This is ensured because |unit.GetLayoutObject()| must be the
// LayoutObject for TextIteratorTextNodeHandler's |text_node_|.
DCHECK(IsA<LayoutText>(layout_object));

bool isSecure =
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;
// Gets underlying text from node which is not desired
// when the text security is set
if (!isSecure && behavior.IgnoresCSSTransforms()) {
String text =
To<LayoutText>(layout_object)
.OriginalText()
.Substring(unit.DOMStart(), unit.DOMEnd() - unit.DOMStart());
if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
text.Replace(kNewlineCharacter, kSpaceCharacter);
}
result.string = text;
result.start = 0;
result.end = result.string.length();
}
Siye Liu . unresolved

Can we put this in a helper method? This adjustment is for IgnoresCSSTransforms case only. Also it might be good to add comment on the helper method about why this logic is needed.

File third_party/blink/web_tests/editing/pasteboard/copy-backslash-with-euc.html
Line 64, Patchset 9 (Latest): else if (fromElementName == "transform" && !(toElementNameSuffix == "text-control"))
Siye Liu . unresolved

Why do we need the extra logic? Can you add some comments?

File third_party/blink/web_tests/editing/pasteboard/paste_text_with_text_transform.html
Line 11, Patchset 9 (Latest): internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = true;
Siye Liu . unresolved

Nit: no need to explicitly set the flag. The runtime feature flag is enabled by default.

Open in Gerrit

Related details

Attention is currently required from:
  • Prashant Nevase
  • Rakesh Goulikar
  • Sambamurthy Bandaru
  • Sanket Joshi
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
    Gerrit-Change-Number: 5399744
    Gerrit-PatchSet: 9
    Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
    Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
    Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
    Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
    Gerrit-CC: Prashant Nevase <pne...@microsoft.com>
    Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
    Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
    Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
    Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
    Gerrit-Attention: Prashant Nevase <pne...@microsoft.com>
    Gerrit-Comment-Date: Wed, 24 Apr 2024 15:30:02 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Sanket Joshi (Gerrit)

    unread,
    Apr 25, 2024, 5:14:15 PMApr 25
    to Sambamurthy Bandaru, Prashant Nevase, AyeAye, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
    Attention needed from Prashant Nevase, Rakesh Goulikar and Sambamurthy Bandaru

    Sanket Joshi added 8 comments

    Patchset-level comments
    File-level comment, Patchset 9 (Latest):
    Sanket Joshi . unresolved

    Would suggest updating the naming everywhere in this CL to "CSSTextTransform" instead of "CSSTransform". CSS transforms are different: https://developer.mozilla.org/en-US/docs/Web/CSS/transform.

    Commit Message
    Line 13, Patchset 9 (Latest):SelectedTextForClipBoard() gets text from text iterator.
    Sanket Joshi . unresolved

    nit: |SelectedTextForClipboard|

    Line 22, Patchset 9 (Latest):This change is put behind a runtime feature flag enabled by default.
    Sanket Joshi . unresolved

    Would be good to explain the reasoning for this as well.

    File third_party/blink/renderer/core/editing/frame_selection.cc
    Line 1052, Patchset 9 (Latest): .SetRenderTextWithoutCSSTransforms(true)
    Sanket Joshi . unresolved

    Can we use similar naming as the plain text case? Ex. |SetIgnoresCSSTransformsForRenderText|

    File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
    Line 71, Patchset 9 (Latest): layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;
    Sanket Joshi . unresolved

    Could you clarify why these adjustments for the `-webkit-text-security` property are required?

    Line 79, Patchset 9 (Latest): if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
    text.Replace(kNewlineCharacter, kSpaceCharacter);
    }
    Sanket Joshi . unresolved

    Could you clarify why these whitespace adjustments are necessary?

    File third_party/blink/renderer/core/editing/serializers/create_markup_options.h
    Line 34, Patchset 9 (Latest): bool RenderTextWithoutCSSTransforms() const {
    Sanket Joshi . unresolved

    nit: Can we be consistent with our naming across this entire change? "without" or "ignore" either is fine, but let's be consistent everywhere.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Prashant Nevase
    • Rakesh Goulikar
    • Sambamurthy Bandaru
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I14bce749c1b0eb927bf71c949d8d496c6fbf79f8
    Gerrit-Change-Number: 5399744
    Gerrit-PatchSet: 9
    Gerrit-Owner: Sambamurthy Bandaru <sambamurt...@microsoft.com>
    Gerrit-Reviewer: Rakesh Goulikar <rago...@microsoft.com>
    Gerrit-Reviewer: Sambamurthy Bandaru <sambamurt...@microsoft.com>
    Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
    Gerrit-CC: Prashant Nevase <pne...@microsoft.com>
    Gerrit-CC: Sanket Joshi <sa...@microsoft.com>
    Gerrit-Attention: Rakesh Goulikar <rago...@microsoft.com>
    Gerrit-Attention: Sambamurthy Bandaru <sambamurt...@microsoft.com>
    Gerrit-Attention: Prashant Nevase <pne...@microsoft.com>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 21:14:03 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Sambamurthy Bandaru (Gerrit)

    unread,
    Apr 29, 2024, 10:21:11 AMApr 29
    to Prashant Nevase, AyeAye, Sanket Joshi, Rakesh Goulikar, Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-...@chromium.org
    Attention needed from Prashant Nevase, Rakesh Goulikar, Sanket Joshi and Siye Liu

    Sambamurthy Bandaru added 12 comments

    Patchset-level comments
    File-level comment, Patchset 9:
    Sanket Joshi . resolved

    Would suggest updating the naming everywhere in this CL to "CSSTextTransform" instead of "CSSTransform". CSS transforms are different: https://developer.mozilla.org/en-US/docs/Web/CSS/transform.

    Sambamurthy Bandaru

    Acknowledged

    Commit Message
    Line 13, Patchset 9:SelectedTextForClipBoard() gets text from text iterator.
    Sanket Joshi . resolved

    nit: |SelectedTextForClipboard|

    Sambamurthy Bandaru

    Acknowledged

    Line 22, Patchset 9:This change is put behind a runtime feature flag enabled by default.
    Sanket Joshi . resolved

    Would be good to explain the reasoning for this as well.

    Sambamurthy Bandaru

    Acknowledged

    File third_party/blink/renderer/core/editing/frame_selection.cc
    Line 1052, Patchset 9: .SetRenderTextWithoutCSSTransforms(true)
    Sanket Joshi . resolved

    Can we use similar naming as the plain text case? Ex. |SetIgnoresCSSTransformsForRenderText|

    Sambamurthy Bandaru

    Acknowledged

    Line 1068, Patchset 9: builder.SetIgnoresCSSTransforms(true);
    Siye Liu . resolved

    Nit: `builder.SetIgnoresCSSTransforms(RuntimeEnabledFeatures::IgnoresCSSTransformsForPlainTextCopyEnabled());`

    Sambamurthy Bandaru

    Acknowledged

    File third_party/blink/renderer/core/editing/iterators/text_iterator_text_node_handler.cc
    Line 70, Patchset 9: bool isSecure =
    Sanket Joshi . resolved

    nit: is_secure

    https://google.github.io/styleguide/cppguide.html#Variable_Names

    Sambamurthy Bandaru

    Acknowledged