Closed Bug 1906978 Opened 2 months ago Closed 2 months ago

SVG text anchor regression with embedding marks (U+202C ... U+202C)

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
Firefox 128
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr128 --- fixed
firefox128 --- wontfix
firefox129 --- fixed
firefox130 --- fixed

People

(Reporter: jonathan.olson, Assigned: jfkthame)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

firefox-embedding-mark-text-anchor.svg
872 bytes, image/svg+xml
Details
testcase with additional examples [updated]
2.87 KB, image/svg+xml
Details
Screenshot comparing Chrome, Firefox 127, and Nightly rendering of the expanded testcase
246.83 KB, image/png
Details
Bug 1906978 - Fix the positioning/anchoring of SVG text runs with bidi control characters. r=#layout,longsonr
48 bytes, text/x-phabricator-request
Details | Review
Bug 1906978 - Fix the positioning/anchoring of SVG text runs with bidi control characters.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Bug 1906978 - Fix the positioning/anchoring of SVG text runs with bidi control characters.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-esr128+
Details | Review

Steps to reproduce:

SVG <text> elements with direction="ltr" whose text starts with the character U+202B (right-to-left embedding) and ends with the character U+202C (pop directional formatting).

Minimal example attached.

Actual results:

Firefox 128 (current), 130.0a1 (nightly):

The text is shifted to the right (by the exact width of the text). It seems like the text-anchor of the text is mis-interpreted as being RTL.

The origin of the text seems shifted to the right, so that it does not match with the text-anchor position.

This resulted in all RTL text in PhET simulations (phet.colorado.edu) being shifted improperly to the right (adding a workaround to adjust the direction and strip embedding marks).

Expected results:

Firefox 127 / Chrome 126,127 / Safari 17:

The text-anchor is NOT shifted for RTL text.

The Bugbug bot thinks this bug should belong to the 'Core::SVG' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → SVG
Product: Firefox → Core

Can you use mozregression to find a regression range please?

Flags: needinfo?(jonathan.olson)

I'm pretty confident this will turn out to be related to the switch from ICU4C to ICU4X/unicode-bidi as the underlying implementation of the Unicode bidi algorithm. This was implemented in bug 1824671 (for Firefox 124, but initially Nightly-only), and enabled for release in bug 1896117 (Firefox 128). Although the resulting layout should be unchanged, the handling of embedding controls (which are invisible in the result) is slightly different, and that must be affecting the SVG behavior, so we need to figure out how/why that happens.

Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regressionwindow-wanted → regression
Regressed by: 1824671, 1896117

Actually, it seems that in cases with directional-override/embedding controls, there were already discrepancies between Firefox and Chrome behavior, so this was always a problematic area. Then the switch of bidi engine caused a further change, but (unfortunately) didn't simply align our behavior.

Flags: needinfo?(jonathan.olson)
Attached image testcase with additional examples (obsolete) —

Here's a version of the testcase with a couple of additional examples using directional controls.

bug 1824671 does however seem to have fixed bug 1131192 and the odd thing is that bug 1131192 examples have directional controls in them so I guess we need to be careful not to regress that.

See Also: → 1131192

Added some more examples to the testcase.

Attachment #9411967 - Attachment is obsolete: true

Safari's rendering matches Chrome's (on the left). Middle is Firefox before switching the bidi implementation, so still using ICU4C; right is current Nightly, using ICU4X/unicode-bidi.

(In reply to Robert Longson [:longsonr] from comment #6)

bug 1824671 does however seem to have fixed bug 1131192 and the odd thing is that bug 1131192 examples have directional controls in them so I guess we need to be careful not to regress that.

Ah, interesting - yeah, overall I'd say the new behavior here (as seen in comment 8) is less broken than it used to be; the text all appears correctly, without parts overprinting each other, just the overall anchor alignment is wrong when the text starts with a RTL run. I think this is because ShiftAnchoredChunk assumes the (logical) first character of the chunk is what should be anchored, but that isn't true if bidi-reversal is in play.

The case where it starts with RTL U+202B and has a pop U+202C in the middle is currently something I haven't been able to work around:

Example string: ">pH: 7.00" ("\u202bpH\u202c: 7.00")

Neither text-anchor="start" or text-anchor="end" result in a correct result.

The added reftests here failed both prior to and after the bidi-engine update,
although the exact nature of the failures changed due to the engine leaving the
bidi control characters in different places in the reordered text. (This is
explicitly unspecified by the Unicode Bidi Algorithm, because the control codes
are nominally removed after processing; leaving them present but invisible is
an implementation detail.)

The key fix here is that DoGlyphPositioning() anchors the first addressable
character of the text, not necessarily the absolute first codepoint, as that
may be a "deleted" control code whose position is arbitrary.

Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad57df08b538
Fix the positioning/anchoring of SVG text runs with bidi control characters. r=longsonr,layout-reviewers,emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47116 for changes under testing/web-platform/tests

https://hg.mozilla.org/mozilla-central/rev/ad57df08b538

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
status-firefox130: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 130 Branch
Upstream PR merged by moz-wptsync-bot

The added reftests here failed both prior to and after the bidi-engine update,
although the exact nature of the failures changed due to the engine leaving the
bidi control characters in different places in the reordered text. (This is
explicitly unspecified by the Unicode Bidi Algorithm, because the control codes
are nominally removed after processing; leaving them present but invisible is
an implementation detail.)

The key fix here is that DoGlyphPositioning() anchors the first addressable
character of the text, not necessarily the absolute first codepoint, as that
may be a "deleted" control code whose position is arbitrary.

Original Revision: https://phabricator.services.mozilla.com/D216400

Attachment #9412714 - Flags: approval-mozilla-beta?

beta Uplift Approval Request

  • User impact if declined: SVG text involving bidi controls may be misplaced
  • Code covered by automated testing: yes
  • Fix verified in Nightly: no
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: n/a
  • Risk associated with taking this patch: low
  • Explanation of risk level: narrowly-targeted change to skip over invisible controls when anchoring svg text run
  • String changes made/needed: none
  • Is Android affected?: yes

The added reftests here failed both prior to and after the bidi-engine update,
although the exact nature of the failures changed due to the engine leaving the
bidi control characters in different places in the reordered text. (This is
explicitly unspecified by the Unicode Bidi Algorithm, because the control codes
are nominally removed after processing; leaving them present but invisible is
an implementation detail.)

The key fix here is that DoGlyphPositioning() anchors the first addressable
character of the text, not necessarily the absolute first codepoint, as that
may be a "deleted" control code whose position is arbitrary.

Original Revision: https://phabricator.services.mozilla.com/D216400

Attachment #9412715 - Flags: approval-mozilla-esr128?

esr128 Uplift Approval Request

  • User impact if declined: SVG text involving bidi controls may be misplaced
  • Code covered by automated testing: yes
  • Fix verified in Nightly: no
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: n/a
  • Risk associated with taking this patch: low
  • Explanation of risk level: narrowly-targeted change to skip over invisible controls when anchoring svg text run
  • String changes made/needed: none
  • Is Android affected?: yes
Attachment #9412714 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/4f105579317d
See Also: 1131192 →
Attachment #9412715 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+
https://hg.mozilla.org/releases/mozilla-esr128/rev/746d01c62756
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: