Replace CR/NL by space - don't remove altogether when xml:space=default #32888
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 86898fe)
|
Ahmad-S792
changed the title
Ahmad-S792
force-pushed
the
eng/Replace-CRNL-by-space---dont-remove-altogether-when-xmlspacedefault
branch
from
86898fe
to
9b4ab67
Compare
|
EWS run on previous version of this PR (hash 9b4ab67) |
webkit-ews-buildbot
added
the
merging-blocked
Ahmad-S792
removed
the
merging-blocked
Ahmad-S792
force-pushed
the
eng/Replace-CRNL-by-space---dont-remove-altogether-when-xmlspacedefault
branch
from
9b4ab67
to
ddddc32
Compare
|
EWS run on previous version of this PR (hash ddddc32) |
webkit-ews-buildbot
added
the
merging-blocked
Ahmad-S792
removed
the
merging-blocked
Ahmad-S792
force-pushed
the
eng/Replace-CRNL-by-space---dont-remove-altogether-when-xmlspacedefault
branch
from
ddddc32
to
c7fb8b6
Compare
|
EWS run on previous version of this PR (hash c7fb8b6) |
webkit-ews-buildbot
added
the
merging-blocked
https://bugs.webkit.org/show_bug.cgi?id=278524 rdar://134545958 Reviewed by NOBODY (OOPS!). This patch aligns WebKit with Gecko / Firefox and Blink / Chromium. Merge: https://chromium.googlesource.com/chromium/src.git/+/0c39a6f29e59594a7b9c412a8203af9fc43e4ecd This moves handling of xml:space=default closer to the more generic white-space handling, by not removing CR and NL characters, but rather just replacing them with a regular space. * Source/WebCore/rendering/svg/RenderSVGInlineText.cpp: (WebCore::normalizeWhitespace): Renamed from `applySVGWhitespaceRules` (WebCore::RenderSVGInlineText::RenderSVGInlineText): (WebCore::RenderSVGInlineText::styleDidChange): * LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Element-childElementCount-dynamic-add-svg-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Element-childElementCount-dynamic-remove-svg-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Element-childElementCount-svg-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Element-firstElementChild-svg-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Element-previousElementSibling-svg-expected.txt: * LayoutTests/platform/glib/svg/custom/text-whitespace-handling-expected.txt: * LayoutTests/platform/glib/svg/text/font-size-below-point-five-expected.txt: * LayoutTests/platform/glib/svg/text/text-tselect-02-f-expected.txt: * LayoutTests/platform/glib/svg/text/text-ws-01-t-expected.txt: * LayoutTests/platform/glib/svg/text/textPathBoundsBug-expected.txt: * LayoutTests/platform/ios/imported/w3c/web-platform-tests/svg/import/text-altglyph-01-b-manual-expected.txt: * LayoutTests/platform/ios/imported/w3c/web-platform-tests/svg/import/text-tselect-02-f-manual-expected.txt: * LayoutTests/platform/ios/imported/w3c/web-platform-tests/svg/import/text-tselect-03-f-manual-expected.txt: * LayoutTests/platform/ios/svg/W3C-SVG-1.1/styling-css-02-b-expected.txt: * LayoutTests/platform/ios/svg/W3C-SVG-1.1/text-align-08-b-expected.txt: * LayoutTests/platform/ios/svg/text/text-altglyph-01-b-expected.txt: * LayoutTests/platform/ios/svg/text/text-ws-01-t-expected.txt: * LayoutTests/platform/mac/imported/w3c/web-platform-tests/svg/import/text-altglyph-01-b-manual-expected.txt: * LayoutTests/platform/mac/imported/w3c/web-platform-tests/svg/import/text-tselect-02-f-manual-expected.txt: * LayoutTests/platform/mac/imported/w3c/web-platform-tests/svg/import/text-tselect-03-f-manual-expected.txt: * LayoutTests/platform/mac/svg/W3C-SVG-1.1/styling-css-02-b-expected.txt: * LayoutTests/platform/mac/svg/W3C-SVG-1.1/text-align-08-b-expected.txt: * LayoutTests/platform/mac/svg/W3C-SVG-1.1/text-tselect-02-f-expected.txt: * LayoutTests/platform/mac/svg/W3C-SVG-1.1/text-ws-01-t-expected.txt: * LayoutTests/platform/mac/svg/custom/text-whitespace-handling-expected.txt: * LayoutTests/platform/mac/svg/text/font-size-below-point-five-expected.txt: * LayoutTests/platform/mac/svg/text/text-tselect-02-f-expected.txt: * LayoutTests/platform/mac/svg/text/text-ws-01-t-expected.txt: * LayoutTests/platform/mac/svg/zoom/page/zoom-mask-with-percentages-expected.txt: * LayoutTests/platform/wpe/svg/W3C-SVG-1.1/styling-css-02-b-expected.txt: * LayoutTests/platform/wpe/svg/W3C-SVG-1.1/text-align-08-b-expected.txt: * LayoutTests/platform/wpe/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.txt: * LayoutTests/platform/wpe/svg/W3C-SVG-1.1/text-tselect-02-f-expected.txt: * LayoutTests/platform/wpe/svg/W3C-SVG-1.1/text-ws-01-t-expected.txt: * LayoutTests/platform/wpe/svg/text/text-altglyph-01-b-expected.txt: * LayoutTests/svg/custom/invalid-text-content-expected.txt: * LayoutTests/svg/dom/altGlyph-dom-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/paths-dom-01-f-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/styling-css-02-b-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-align-04-b-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-align-06-b-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-align-08-b-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-altglyph-01-b-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-dom-01-f-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-tselect-02-f-manual-expected.txt: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/svg/import/text-tselect-03-f-manual-expected.txt: * LayoutTests/platform/gtk/svg/W3C-SVG-1.1/text-align-08-b-expected.txt: * LayoutTests/platform/gtk/svg/W3C-SVG-1.1/text-tselect-02-f-expected.txt: * LayoutTests/platform/gtk/svg/W3C-SVG-1.1/text-ws-01-t-expected.txt: * LayoutTests/platform/ios/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.txt: * LayoutTests/platform/ios/svg/W3C-SVG-1.1/text-tselect-02-f-expected.txt: * LayoutTests/platform/ios/svg/custom/text-whitespace-handling-expected.txt: * LayoutTests/platform/ios/svg/text/font-size-below-point-five-expected.txt: * LayoutTests/platform/ios/svg/text/text-align-01-b-expected.txt: * LayoutTests/platform/ios/svg/text/text-hkern-expected.txt: * LayoutTests/platform/ios/svg/text/text-tselect-02-f-expected.txt: * LayoutTests/platform/ios/svg/text/text-ws-01-t-expected.txt: * LayoutTests/platform/mac-sonoma/svg/W3C-SVG-1.1/text-tselect-02-f-expected.txt: * LayoutTests/platform/wpe/svg/W3C-SVG-1.1/interact-order-01-b-expected.txt: * LayoutTests/svg/text/textPathBoundsBug-expected.txt: * LayoutTests/platform/ios/svg/text/text-align-01-b-expected.txt: * LayoutTests/platform/ios/svg/text/text-altglyph-01-b-expected.txt: * LayoutTests/platform/ios/svg/text/text-ws-01-t-expected.txt: * LayoutTests/platform/ios/svg/W3C-SVG-1.1/text-ws-01-t-expected.txt: * LayoutTests/svg/W3C-SVG-1.1/styling-css-02-b-expected.txt:
Ahmad-S792
removed
the
merging-blocked
Ahmad-S792
force-pushed
the
eng/Replace-CRNL-by-space---dont-remove-altogether-when-xmlspacedefault
branch
from
c7fb8b6
to
2fbcfd8
Compare
|
EWS run on current version of this PR (hash 2fbcfd8) |
Ahmad-S792
requested review from
smfr,
rwlbuis,
shallawa and
nikolaszimmermann
smfr
reviewed
Aug 30, 2024
| newString = makeStringByReplacingAll(newString, '\t', ' '); | ||
| newString = makeStringByReplacingAll(newString, '\n', ' '); |
There was a problem hiding this comment.
This is really inefficient. We must have code somewhere to replace multiple characters in one traversal.
Is there an expectation that CRLF will get converted to a single space?
There was a problem hiding this comment.
We fail following test case across browser - https://jsfiddle.net/asyrzuz2/, we use to do emptyString (""_s) before.
I will search through code to see, if there is a way to do all these via single go.
There was a problem hiding this comment.
The WTF::String::simplifyWhitespace function is close to what we want. Especially because the comment mentions “all contiguous space characters will be consolidated” and that’s part of what simplifyWhitespace does. But it’s possible we’ll have to write a new function if we want something that’s super efficient and does exactly the right operation, and that is probably beyond the scope of what we should do in this pull request that is focused on correctness rather than efficiency. Scanning the string multiple times is not great, but doing so with the super-efficient “just check for one specific character” case might even be faster than scanning once with a less efficient inner loop.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2fbcfd8
2fbcfd8