Ignore text transform for plain text copy [chromium/src : main]
Siye Liu (Gerrit)
Siye Liu added 7 comments![]( "Open in Gerrit")
String FrameSelection::SelectedHTMLForClipboard() const {Do we also need to set `ignores_css_transforms` to `true` here?
TextIteratorBehavior::Builder::SetIgnoresCssTransforms(bool value) {Please add some test coverage for this new behavior.
if (behavior.EmitsOriginalText() || behavior.IgnoresCssTransforms()) {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.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());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.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());There are a few unit test failures in the CQ dry run. Please address those failures.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());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`.
Related details
- Rakesh Goulikar
- Sambamurthy Bandaru
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sanket Joshi (Gerrit)
Sanket Joshi added 1 comment![]( "Open in Gerrit")
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.
Related details
- Rakesh Goulikar
- Sambamurthy Bandaru
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sanket Joshi (Gerrit)
Sanket Joshi added 1 comment![]( "Open in Gerrit")
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
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau removed dpr...@google.com from this change![]( "Open in Gerrit")
Related details
- Rakesh Goulikar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sambamurthy Bandaru (Gerrit)
Sambamurthy Bandaru added 8 comments![]( "Open in Gerrit")
Sambamurthy BandaruGiven 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-codeIn 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.
Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'
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-codeIn 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
Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'
String FrameSelection::SelectedHTMLForClipboard() const {Do we also need to set `ignores_css_transforms` to `true` here?
`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.
TextIteratorBehavior::Builder::SetIgnoresCssTransforms(bool value) {Please add some test coverage for this new behavior.
Added web tests
if (behavior.EmitsOriginalText() || behavior.IgnoresCssTransforms()) {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.
Acknowledged
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());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.
Updated commit description with code flow
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());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`.
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.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());There are a few unit test failures in the CQ dry run. Please address those failures.
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 5 comments![]( "Open in Gerrit")
String FrameSelection::SelectedHTMLForClipboard() const {Sambamurthy BandaruDo we also need to set `ignores_css_transforms` to `true` here?
`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.
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?
TextIteratorBehavior::Builder::SetIgnoresCssTransforms(bool value) {Sambamurthy BandaruPlease add some test coverage for this new behavior.
Added web tests
Done
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());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`.
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.
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?
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());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.
Updated commit description with code flow
Done
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;I think we may not need this case because the runtime feature flag is `true` by default.
Related details
- Rakesh Goulikar
- Sambamurthy Bandaru
- Sanket Joshi
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sambamurthy Bandaru (Gerrit)
Sambamurthy Bandaru added 3 comments![]( "Open in Gerrit")
String FrameSelection::SelectedHTMLForClipboard() const {Sambamurthy BandaruDo we also need to set `ignores_css_transforms` to `true` here?
Siye Liu`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.
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?
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.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());Sambamurthy BandaruAre 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`.
Siye LiuLayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.
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?
Text iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;I think we may not need this case because the runtime feature flag is `true` by default.
Thought we may need this in case we disable the runtime feature flag
Related details
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 3 comments![]( "Open in Gerrit")
String FrameSelection::SelectedHTMLForClipboard() const {Sambamurthy BandaruDo we also need to set `ignores_css_transforms` to `true` here?
Siye Liu`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.
Sambamurthy BandaruSince 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?
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.
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.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());Sambamurthy BandaruAre 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`.
Siye LiuLayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.
Sambamurthy BandaruWhat 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?
Text iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true
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.
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;Sambamurthy BandaruI think we may not need this case because the runtime feature flag is `true` by default.
Thought we may need this in case we disable the runtime feature flag
Good consideration. It's okay to only have the default runtime feature flag case since it's turned on by default.
Related details
- Rakesh Goulikar
- Sambamurthy Bandaru
- Sanket Joshi
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sambamurthy Bandaru (Gerrit)
Sambamurthy Bandaru added 3 comments![]( "Open in Gerrit")
String FrameSelection::SelectedHTMLForClipboard() const {Sambamurthy BandaruDo we also need to set `ignores_css_transforms` to `true` here?
Siye Liu`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.
Sambamurthy BandaruSince 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?
Siye LiuYes, that looks safe. SelectedHTMLForClipboard used only for copy, text drag and print renderer. And the selected markup's style still has the text-transform.
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.
Agreed. Added a flag 'SetRenderTextWithoutCSSTransforms' in CreateMarkupOptions. Named it that way because the markup will still have text-transform in Style.
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());Sambamurthy BandaruAre 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`.
Siye LiuLayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.
Sambamurthy BandaruWhat 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?
Siye LiuText iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true
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.
Done. Added a flag in CreateMarkupOptions and this is followed from there.
See latest patchset
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;Sambamurthy BandaruI think we may not need this case because the runtime feature flag is `true` by default.
Siye LiuThought we may need this in case we disable the runtime feature flag
Good consideration. It's okay to only have the default runtime feature flag case since it's turned on by default.
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Prashant Nevase (Gerrit)
Prashant Nevase added 1 comment![]( "Open in Gerrit")
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)With this cl description looks unnecessarily lengthy. Is it possible to summarize this?
Related details
- Rakesh Goulikar
- Sambamurthy Bandaru
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sanket Joshi (Gerrit)
Sanket Joshi added 3 comments![]( "Open in Gerrit")
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)With this cl description looks unnecessarily lengthy. Is it possible to summarize this?
+1
.SetRenderTextWithoutCSSTransforms(true)Shouldn't this change also be made behind the feature flag?
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()Shouldn't these changes also be made behind the feature flag?
Related details
- Prashant Nevase
- Rakesh Goulikar
- Sambamurthy Bandaru
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sambamurthy Bandaru (Gerrit)
Sambamurthy Bandaru added 7 comments![]( "Open in Gerrit")
Sambamurthy BandaruGiven 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-codeIn 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
Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'
Done
Sambamurthy BandaruGiven 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-codeIn 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.
Added a runtime feature flag 'IgnoresCSSTransformsForPlainTextCopy'
|
+-- 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)Sanket JoshiWith this cl description looks unnecessarily lengthy. Is it possible to summarize this?
+1
Updated description
String FrameSelection::SelectedHTMLForClipboard() const {Sambamurthy BandaruDo we also need to set `ignores_css_transforms` to `true` here?
Siye Liu`ignores_css_transforms` is set to `true` in `StyledMarkupAccumulator::RenderedText(Text& text_node)` which falls in the code flow from here.
Sambamurthy BandaruSince 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?
Siye LiuYes, that looks safe. SelectedHTMLForClipboard used only for copy, text drag and print renderer. And the selected markup's style still has the text-transform.
Sambamurthy BandaruEven 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.
Agreed. Added a flag 'SetRenderTextWithoutCSSTransforms' in CreateMarkupOptions. Named it that way because the markup will still have text-transform in Style.
Done
.SetRenderTextWithoutCSSTransforms(true)Shouldn't this change also be made behind the feature flag?
The options are used in RenderedText() method in styled accumulator. There it was behind the feature flag
TextIteratorBehavior::Builder().SetIgnoresCssTransforms(true).Build());Sambamurthy BandaruAre 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`.
Siye LiuLayoutText::TransformedText() method is available for those cases, for eg. it is used when the layout is created. PlainText() method could return text without transform.
Sambamurthy BandaruWhat 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?
Siye LiuText iterator behavior object can be passed to RenderedText() from 'SelectedHTMLForClipboard' where it is safe to set the flag to true
Sambamurthy BandaruI 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.
Done. Added a flag in CreateMarkupOptions and this is followed from there.
See latest patchset
Done
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = false;Sambamurthy BandaruI think we may not need this case because the runtime feature flag is `true` by default.
Siye LiuThought we may need this in case we disable the runtime feature flag
Sambamurthy BandaruGood consideration. It's okay to only have the default runtime feature flag case since it's turned on by default.
Removed
- Prashant Nevase
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sambamurthy Bandaru (Gerrit)
Sambamurthy Bandaru added 3 comments![]( "Open in Gerrit")
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)Sanket JoshiWith this cl description looks unnecessarily lengthy. Is it possible to summarize this?
Sambamurthy Bandaru+1
Updated description
Done
.SetRenderTextWithoutCSSTransforms(true)Sambamurthy BandaruShouldn't this change also be made behind the feature flag?
The options are used in RenderedText() method in styled accumulator. There it was behind the feature flag
Done
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()Sambamurthy BandaruShouldn't these changes also be made behind the feature flag?
behavior.SetIgnoresCSSTransforms() is behind feature flag
Related details
- Prashant Nevase
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 4 comments![]( "Open in Gerrit")
builder.SetIgnoresCSSTransforms(true);Nit: `builder.SetIgnoresCSSTransforms(RuntimeEnabledFeatures::IgnoresCSSTransformsForPlainTextCopyEnabled());`
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();
}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.
else if (fromElementName == "transform" && !(toElementNameSuffix == "text-control"))Why do we need the extra logic? Can you add some comments?
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = true;Nit: no need to explicitly set the flag. The runtime feature flag is enabled by default.
Related details
- Prashant Nevase
- Rakesh Goulikar
- Sambamurthy Bandaru
- Sanket Joshi
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sanket Joshi (Gerrit)
Sanket Joshi added 8 comments![]( "Open in Gerrit")
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.
SelectedTextForClipBoard() gets text from text iterator.nit: |SelectedTextForClipboard|
This change is put behind a runtime feature flag enabled by default.Would be good to explain the reasoning for this as well.
.SetRenderTextWithoutCSSTransforms(true)Can we use similar naming as the plain text case? Ex. |SetIgnoresCSSTransformsForRenderText|
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;Could you clarify why these adjustments for the `-webkit-text-security` property are required?
if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
text.Replace(kNewlineCharacter, kSpaceCharacter);
}Could you clarify why these whitespace adjustments are necessary?
bool RenderTextWithoutCSSTransforms() const {nit: Can we be consistent with our naming across this entire change? "without" or "ignore" either is fine, but let's be consistent everywhere.
Related details
- Prashant Nevase
- Rakesh Goulikar
- Sambamurthy Bandaru
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sambamurthy Bandaru (Gerrit)
Sambamurthy Bandaru added 12 comments![]( "Open in Gerrit")
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.
Acknowledged
SelectedTextForClipBoard() gets text from text iterator.Sambamurthy Bandarunit: |SelectedTextForClipboard|
Acknowledged
This change is put behind a runtime feature flag enabled by default.Would be good to explain the reasoning for this as well.
Acknowledged
.SetRenderTextWithoutCSSTransforms(true)Sambamurthy BandaruCan we use similar naming as the plain text case? Ex. |SetIgnoresCSSTransformsForRenderText|
Acknowledged
builder.SetIgnoresCSSTransforms(true);Sambamurthy BandaruNit: `builder.SetIgnoresCSSTransforms(RuntimeEnabledFeatures::IgnoresCSSTransformsForPlainTextCopyEnabled());`
Acknowledged
bool isSecure =Sambamurthy Bandarunit: is_secure
https://google.github.io/styleguide/cppguide.html#Variable_Names
Acknowledged
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;Could you clarify why these adjustments for the `-webkit-text-security` property are required?
Return masked text from |mapping.GetText()| when |-webkit-text-security| is set. If original text is required, |behavior.EmitsOriginalText()| flag should be used.
Added same comment in src
if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
text.Replace(kNewlineCharacter, kSpaceCharacter);
}Could you clarify why these whitespace adjustments are necessary?
|LayoutText::PlainText()| has text with collapsed line breaks. Collapse line breaks here as well to keep the results consistent.
Added same comment in src.
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();
}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.
Done
nit: Can we be consistent with our naming across this entire change? "without" or "ignore" either is fine, but let's be consistent everywhere.
Acknowledged
else if (fromElementName == "transform" && !(toElementNameSuffix == "text-control"))Why do we need the extra logic? Can you add some comments?
Here, plain text is pasted into input element and html text is pasted into span element. Seems fine. When the actual text is read from 'element.innerText', it returns the transformed text if style contains text-transform. So, for span element the text would be in uppercase.
Spec doesn't specify what element.innerText should return. Firefox too returns transformed text if style has text-transforms.
internals.runtimeFlags.ignoresCSSTransformsForPlainTextCopyEnabled = true;Nit: no need to explicitly set the flag. The runtime feature flag is enabled by default.
- Prashant Nevase
- Rakesh Goulikar
- Sanket Joshi
- Siye Liu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siye Liu (Gerrit)
Siye Liu added 4 comments![]( "Open in Gerrit")
.SetIgnoresCSSTextTransformsForRenderedText(true)Should this be `RuntimeEnabledFeatures::IgnoresCSSTextTransformsForPlainTextCopyEnabled()`?
.SetEntersTextControls(true)Nit: `.SetIgnoresCSSTextTransforms(RuntimeEnabledFeatures::IgnoresCSSTextTransformsForPlainTextCopyEnabled())`
builder.SetIgnoresCSSTextTransforms(
options_.IgnoresCSSTextTransformsForRenderedText());`options_.IgnoresCSSTextTransformsForRenderedText()` should always be `false` if the runtime feature flag is not enabled. So can we simply the code by removing the runtime feature flag check here?
else if (fromElementName == "transform" && !(toElementNameSuffix == "text-control"))Sambamurthy BandaruWhy do we need the extra logic? Can you add some comments?
Here, plain text is pasted into input element and html text is pasted into span element. Seems fine. When the actual text is read from 'element.innerText', it returns the transformed text if style contains text-transform. So, for span element the text would be in uppercase.
Spec doesn't specify what element.innerText should return. Firefox too returns transformed text if style has text-transforms.
- Prashant Nevase
- Rakesh Goulikar
- Sambamurthy Bandaru
- Sanket Joshi
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sanket Joshi (Gerrit)
Sanket Joshi added 5 comments![]( "Open in Gerrit")
.SetIgnoresCSSTextTransformsForRenderedText(ignores_text_transforms)The intent of the feature flag is that it should gate all deltas in the CL compared to what was there before. That ensures that if we do need to do a kill switch (ie. turn the feature flag off remotely), then we truly can get back to the same state before this change, which is known to be good.
What you have here isn't necessarily functionally wrong, so not requiring any specific changes. But FYI that we do take on additional risk that the kill switch won't work when we add any code that is expected to no-op with the feature flag off.
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;Sambamurthy BandaruCould you clarify why these adjustments for the `-webkit-text-security` property are required?
Return masked text from |mapping.GetText()| when |-webkit-text-security| is set. If original text is required, |behavior.EmitsOriginalText()| flag should be used.
Added same comment in src
Similar to above. Could you confirm if/how other browsers handle this property for plain text copy, as well as what is Chromium's current behavior (prior to this change)? Also, can you add a test to cover this case?
if (!layout_object.StyleRef().ShouldPreserveBreaks()) {
text.Replace(kNewlineCharacter, kSpaceCharacter);
}Sambamurthy BandaruCould you clarify why these whitespace adjustments are necessary?
|LayoutText::PlainText()| has text with collapsed line breaks. Collapse line breaks here as well to keep the results consistent.
Added same comment in src.
Could you confirm whether other browsers also collapse line breaks for plain text copy, as well as what is Chromium's current behavior (prior to this change)? Also, can you add a test to cover this case?
bool is_secure =
layout_object.StyleRef().TextSecurity() != ETextSecurity::kNone;
// Return masked text from |mapping.GetText()| when |-webkit-text-security| is
// set. If original text is required, |behavior.EmitsOriginalText()| flag
// should be used
if (!is_secure && behavior.IgnoresCSSTextTransforms()) {
String text = TextIgnoringCSSTextTransforms(layout_object);
result.string =
text.Substring(unit.DOMStart(), unit.DOMEnd() - unit.DOMStart());
result.start = 0;
result.end = result.string.length();
}Can we put this code behind the enabled-by-default runtime feature flag?
status: "stable",Have you validated these changes with the feature flag off?
Related details
- Prashant Nevase
- Rakesh Goulikar
- Sambamurthy Bandaru
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |