From 847cd8c6ca94e0ce587937c08cd23760812fe17d Mon Sep 17 00:00:00 2001 From: Tony Jin Date: Fri, 25 Aug 2017 12:50:38 -0700 Subject: [PATCH] Fix: Safari font rendering issue (#332) Should fix https://github.com/mozilla/pdf.js/issues/8700 and https://cloud.box.com/s/xd92oda655j808n0zvklga0qt2tltn6v on Safari --- src/lib/Browser.js | 53 ++++++++++++------- src/lib/__tests__/Browser-test.js | 32 ++++++++--- src/lib/viewers/doc/DocBaseViewer.js | 2 +- .../doc/__tests__/DocBaseViewer-test.js | 12 ++--- 4 files changed, 66 insertions(+), 33 deletions(-) diff --git a/src/lib/Browser.js b/src/lib/Browser.js index 6fb914dad..c507f0c80 100644 --- a/src/lib/Browser.js +++ b/src/lib/Browser.js @@ -61,7 +61,7 @@ class Browser { * @public * @param {string} type - The mime type to check. * @param {string} probability - Should either be 'maybe' or 'probably' - * @return {boolean} true if browser supports a particular type + * @return {boolean} True if browser supports a particular type */ static canPlayType(type, probability) { let elem; @@ -84,7 +84,7 @@ class Browser { * * @public * @param {string} mime - information about the AVC profile codec - * @return {boolean} true if browser supports HTML5 H264 main video playback + * @return {boolean} True if browser supports HTML5 H264 main video playback */ static canPlayH264(mime) { return Browser.canPlayType(mime, 'maybe') || Browser.canPlayType(mime, 'probably'); @@ -97,7 +97,7 @@ class Browser { * Also see {@link https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats|MDN} * * @public - * @return {boolean} true if browser supports HTML5 H264 baseline video playback + * @return {boolean} True if browser supports HTML5 H264 baseline video playback */ static canPlayH264Baseline() { return Browser.canPlayH264(MIME_H264_BASELINE); @@ -110,7 +110,7 @@ class Browser { * Also see {@link https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats|MDN} * * @public - * @return {boolean} true if browser supports HTML5 H264 main video playback + * @return {boolean} True if browser supports HTML5 H264 main video playback */ static canPlayH264Main() { return Browser.canPlayH264(MIME_H264_MAIN); @@ -123,7 +123,7 @@ class Browser { * Also see {@link https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats|MDN} * * @public - * @return {boolean} true if browser supports HTML5 H264 high video playback + * @return {boolean} True if browser supports HTML5 H264 high video playback */ static canPlayH264High() { return Browser.canPlayH264(MIME_H264_HIGH); @@ -137,7 +137,7 @@ class Browser { * Also see {@link https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats|MDN} * * @public - * @return {boolean} true if browser supports HTML5 MP3 audio playback + * @return {boolean} True if browser supports HTML5 MP3 audio playback */ static canPlayMP3() { return Browser.canPlayType('audio/mpeg', 'maybe') || Browser.canPlayType('audio/mpeg', 'probably'); @@ -149,7 +149,7 @@ class Browser { * to the H264 container (since we use H264 and not webm) * * @public - * @return {boolean} true if dash is usable + * @return {boolean} True if dash is usable */ static canPlayDash() { const mse = global.MediaSource; @@ -168,7 +168,7 @@ class Browser { * Checks the browser for Media Source Extensions support * * @public - * @return {boolean} true if MediaSource extensions are enabled + * @return {boolean} True if MediaSource extensions are enabled */ static hasMSE() { return !!global.MediaSource; @@ -178,7 +178,7 @@ class Browser { * Returns true if the browser supports webgl or experimental webgl * * @public - * @return {boolean} - returns true if the browser supports WebGL + * @return {boolean} True if the browser supports WebGL */ static hasWebGL() { if (!gl) { @@ -226,7 +226,7 @@ class Browser { * the Box3DRuntime for displaying Model Preview * * @public - * @return {boolean} true if browser fully supports Model Previewing + * @return {boolean} True if browser fully supports Model Previewing */ static supportsModel3D() { if (!Browser.hasWebGL()) { @@ -242,7 +242,7 @@ class Browser { * Determines if flash is installed. * * @public - * @return {boolean} true if browser has flash + * @return {boolean} True if browser has flash */ static hasFlash() { let hasFlash = false; @@ -258,7 +258,7 @@ class Browser { * Returns true if the browser supports SVG * * @public - * @return {boolean} is svg supported + * @return {boolean} True if svg supported */ static hasSVG() { return document.implementation.hasFeature('http://www.w3.org/TR/SVG11/feature#BasicStructure', '1.1'); @@ -279,7 +279,7 @@ class Browser { * Returns whether the browser is a mobile browser. * * @public - * @return {boolean} true if browser supports download + * @return {boolean} True if browser supports download */ static isMobile() { // Relying on the user agent to avoid desktop browsers on machines with touch screens. @@ -292,7 +292,7 @@ class Browser { * Currently not supported by IE11 or Safari 10.0 http://caniuse.com/#feat=download * * @public - * @return {boolean} true if browser supports download + * @return {boolean} True if browser supports download */ static canDownload() { return !Browser.isMobile() || (!window.externalHost && 'download' in document.createElement('a')); @@ -302,7 +302,7 @@ class Browser { * Returns whether or not the device is running IOS * * @public - * @return {boolean} true if the device is running IOS + * @return {boolean} True if the device is running IOS */ static isIOS() { return /(iPad|iPhone|iPod)/g.test(userAgent); @@ -312,20 +312,33 @@ class Browser { * Returns whether or not the device is running Android * * @public - * @return {boolean} true if the device is running Android + * @return {boolean} True if the device is running Android */ static isAndroid() { return /Android/g.test(userAgent); } /** - * Returns whether or not the device is running IOS 10.3.x that has Font Ligature rendering issue. + * Returns whether or not the device is a laptop/desktop Mac * * @public - * @return {boolean} Whether device is running 10.3.x + * @return {boolean} Whether device is a Mac */ - static isIOSWithFontIssue() { - return Browser.isIOS() && /(?:OS\s)10_3/i.test(userAgent); + static isMac() { + return /Macintosh; Intel Mac OS X/g.test(userAgent); + } + + /** + * Returns whether or not the device is running IOS 10.3.x or browser is desktop Safari, both of which have Font + * Ligature rendering issues due to the font loading API. + * + * @public + * @return {boolean} Whether device or browser have font ligature issues + */ + static hasFontIssue() { + return ( + (Browser.isIOS() && /(?:OS\s)10_3/i.test(userAgent)) || (Browser.isMac() && Browser.getName() === 'Safari') + ); } } diff --git a/src/lib/__tests__/Browser-test.js b/src/lib/__tests__/Browser-test.js index d5d7c0358..50db5c59a 100644 --- a/src/lib/__tests__/Browser-test.js +++ b/src/lib/__tests__/Browser-test.js @@ -461,22 +461,42 @@ describe('lib/Browser', () => { }); }); - describe('isIOSWithFontIssue()', () => { + describe('isMac()', () => { + it('should return true if device is a Mac', () => { + Browser.overrideUserAgent('(Macintosh; Intel Mac OS X 10_10_4)'); + const mac = Browser.isMac(); + expect(mac).to.be.true; + }); + + it('should return false if device is not a Mac', () => { + Browser.overrideUserAgent('(Windows NT 6.1; Win64; x64; rv:47.0)'); + const mac = Browser.isMac(); + expect(mac).to.be.false; + }); + }); + + describe('hasFontIssue()', () => { it('should return true if device is on ios and is OS 10.3.XX', () => { Browser.overrideUserAgent('iPhone OS 10_3_90 safari/2'); - const hasIssue = Browser.isIOSWithFontIssue(); + const hasIssue = Browser.hasFontIssue(); expect(hasIssue).to.be.true; }); it('should return false if device is on ios and is not OS 10.3.XX', () => { Browser.overrideUserAgent('iPhone OS 10_5_90 safari/2'); - const hasIssue = Browser.isIOSWithFontIssue(); + const hasIssue = Browser.hasFontIssue(); expect(hasIssue).to.be.false; }); - it('should return false if device is on ios and is not mobile', () => { - Browser.overrideUserAgent('DesktopDevice OS 10_3_90 safari/18902374701347589235'); - const hasIssue = Browser.isAndroid(); + it('should return true if device is a Mac running Safari', () => { + Browser.overrideUserAgent('Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/600.7.12 (KHTML, like Gecko) Version/8.0.7 Safari/600.7.12'); + const hasIssue = Browser.hasFontIssue(); + expect(hasIssue).to.be.true; + }); + + it('should return false if device is a Mac and not on Safari', () => { + Browser.overrideUserAgent('Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.90 Safari/537.36'); + const hasIssue = Browser.hasFontIssue(); expect(hasIssue).to.be.false; }); }); diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index e0fd215e2..a96419b36 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -549,7 +549,7 @@ class DocBaseViewer extends BaseViewer { // Disable font faces on IOS 10.3.X // @NOTE(JustinHoldstock) 2017-04-11: Check to remove this after next IOS release after 10.3.1 - PDFJS.disableFontFace = PDFJS.disableFontFace || Browser.isIOSWithFontIssue(); + PDFJS.disableFontFace = PDFJS.disableFontFace || Browser.hasFontIssue(); // Disable range requests for files smaller than MINIMUM_RANGE_REQUEST_FILE_SIZE (25MB) for // previews outside of the US since the additional latency overhead per range request can be diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js index 3bd2a7cb5..0608a041b 100644 --- a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js @@ -842,14 +842,14 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { expect(PDFJS.externalLinkRel).to.equal('noopener noreferrer nofollow'); }); - // @NOTE(JustinHoldstock) 2017-04-11: Check to remove this after next IOS release after 10.3.1 - it('should test user agent if on Safari Mobile for IOS 10.3', () => { - const getStub = sandbox.stub(Browser, 'isIOSWithFontIssue').returns(true); + // @NOTE(JustinHoldstock) 2017-04-11: Check to remove or modify this after next IOS release after 10.3.1 or + // Safari version + it('should test if browser has font rendering issue', () => { + PDFJS.disableFontFace = false; + sandbox.mock(Browser).expects('hasFontIssue').returns(true); + docBase.setupPdfjs(); - // Mobile stub cannot be called if get stub is never called. - // See note for this test, for more info. - expect(getStub).to.be.called; expect(PDFJS.disableFontFace).to.be.true; });