970891 - Whitespace does not fall through font stack when between inline elements styled with font-*

Status: RESOLVED FIXED

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

Description

[Nick Evans]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140127194636

Steps to reproduce:

1. Open test.html with test.woff in the same directory.


Actual results:

Whitespace appears as .notdef if:
- The first font on the stack does not have a space glyph, e.g. a symbol font.
- The whitespace is between inline elements.
- Those inline elements are styled with font-* properties that are not set to "normal", e.g. font-style: italic.


Expected results:

The whitespace should fall through the font stack and appear as a space glyph from a lower font. This way the space width will match the letter glyphs. (If the symbol font imposes a space glyph then the space width will not match the letter glyphs.)

Comment 1

[Nick Evans]

Attachment: test.woff

Comment 2

[Nick Evans]

Attachment: screenshot.PNG

Comment 3

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/511078d51f71
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120105 Firefox/12.0a1 ID:20120105035122
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c0b62edd2917
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120105 Firefox/12.0a1 ID:20120105041225
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=511078d51f71&tochange=c0b62edd2917

Regressed by: bug 703100

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: don't assume the primary font will always support the <space> character - fall back down the font stack if necessary.

This arises because gfxFontGroup::MakeSpaceTextRun (rashly) assumes that the primary font in the font-group's list can be used for <space>; that's not necessarily true.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8374089 [details] [diff] [review]
don't assume the primary font will always support the <space> character - fall back down the font stack if necessary.

Review of attachment 8374089 [details] [diff] [review]:
-----------------------------------------------------------------

Reftest please!

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: reftest for font fallback on the <space> character.

Comment 7

[Nick Evans]

Did this patch slip through the net? (Please excuse the mixed metaphors.)

This issue remains in Nightly 30.0a1 (2014-02-23).

Comment 8

[Jonathan Kew [:jfkthame]]

I was intending to land the patch and test together, once the test is reviewed - Roc, ping?

Comment 9

[Nick Evans]

Jonathan, these messages don't seem to be reaching Robert. Is there another way you can contact him?

Comment 10

[Alice0775 White]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> I was intending to land the patch and test together, once the test is
> reviewed - Roc, ping?

Sorry, spam

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75d3146ac0d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c4304d45d7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0e1abb0b06

https://tbpl.mozilla.org/php/getParsedLog.php?id=35725469&tree=Mozilla-Inbound

Comment 13

[Ryan VanderMeulen [:RyanVM]]

And linux.
https://tbpl.mozilla.org/php/getParsedLog.php?id=35726388&tree=Mozilla-Inbound

Please run this through Try before pushing again.

Comment 14

[Jonathan Kew [:jfkthame]]

Tryserver says this is OK now:
https://tbpl.mozilla.org/?tree=Try&rev=3af789257964

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c83d1306f2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6a6fa1e025

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c83d1306f2a
https://hg.mozilla.org/mozilla-central/rev/4e6a6fa1e025