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.

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

gfx: Implement `letter-spacing` per CSS 2.1 § 16.4. #4325

Merged
merged 1 commit into from Dec 12, 2014

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Dec 11, 2014

The ligature disabling code has been manually verified, but I was unable
to reftest it. (The only way I could think of would be to create an
Ahem-like font with a ligature table, but that would be an awful lot of
work.)

Near as I can tell, the method used to apply the spacing (manually
inserting extra advance post-shaping) matches Gecko.

r? @SimonSapin

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Dec 11, 2014

Critic review: https://critic.hoppipolla.co.uk/r/3460

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@SimonSapin
Copy link
Member

SimonSapin commented Dec 11, 2014

The ligature disabling code has been manually verified, but I was unable to reftest it.

I’m not sure if this would work, but maybe you could have three files: one with "fi" (or some other ligature), one with "fi" that should be different from the first, and one with "fi" with letter spacing that should be like the latter?

… reading the spec, that might be tricky since ligatures are disabled "When the effective spacing between two characters is not zero", not just when letter-spacing is not normal. But then it’s only a "should", so maybe it’s not that important?

@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 11, 2014

Yeah, I saw that language in the spec and disabled ligatures if letter-spacing was 0 as well. I'm a little wary of violating "should" in the spec just for tests, especially since we can test it another way (just with a fair amount of mucking around in FontForge).

Furthermore, it's kind of hard to find a font that Servo will actually draw ligatures with right now. This is because of #4320 — we only support the newest of the new formats for ligatures, and most fonts use the old morx stuff.

@SimonSapin
Copy link
Member

SimonSapin commented Dec 11, 2014

Alright, let’s not block this PR on this, we can refine later.

@pcwalton pcwalton force-pushed the pcwalton:letter-spacing branch 2 times, most recently from cc414f0 to fcf7819 Dec 12, 2014
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 12, 2014

if (*font_and_shaping_options).options.flags.contains(IGNORE_LIGATURES_SHAPING_FLAG) &&
tag == GSUB {
debug!("ignoring ligatures, so ignoring `GSUB` table");
return ptr::null_mut()

This comment has been minimized.

@mbrubeck

mbrubeck Dec 12, 2014

Contributor

Disabling the GSUB table seems like a very blunt instrument. Can we disable ligatures only, by setting the "liga" feature to 0 when we call hb_shape?

@pcwalton pcwalton force-pushed the pcwalton:letter-spacing branch from fcf7819 to c72e2d3 Dec 12, 2014
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 12, 2014

I changed it to use HarfBuzz features. re-r? @mbrubeck

bors-servo pushed a commit that referenced this pull request Dec 12, 2014
The ligature disabling code has been manually verified, but I was unable
to reftest it. (The only way I could think of would be to create an
Ahem-like font with a ligature table, but that would be an awful lot of
work.)

Near as I can tell, the method used to apply the spacing (manually
inserting extra advance post-shaping) matches Gecko.

r? @SimonSapin
@jdm
Copy link
Member

jdm commented Dec 12, 2014

This is definitely not something that should be retried blindly. Can we please be more careful about looking at test failure logs?

bors-servo pushed a commit that referenced this pull request Dec 12, 2014
The ligature disabling code has been manually verified, but I was unable
to reftest it. (The only way I could think of would be to create an
Ahem-like font with a ligature table, but that would be an awful lot of
work.)

Near as I can tell, the method used to apply the spacing (manually
inserting extra advance post-shaping) matches Gecko.

r? @SimonSapin
The ligature disabling code has been manually verified, but I was unable
to reftest it. (The only way I could think of would be to create an
Ahem-like font with a ligature table, but that would be an awful lot of
work.)

Near as I can tell, the method used to apply the spacing (manually
inserting extra advance post-shaping) matches Gecko.
@pcwalton pcwalton force-pushed the pcwalton:letter-spacing branch from c72e2d3 to 07bc97e Dec 12, 2014
@pcwalton

This comment has been minimized.

Copy link
Owner Author

pcwalton commented on 07bc97e Dec 12, 2014

r=mbrubeck

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on 07bc97e Dec 12, 2014

saw approval from mbrubeck
at pcwalton@07bc97e

This comment has been minimized.

Copy link
Contributor

bors-servo replied Dec 12, 2014

merging pcwalton/servo/letter-spacing = 07bc97e into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Dec 12, 2014

pcwalton/servo/letter-spacing = 07bc97e merged ok, testing candidate = 914f272

This comment has been minimized.

Copy link
Contributor

bors-servo replied Dec 12, 2014

fast-forwarding master to auto = 914f272

bors-servo pushed a commit that referenced this pull request Dec 12, 2014
The ligature disabling code has been manually verified, but I was unable
to reftest it. (The only way I could think of would be to create an
Ahem-like font with a ligature table, but that would be an awful lot of
work.)

Near as I can tell, the method used to apply the spacing (manually
inserting extra advance post-shaping) matches Gecko.

r? @SimonSapin
@bors-servo bors-servo closed this Dec 12, 2014
@bors-servo bors-servo merged commit 07bc97e into servo:master Dec 12, 2014
1 check passed
1 check passed
default all tests passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.