Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

getStartPositionOfChar(n) on an element with 'n' characters of text doesn't throw IndexSizeError #12802

Merged
merged 1 commit into from Apr 18, 2023
Merged

getStartPositionOfChar(n) on an element with 'n' characters of text doesn't throw IndexSizeError #12802

merged 1 commit into from Apr 18, 2023

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Apr 17, 2023 •
edited by webkit-early-warning-system

25e8d82

getStartPositionOfChar(n) on an element with 'n' characters of text doesn't throw IndexSizeError

https://bugs.webkit.org/show_bug.cgi?id=255500
rdar://problem/108115821

Reviewed by Said Abou-Hallawa.

This patch aligns WebKit to Blink / Chromium, Gecko / Firefox and Web-Spec.

Cherry-Pick: https://src.chromium.org/viewvc/blink?view=revision&;revision=201411

According to the spec [1], IndexSizeError should be thrown if:

"...the charnum is negative or if charnum is greater than or equal to
 the number of characters at this node."

[1] http://www.w3.org/TR/SVG11/text.html#__svg__SVGTextContentElement__getStartPositionOfChar
    The current SVG2 draft has a different formulation:

     SVG2 - https://svgwg.org/svg2-draft/text.html#__svg__SVGTextContentElement__getStartPositionOfChar
     "If cluster is null, then then throw a DOMException with code
      INDEX_SIZE_ERR."

    but will have the same result.

* Source/WebCore/svg/SVGTextContentElement.cpp:
(SVGTextContentElement::getStartPositionOfChar):
(SVGTextContentElement::getEndPositionOfChar):
(SVGTextContentElement::getExtentOfChar):
(SVGTextContentElement::getRotationOfChar):
* LayoutTests/svg/text/svgtextcontentelement-equality-methods-parameters.html: Add Test
* LayoutTests/svg/text/svgtextcontentelement-equality-methods-parameters-expected.txt: Add Test Expectation

Canonical link: https://commits.webkit.org/263049@main

f5a11eb

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1   🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2   🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added the SVG For bugs in the SVG implementation. label Apr 17, 2023
@Ahmad-S792 Ahmad-S792 self-assigned this Apr 17, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash fde751b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
  🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 17, 2023
@Ahmad-S792 Ahmad-S792 force-pushed the fix255500-getStartPositionOfChar branch from fde751b to 8aa5e0b Compare April 17, 2023 18:34
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Apr 17, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 8aa5e0b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

shallawa
shallawa approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@shallawa shallawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not svgtextcontentelement-euqality-methods-parameters.html be added as a wpt test?

@Ahmad-S792
Copy link
Contributor Author

Should not svgtextcontentelement-euqality-methods-parameters.html be added as a wpt test?

I think adding full test from Chromium repository to WPT would be beneficial for all browsers. Let me raise an issue on WPT and if needed, I am happy to move Chromium test to WPT. :-)

@Ahmad-S792 Ahmad-S792 force-pushed the fix255500-getStartPositionOfChar branch from 8aa5e0b to f5a11eb Compare April 17, 2023 23:56
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash f5a11eb)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1   🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2   🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added the merge-queue Applied to send a pull request to merge-queue label Apr 17, 2023
@Ahmad-S792
getStartPositionOfChar(n) on an element with 'n' characters of text d…
25e8d82 
…oesn't throw IndexSizeError

https://bugs.webkit.org/show_bug.cgi?id=255500
rdar://problem/108115821

Reviewed by Said Abou-Hallawa.

This patch aligns WebKit to Blink / Chromium, Gecko / Firefox and Web-Spec.

Cherry-Pick: https://src.chromium.org/viewvc/blink?view=revision&revision=201411

According to the spec [1], IndexSizeError should be thrown if:

"...the charnum is negative or if charnum is greater than or equal to
 the number of characters at this node."

[1] http://www.w3.org/TR/SVG11/text.html#__svg__SVGTextContentElement__getStartPositionOfChar
    The current SVG2 draft has a different formulation:

     SVG2 - https://svgwg.org/svg2-draft/text.html#__svg__SVGTextContentElement__getStartPositionOfChar
     "If cluster is null, then then throw a DOMException with code
      INDEX_SIZE_ERR."

    but will have the same result.

* Source/WebCore/svg/SVGTextContentElement.cpp:
(SVGTextContentElement::getStartPositionOfChar):
(SVGTextContentElement::getEndPositionOfChar):
(SVGTextContentElement::getExtentOfChar):
(SVGTextContentElement::getRotationOfChar):
* LayoutTests/svg/text/svgtextcontentelement-equality-methods-parameters.html: Add Test
* LayoutTests/svg/text/svgtextcontentelement-equality-methods-parameters-expected.txt: Add Test Expectation

Canonical link: https://commits.webkit.org/263049@main
@webkit-commit-queue webkit-commit-queue merged commit 25e8d82 into WebKit:main Apr 18, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 263049@main (25e8d82): https://commits.webkit.org/263049@main

Reviewed commits have been landed. Closing PR #12802 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 18, 2023
@Ahmad-S792 Ahmad-S792 deleted the fix255500-getStartPositionOfChar branch April 19, 2023 21:25
shallawa
shallawa reviewed May 2, 2023
View reviewed changes
LayoutTests/svg/text/svgtextcontentelement-equality-methods-parameters.html
Comment on lines +21 to +25
function assert_equals_to_SVGPoint(actualPoint, expectedPoint)
{
assert_equals(actualPoint.x, expectedPoint.x);
assert_equals(actualPoint.y, expectedPoint.y);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not used. Please remove.

LayoutTests/svg/text/svgtextcontentelement-equality-methods-parameters.html
Comment on lines +27 to +33
function assert_equals_to_SVGRect(actualRect, expectedRect)
{
assert_equals(actualRect.x, expectedRect.x);
assert_equals(actualRect.y, expectedRect.y);
assert_equals(actualRect.width, expectedRect.width);
assert_equals(actualRect.height, expectedRect.height);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not used. Please remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shallawa shallawa shallawa approved these changes

@smfr smfr Awaiting requested review from smfr

@nikolaszimmermann nikolaszimmermann Awaiting requested review from nikolaszimmermann

Assignees

@Ahmad-S792 Ahmad-S792

Labels
SVG For bugs in the SVG implementation.
Projects
None yet
Milestone
No milestone
5 participants
@Ahmad-S792 @webkit-early-warning-system @webkit-commit-queue @shallawa @webkit-ews-buildbot