gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. #10937

pcwalton · 2016-04-30T00:52:29Z

This matches what I believe the OS native defaults to be.

Partially addresses #9487.

r? @metajack
cc @paulrouget

This change is

highfive · 2016-04-30T00:52:52Z

Warning

These commits modify gfx code, but no tests are modified. Please consider adding a test!

metajack · 2016-05-01T21:03:25Z

@bors-servo r+

I'm curious why we aren't using font-config for this mapping since that seems to be its function. But this looks good.

bors-servo · 2016-05-01T21:03:27Z

📌 Commit c46c59a has been approved by metajack

bors-servo · 2016-05-01T21:03:29Z

⌛ Testing commit c46c59a with merge 727bf7a...


        Auto merge of #10937 - pcwalton:mac-helvetica, r=metajack

@metajack

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. This matches what I believe the OS native defaults to be. Partially addresses #9487. r? @metajack cc @paulrouget  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10937)

bors-servo · 2016-05-01T21:50:32Z

💔 Test failed - android

pcwalton · 2016-05-02T17:57:03Z

r? @metajack

Added Android.

metajack · 2016-05-03T04:13:24Z

@bors-servo r+

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-05-03T04:13:25Z

📌 Commit bcdb53e has been approved by metajack

bors-servo · 2016-05-03T04:17:32Z

⌛ Testing commit bcdb53e with merge 10f043d...


        Auto merge of #10937 - pcwalton:mac-helvetica, r=metajack

@metajack

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. This matches what I believe the OS native defaults to be. Partially addresses #9487. r? @metajack cc @paulrouget  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10937)

bors-servo · 2016-05-03T04:58:16Z

💔 Test failed - mac-rel-css

highfive · 2016-05-03T04:58:18Z

  ▶ FAIL [expected PASS] /css-text-3_dev/html/word-break-normal-bo-000.htm
  └   → /css-text-3_dev/html/word-break-normal-bo-000.htm 4f0df934a67a74b4001e44416adbd0870ca317ff
/css-text-3_dev/html/reference/word-break-normal-bo-ref-000.htm 094fa50bd8bc5fc0817433414d3548933e9ed251
Testing 4f0df934a67a74b4001e44416adbd0870ca317ff == 094fa50bd8bc5fc0817433414d3548933e9ed251

jdm · 2016-05-03T15:58:27Z

In the reference both boxes look like the second one:


        Auto merge of #10937 - pcwalton:mac-helvetica, r=metajack

@metajack

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. This matches what I believe the OS native defaults to be. Partially addresses #9487. r? @metajack cc @paulrouget  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10937)

bors-servo · 2016-05-06T06:35:44Z

💔 Test failed - mac-rel-wpt

highfive · 2016-05-06T06:35:46Z

  ▶ FAIL [expected PASS] /_mozilla/css/input_selection_incremental_a.html
  └   → /_mozilla/css/input_selection_incremental_a.html 77206f4ef5cf4394f733dae85b67e229dd8e8cc6
/_mozilla/css/input_selection_incremental_ref.html 91db21a5d4b097bc5bee206f0d4bd2c977457ff1
Testing 77206f4ef5cf4394f733dae85b67e229dd8e8cc6 == 91db21a5d4b097bc5bee206f0d4bd2c977457ff1

jdm · 2016-05-06T14:11:01Z

The input_selection_incremental tests use font-family: sans-serif and hasn't failed on any other PR before this. We'll need to address it before continuing to try to merge this.

metajack · 2016-05-06T14:32:25Z

@pcwalton Can you investigate?

highfive · 2016-05-09T19:45:11Z

New code was committed to pull request.


        gfx: Map `sans-serif` to Helvetica on Mac, DejaVu Sans on Linux, and

Roboto on Android. This matches what I believe the OS native defaults to be. Partially addresses #9487.

pcwalton · 2016-05-09T19:46:43Z

The test was failing because the line-height of Helvetica is different from that of Arial. Moving the font in the reference to an attribute on an inline-block to match the <input> element of the test fixed the problem. Note that the test was failing in Gecko for the same reason, and with this fix the test now passes in Gecko.

pcwalton · 2016-05-09T19:47:16Z

@bors-servo: r=metajack

bors-servo · 2016-05-09T19:47:17Z

📌 Commit 2ac9747 has been approved by metajack

metajack · 2016-05-09T19:48:35Z

So the test was only failing on one platform, and now it fails on all of them. Your comment seemed to imply we now pass.

Previously, bors-servo wrote…

📌 Commit 2ac9747 has been approved by metajack

Reviewed 1 of 1 files at r3.
Review status: 4 of 5 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-05-09T19:48:37Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #11090

bors-servo · 2016-05-09T20:02:25Z

⌛ Testing commit 2ac9747 with merge 180a981...


        Auto merge of #10937 - pcwalton:mac-helvetica, r=metajack

@metajack

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. This matches what I believe the OS native defaults to be. Partially addresses #9487. r? @metajack cc @paulrouget  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10937)

pcwalton · 2016-05-09T20:02:54Z

Hmm? Sorry, I meant the incremental input test, not the word break one.

metajack · 2016-05-09T20:06:57Z

Weird. Reviewable didn't show changes to the other test. Why does the word break test now also fail on OS X?

pcwalton · 2016-05-09T20:13:00Z

It was only accidentally passing before. It's a test of Indic script word breaking rules, and we don't display Indic fonts yet.

bors-servo · 2016-05-09T20:50:03Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned metajack Apr 30, 2016

highfive added the S-awaiting-review label Apr 30, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 1, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels May 1, 2016

pcwalton force-pushed the pcwalton:mac-helvetica branch from c46c59a to bcdb53e May 2, 2016

highfive added S-awaiting-review and removed S-tests-failed labels May 2, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 3, 2016

highfive removed the S-awaiting-merge label May 3, 2016

highfive added the S-tests-failed label May 3, 2016

pcwalton force-pushed the pcwalton:mac-helvetica branch from bcdb53e to 27b2419 May 4, 2016

highfive added S-awaiting-review and removed S-tests-failed labels May 4, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels May 6, 2016

highfive removed the S-awaiting-merge label May 6, 2016

highfive added the S-tests-failed label May 6, 2016

jdm mentioned this pull request May 6, 2016

Intermittent failure in /_mozilla/css/input_selection_incremental_a.html #11025

Closed

pcwalton force-pushed the pcwalton:mac-helvetica branch from 27b2419 to 2ac9747 May 9, 2016

highfive added S-awaiting-review and removed S-tests-failed labels May 9, 2016

gfx: Map `sans-serif` to Helvetica on Mac, DejaVu Sans on Linux, and

Loading status checks…

2ac9747

Roboto on Android. This matches what I believe the OS native defaults to be. Partially addresses #9487.

highfive added S-awaiting-merge and removed S-awaiting-review labels May 9, 2016

highfive removed the S-awaiting-merge label May 9, 2016

servo / servo

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. #10937

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. #10937

pcwalton commented Apr 30, 2016 •

edited by larsbergstrom

highfive commented Apr 30, 2016

metajack commented May 1, 2016

bors-servo commented May 1, 2016

bors-servo commented May 1, 2016

bors-servo commented May 1, 2016

pcwalton commented May 2, 2016

metajack commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 3, 2016

highfive commented May 3, 2016

jdm commented May 3, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

jdm commented May 6, 2016

metajack commented May 6, 2016

highfive commented May 9, 2016

pcwalton commented May 9, 2016

pcwalton commented May 9, 2016

bors-servo commented May 9, 2016

metajack commented May 9, 2016

bors-servo commented May 9, 2016

bors-servo commented May 9, 2016

pcwalton commented May 9, 2016

metajack commented May 9, 2016

pcwalton commented May 9, 2016

bors-servo commented May 9, 2016

servo / servo

Join GitHub today

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. #10937

gfx: Map `sans-serif` to Helvetica on Mac and DejaVu Sans on Linux. #10937

Conversation

pcwalton commented Apr 30, 2016 • edited by larsbergstrom

highfive commented Apr 30, 2016

metajack commented May 1, 2016

bors-servo commented May 1, 2016

bors-servo commented May 1, 2016

bors-servo commented May 1, 2016

pcwalton commented May 2, 2016

metajack commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 3, 2016

bors-servo commented May 3, 2016

highfive commented May 3, 2016

jdm commented May 3, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

jdm commented May 6, 2016

metajack commented May 6, 2016

highfive commented May 9, 2016

pcwalton commented May 9, 2016

pcwalton commented May 9, 2016

bors-servo commented May 9, 2016

metajack commented May 9, 2016

bors-servo commented May 9, 2016

bors-servo commented May 9, 2016

pcwalton commented May 9, 2016

metajack commented May 9, 2016

pcwalton commented May 9, 2016

bors-servo commented May 9, 2016

pcwalton commented Apr 30, 2016 •

edited by larsbergstrom