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

don't disable subpixel positioning for a text run when it is requested #3317

Merged
merged 2 commits into from
Nov 21, 2018

Conversation

lsalzman
Copy link
Contributor

@lsalzman lsalzman commented Nov 16, 2018

This resolves Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1506053

Gecko is generating metrics with DWrite assuming subpixel positioning. So if it requests WR use subpixel position, we have to honor it, otherwise the rendered glyphs will not match up. It's not really tenable to just disable subpixel positioning on the metrics side, since that requires using entirely different render modes (GDI vs. ClearType) which may significantly impact appearance.

The line that disabled subpixel positioning did not even previously exist till I accidentally added it in 58ccf52

This just undoes that mistake.


This change is Reviewable

@nical
Copy link
Contributor

nical commented Nov 16, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 3fdfef9 has been approved by nical

bors-servo pushed a commit that referenced this pull request Nov 16, 2018
don't disable subpixel positioning for a text run when it is requested

This resolves Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1506053

Gecko is generating metrics with DWrite assuming subpixel positioning. So if it requests WR use subpixel position, we have to honor it, otherwise the rendered glyphs will not match up. It's not really tenable to just disable subpixel positioning on the metrics side, since that requires using entirely different render modes (GDI vs. ClearType) which may significantly impact appearance.

The line that disabled subpixel positioning did not even previously exist till I accidentally added it in 58ccf52

This just undoes that mistake.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3317)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Testing commit 3fdfef9 with merge d552887...

@bors-servo
Copy link
Contributor

💔 Test failed - status-appveyor

@gw3583
Copy link
Contributor

gw3583 commented Nov 20, 2018

REFTEST INFO | 238 passing, 1 failing
Reftests with unexpected results:
	reftests\text\large-glyphs.yaml != reftests\text\blank.yaml
WARN: rx::HLSLCompiler::compileToBinary(228): 

@gw3583
Copy link
Contributor

gw3583 commented Nov 21, 2018

@lsalzman Looks like a failing reftest on CI.

@gw3583
Copy link
Contributor

gw3583 commented Nov 21, 2018

@bors-servo r=nical

@bors-servo
Copy link
Contributor

📌 Commit 1c36253 has been approved by nical

@bors-servo
Copy link
Contributor

⌛ Testing commit 1c36253 with merge 5dba919...

bors-servo pushed a commit that referenced this pull request Nov 21, 2018
don't disable subpixel positioning for a text run when it is requested

This resolves Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1506053

Gecko is generating metrics with DWrite assuming subpixel positioning. So if it requests WR use subpixel position, we have to honor it, otherwise the rendered glyphs will not match up. It's not really tenable to just disable subpixel positioning on the metrics side, since that requires using entirely different render modes (GDI vs. ClearType) which may significantly impact appearance.

The line that disabled subpixel positioning did not even previously exist till I accidentally added it in 58ccf52

This just undoes that mistake.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3317)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: nical
Pushing 5dba919 to master...

@bors-servo bors-servo merged commit 1c36253 into servo:master Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants