Cross-browser shield text #1010

ZeLonewolf · 2023-12-19T04:08:51Z

Fixes #625

This PR fixes differences between chrome and webkit by using fudge factors.

It also fixes several "I don't know how this even worked before" bugs in the banner layout code, and does some related typescript cleanup in the affected routines.

They are not pixel-perfect between browsers but they're very close. Tested on Ubuntu.

Webkit

Chrome

Firefox

ZeLonewolf · 2023-12-19T04:36:37Z

shieldlib/src/shield_banner.ts

-    top: r.options.bannerPadding,
+    top: 0,


Removed because the padding is accomplished by placing a gap between successive layouts, not by buffering the layout itself.

ZeLonewolf · 2023-12-19T04:36:57Z

shieldlib/src/shield_banner.ts

-        bannerIndex * r.px(r.options.bannerHeight - r.options.bannerPadding)
+        bannerIndex * r.px(r.options.bannerHeight + r.options.bannerPadding)


I'm not even sure how this code worked with this bug.

ZeLonewolf · 2023-12-19T04:39:28Z

shieldlib/src/shield_text.ts

-  var textWidth = metrics.width;
-  var textHeight = metrics.actualBoundingBoxDescent;
+  let textWidth: number =
+    Math.abs(metrics.actualBoundingBoxLeft) +
+    Math.abs(metrics.actualBoundingBoxRight);
+  let textHeight: number =
+    Math.abs(metrics.actualBoundingBoxDescent) +
+    Math.abs(metrics.actualBoundingBoxAscent);

-  var availHeight = bounds.height - padTop - padBot;
-  var availWidth = bounds.width - padLeft - padRight;
+  //Adjust for excess descender text height across browsers
+  textHeight *= 0.9;

-  var xBaseline = padLeft + availWidth / 2;
+  //Adjust for excess text height measured in Webkit engine specifically
+  if (isRunningInWebKit()) {
+    textHeight *= 0.54;
+  }
+
+  let availHeight: number = bounds.height - padTop - padBot;
+  let availWidth: number = bounds.width - padLeft - padRight;
+
+  let xBaseline: number = padLeft + availWidth / 2;


This is the main fix. We check both ascent/descent and left/right to get height/width, and then we make empirical adjustments to account for how the different browsers actually handle these fonts.

This makes me wonder if WebKit is reporting the full line height as the computer text height.

ZeLonewolf · 2023-12-19T04:39:55Z

shieldlib/src/shield_text.ts

-  let textConstraint = textLayoutFunc(
-    { height: availHeight, width: availWidth },
-    { height: textHeight, width: textWidth },
+  let spaceAvail: Dimension = { height: availHeight, width: availWidth };
+  let measuredTextBounds: Dimension = { height: textHeight, width: textWidth };
+
+  let textConstraint: TextTransform = textLayoutFunc(
+    spaceAvail,
+    measuredTextBounds,


Just a nicety to add typescript typing and make it clear what's happening.

ZeLonewolf · 2023-12-19T04:40:25Z

shieldlib/src/shield_text.ts

+  ctx.textAlign = "left";
  ctx.textBaseline = "top";


top/left makes the positioning math a bit easier and more consistent.

zekefarwell · 2023-12-19T19:03:50Z

I tested this out on Mac OS 13, Windows 11, iOS 17, and Android 14. Looks quite consistent across all the browsers I tested! Examples of the interstate shield only for a focused comparison:

Chrome on Windows

Chrome on Mac

Chrome on Android

Safari on Mac

Safari on iOS

Firefox on Windows

Firefox on Mac

Firefox on Android

Definitely some vertical padding adjustment needed for this and other shields, but that can probably be a follow up PR. Another issue I'm seeing is Chrome on Windows getting the wide shield blank for 2 digit refs. On the plus side, Safari and Firefox on Windows get the narrow shield blank for 111.

ZeLonewolf · 2023-12-19T19:53:44Z

Definitely some vertical padding adjustment needed for this and other shields, but that can probably be a follow up PR. Another issue I'm seeing is Chrome on Windows getting the wide shield blank for 2 digit refs. On the plus side, Safari and Firefox on Windows get the narrow shield blank for 111.

It looks like that only happens for the slightly wider 2-digit numbers, so I suspect I can just micro-adjust that threshold and achieve the intended effect.

ZeLonewolf · 2023-12-20T02:03:34Z

Reducing the threshold from 12 to 11.8 seems to be enough for the right 2-digit to 3-digit transition on Windows/Chrome.

zekefarwell · 2023-12-21T02:02:17Z

Reducing the threshold from 12 to 11.8 seems to be enough for the right 2-digit to 3-digit transition on Windows/Chrome

Looks like it. I can no longer reproduce. 👍

I noticed a couple of regressions that it may may be nice to resolve before merging this. The banner text is larger on this branch than on main. Also in some cases the banner text size changes between the narrow and wide variants of a certain shield.

This PR

Main branch

Some shields have a thicker top border on this branch that isn't present on main. I'm only seeing this issue on (some) bannered shields so it may be related to the banner text somehow.

This PR

Main Branch

claysmalley · 2023-12-21T02:14:49Z

Some shields have a thicker top border on this branch that isn't present on main. I'm only seeing this issue on (some) bannered shields so it may be related to the banner text somehow.

This appears to be affecting shields generated by draw functions. SVG shields look okay.

ZeLonewolf · 2023-12-22T03:40:04Z

755117b fixes the weirdness we were seeing with some shields. I simplified the code that draws shields when banners are present and removed an extra draw that wasn't supposed to be there.

1ec5

Looks better than before in MobileSafari:

1ec5 · 2024-01-12T22:33:14Z

shieldlib/src/shield_text.ts

-  var textWidth = metrics.width;
-  var textHeight = metrics.actualBoundingBoxDescent;
+  let textWidth: number =
+    Math.abs(metrics.actualBoundingBoxLeft) +
+    Math.abs(metrics.actualBoundingBoxRight);
+  let textHeight: number =
+    Math.abs(metrics.actualBoundingBoxDescent) +
+    Math.abs(metrics.actualBoundingBoxAscent);

-  var availHeight = bounds.height - padTop - padBot;
-  var availWidth = bounds.width - padLeft - padRight;
+  //Adjust for excess descender text height across browsers
+  textHeight *= 0.9;

-  var xBaseline = padLeft + availWidth / 2;
+  //Adjust for excess text height measured in Webkit engine specifically
+  if (isRunningInWebKit()) {
+    textHeight *= 0.54;
+  }
+
+  let availHeight: number = bounds.height - padTop - padBot;
+  let availWidth: number = bounds.width - padLeft - padRight;
+
+  let xBaseline: number = padLeft + availWidth / 2;


This makes me wonder if WebKit is reporting the full line height as the computer text height.

ZeLonewolf added 5 commits December 18, 2023 18:42

typing

6b7bb43

Cross-browser compatibility

7a7b88c

Merge branch 'main' into zlw-shield-text-sizing

3c30576

Merge branch 'main' into zlw-shield-text-sizing

3dc5a69

Add check for undefined window (for test cases)

6d461de

ZeLonewolf marked this pull request as ready for review December 19, 2023 04:35

ZeLonewolf commented Dec 19, 2023

View reviewed changes

Revert minor spacing

de78690

ZeLonewolf commented Dec 19, 2023

View reviewed changes

ZeLonewolf mentioned this pull request Dec 19, 2023

Consistent shield text layout #768

Closed

ZeLonewolf added 5 commits December 19, 2023 20:08

Slightly reduce 2->3 shield threshold to 11.9px

c19fb5f

Slightly reduce 2->3 shield threshold to 11.8px

e2e235f

Slightly reduce 2->3 shield threshold to 11.5px

29cc67a

Slightly reduce 2->3 shield threshold to 11px

002c7ca

Change 2->3 shield threshold to 11.8px

1c65cc6

ZeLonewolf requested review from claysmalley and zekefarwell and removed request for claysmalley December 20, 2023 02:03

claysmalley added enhancement New feature or request shield-generator Issues specific to the shield library labels Dec 20, 2023

Merge branch 'main' into zlw-shield-text-sizing

fb0b720

claysmalley removed the enhancement New feature or request label Dec 21, 2023

claysmalley added the bug Something isn't working label Dec 21, 2023

ZeLonewolf added 2 commits December 20, 2023 22:52

Refactor math expression

d1eaa59

Fix bad programming decisions from past Brian

755117b

ZeLonewolf requested a review from claysmalley December 22, 2023 03:39

Merge branch 'main' into zlw-shield-text-sizing

6574649

ZeLonewolf mentioned this pull request Dec 23, 2023

Shield text rendering: width adjustments and Overpass font #544

Closed

ZeLonewolf added 4 commits December 24, 2023 12:06

Merge branch 'main' into zlw-shield-text-sizing

b714631

Merge branch 'main' into zlw-shield-text-sizing

eddfc6a

Merge branch 'main' into zlw-shield-text-sizing

1cf23a5

Merge branch 'main' into zlw-shield-text-sizing

e54196c

1ec5 approved these changes Jan 12, 2024

View reviewed changes

ZeLonewolf merged commit 71c9d4c into main Jan 12, 2024
6 checks passed

ZeLonewolf deleted the zlw-shield-text-sizing branch January 12, 2024 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-browser shield text #1010

Cross-browser shield text #1010

ZeLonewolf commented Dec 19, 2023 •

edited

Loading

ZeLonewolf Dec 19, 2023

ZeLonewolf Dec 19, 2023

ZeLonewolf Dec 19, 2023

1ec5 Jan 12, 2024

ZeLonewolf Dec 19, 2023

ZeLonewolf Dec 19, 2023

zekefarwell commented Dec 19, 2023 •

edited by ZeLonewolf

Loading

ZeLonewolf commented Dec 19, 2023

ZeLonewolf commented Dec 20, 2023

zekefarwell commented Dec 21, 2023 •

edited by ZeLonewolf

Loading

claysmalley commented Dec 21, 2023

ZeLonewolf commented Dec 22, 2023 •

edited

Loading

1ec5 left a comment •

edited by ZeLonewolf

Loading

1ec5 Jan 12, 2024

		bannerIndex * r.px(r.options.bannerHeight - r.options.bannerPadding)
		bannerIndex * r.px(r.options.bannerHeight + r.options.bannerPadding)

Cross-browser shield text #1010

Cross-browser shield text #1010

Conversation

ZeLonewolf commented Dec 19, 2023 • edited Loading

ZeLonewolf Dec 19, 2023

Choose a reason for hiding this comment

ZeLonewolf Dec 19, 2023

Choose a reason for hiding this comment

ZeLonewolf Dec 19, 2023

Choose a reason for hiding this comment

1ec5 Jan 12, 2024

Choose a reason for hiding this comment

ZeLonewolf Dec 19, 2023

Choose a reason for hiding this comment

ZeLonewolf Dec 19, 2023

Choose a reason for hiding this comment

zekefarwell commented Dec 19, 2023 • edited by ZeLonewolf Loading

ZeLonewolf commented Dec 19, 2023

ZeLonewolf commented Dec 20, 2023

zekefarwell commented Dec 21, 2023 • edited by ZeLonewolf Loading

claysmalley commented Dec 21, 2023

ZeLonewolf commented Dec 22, 2023 • edited Loading

1ec5 left a comment • edited by ZeLonewolf Loading

Choose a reason for hiding this comment

1ec5 Jan 12, 2024

Choose a reason for hiding this comment

ZeLonewolf commented Dec 19, 2023 •

edited

Loading

zekefarwell commented Dec 19, 2023 •

edited by ZeLonewolf

Loading

zekefarwell commented Dec 21, 2023 •

edited by ZeLonewolf

Loading

ZeLonewolf commented Dec 22, 2023 •

edited

Loading

1ec5 left a comment •

edited by ZeLonewolf

Loading