1655364 - Consolas text above a certain size does not get right-aligned properly

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Cameron McCormack (:heycam)]

I'm experimenting with Consolas as the default monospace font on Windows, and I find that layout/reftests/text/1522857-1.html fails. (It also fails if I modify the test to use Consolas instead of monospace.) It only fails, on my machine, when the font-size is at least 125px. This feels like a return of bug 1522857 but I couldn't reproduce this with various other fonts on my system.

Comment 1

[Cameron McCormack (:heycam)]

So it seems like we have a similar issue here, where the CompressedGlyph is not being marked as CHAR_IS_SPACE. When the text run is initially created, we do correctly set that flag, under gfxShapedText::SetupClusterBoundaries, but when we got to shape it, we overwrite the CompressedGlyph with a new value without CHAR_IS_SPACE. That's done by gfxHarfBuzzShaper::SetGlyphsFromRun calling gfxShapedText::SetGlyphs.

Comment 2

[Cameron McCormack (:heycam)]

I think most (but not all) callers of gfxShapedText::SetGlyphs do want to preserve the character type flags.

Comment 3

[Cameron McCormack (:heycam)]

Maybe the CAN_BREAK_BEFORE flags too?

Comment 4

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb58f84620cf15db49ca6216be29f87e239f8bd3

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Bug 1655364 - Don't clobber break and char type flags when adjusting glyphs after shaping.

Comment 6

[Jonathan Kew [:jfkthame]]

Here's a try push of the alternative approach I'd like to suggest.... let's see how it goes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40388e844f187637f22c701c25f16dd315f3ee06

Comment 7

[Jonathan Kew [:jfkthame]]

Looking at gfxShapedText::SetGlyphs and how it's used by the various callers, I think we should rework it so that it doesn't touch any of the character flags, and is only responsible for setting the DetailedGlyph data for the given index. In most cases, callers don't want to touch the other flags, and currently in some cases we have go to extra effort e.g. to read the existing IsClusterStart flag only so as to pass it back again as part of the CompressedGlyph.

So I propose renaming SetGlyphs to SetDetailedGlyphs, and passing in not a CompressedGlyph record (with flags that the caller probably didn't want to deal with at all) but just a glyph count and pointer to the DetailedGlyph records. I'll post a patch for your consideration.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1655364 - Rename gfxShapedText::SetGlyphs to SetDetailedGlyphs and make it just set glyph information, not clobber character-type flags. r=heycam

Comment 9

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c5a70b48fdf36fddf1c6a52e979299e290cb02

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c88adc13a06
Rename gfxShapedText::SetGlyphs to SetDetailedGlyphs and make it just set glyph information, not clobber character-type flags. r=heycam

Comment 11

[Alexandru Michis [:malexandru]]

Backed out changeset 3c88adc13a06 for causing assertion failures regarding glyphs.

Backout link: https://hg.mozilla.org/integration/autoland/rev/c218ff4e2a2c50b6b76a4372672e3aa0df7ce093

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=5362ccf3ec882d7a5ce5b659a1d7a673b0f1075d&searchStr=os%2Cx%2C10.14%2Cwebrender%2Cdebug%2Creftests%2Ctest-macosx1014-64-qr%2Fdebug-reftest-e10s%2Cr6&tochange=2d117fa859d8fd79811a745041335b5f3a9b620d

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311488771&repo=autoland&lineNumber=28669

Comment 12

[Jonathan Kew [:jfkthame]]

Ahh... reftests managed to find a case where I tripped over my own assertion; well, that's what it's there for! Now fixed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07f5cb328a10
Rename gfxShapedText::SetGlyphs to SetDetailedGlyphs and make it just set glyph information, not clobber character-type flags. r=heycam

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/07f5cb328a10