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`.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. | 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