Closed Bug 1513423 Opened 6 years ago Closed 6 years ago

make gfxFont::Measure always lay out glyphs left to right

Categories

(Core :: Layout: Text and Fonts, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files, 1 obsolete file)

We currently go to the effort of placing glyphs right to left when we have an RTL text run, but then at the end of the function we translate final bounds to be left-aligned with the origin. Instead we could always place them left to right, and avoid the final translation. For the data in gfxFont::RunMetrics that we calculate, the result will be the same. The only time we should need to worry about the direction is when applying mOffset from a DetailedGlyph -- since we would be working in a "flipped" coordinate space, we need to negate its x component. I didn't see a meaningful performance improvement with this change, but as a simplification it's probably still worth it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b129682322adc9837cf9a495553f2ae721318f Try looks good, as does spot checking some pages in RTL languages. I don't know if any tests on try use a font that has a DetailedGlyph with a non-zero mOffset though.
This avoids multiplying a bunch of values by -1, and then undoing its effect at the end of gfxFont::Measure.
Priority: -- → P5

The trouble here is that while it's fine to conceptually lay out the glyphs LTR for the purpose of accumulating advances to figure out the total width of the glyph run, so we wouldn't need to negate advances when updating the current position, etc., this doesn't work for the other thing gfxFont::Measure does, which is to accumulate the overall bounding box.

Consider two glyphs [x] and [y]. Suppose they each have an advance of 100px. The "ink" of each glyph entirely fills its advance width; glyph [x] also has a swash tail that extends a further 50px beyond its advance.

When we iterate LTR over the glyph sequence [x][y], accumulating widths and bounding box, the total advance will be 200px, and the bounding box will extend from 0 to 200px; the "tail" of [x] extends half-way across the width of [y], but is subsumed within its overall bbox when we Union the rects.

If the glyphs are laid out RTL, the total advance will still be 200px (now measured in the leftward direction, so the x-coordinate will become -200px during the iteration; we then reverse the sign again to return the positive width of the run). But the bounding box should now be 250px wide, because the swash tail of [x] projects to the right of the origin, while the advances of the glyphs move to the left. It no longer overlaps the advance of [y]; instead, it extends the overall bbox to the other side of the origin.

So it doesn't work to simply do the bounding-box computation in LTR mode; running RTL affects not only the origin of the bounds (which we then reset, as noted) but also the overall width.

To handle this, we basically need to reflect the bounding box of each glyph in runs that are really RTL (although we're measuring as LTR), so that the left and right sidebearings are swapped; then the merging of glyph bounding boxes to form the overall bounds will be correct (modulo reflection); at the end, we can reverse this transformation to get the correct final bounds.

Pushed a new try job to see how this fares:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc3576dcf9940497a5d54476019f74a70a2b5674

OK, I've created a couple of extended versions of bugs/1116480-1-fakeitalic-overflow, which tends to fail if we get the RTL bounding box wrong; the new tests add a bunch of delta-positioned DetailedGlyph components, as well as a new reftest based on the Awami example from https://phabricator.services.mozilla.com/D14227#355901. This should give us some confidence we're not breaking things here.

With the new patch, we eliminate all the direction * ... multiplications in Measure; the only bidi-related work left is a couple of short if (isRTL) ... fragments to account for the flipping of bounding boxes as discussed in comment 2.

Attachment #9030615 - Attachment is obsolete: true
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c949fbdda08f Additional testcases for correct bounding box of complex RTL text runs. r=heycam https://hg.mozilla.org/integration/autoland/rev/6b47fb297b39 Measure text by always laying out glyphs LTR. r=heycam
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Regressions: 1567397
Regressions: 1599841
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: