Set text baseline conditionally by browser. #3370

dstein64 · 2021-06-16T23:53:48Z

This changes the textBaseline from ideographic to bottom for Firefox, to resolve Issue #3353. I made changes in all places where middle was changed to ideographic as part of PR #3279. The change was not made for Chrome, to avoid Powerline fonts from becoming slightly unaligned. On Firefox, it appears the Poweline text is slightly high when I set the font to monospace and slightly low when I set it to DejaVu Sans Mono, but in both cases an improvement versus the low/truncated text when using ideographic for textBaseline prior to this PR's update, as the text is no longer truncated and the Powerline text is closer to vertically centered alignment.

Firefox fontFamily=monospace textBaseline=ideographic (before this PR's update)

Firefox fontFamily=monospace textBaseline=bottom (after this PR's update)

Firefox fontFamily=DejaVu Sans Mono textBaseline=ideographic (before this PR's update)

Firefox fontFamily=DejaVu Sans Mono textBaseline=bottom (after this PR's update)

In my testing, the update worked as expected (other than the possible slight Powerline alignment issue mentioned above). However, I don't know whether there are assumptions elsewhere that this would break, and I wasn't sure where in the code would be an appropriate place for defining TEXT_BASELINE, which is currently in browser/renderer/atlas/Constants.ts.

Any feedback would be appreciated if this is a sufficient way to address Issue #3353 and there is further testing I should conduct and/or additional changes that should be made.

chkaos · 2021-07-12T10:30:36Z

@dstein64 Great styling, how did you do that?

dstein64 · 2021-07-13T16:53:24Z

@chkaos, the prompt styling is from Powerline, used here specifically to test that the update from PR #3279 would still apply as intended (that PR fixed an issue with the display of Powerline fonts).

Tyriar

I think we used to use top instead of bottom to avoid this cut off:

dstein64 · 2021-07-13T21:45:24Z

Was the cutoff in that image generated using this branch? I'm unable to reproduce truncated text when using the update from this PR.

"I think we used to use top instead of bottom to avoid this cut off:"

Changing bottom to top on this branch with no other changes results in no text showing. Is the change request to 1) switch all browsers to use top instead of ideographic along with necessary corresponding updates, 2) switch Firefox to top along with necessary updates only for Firefox (ideographic would continue to be used for other browsers), or 3) some other modification?

Tyriar · 2021-08-31T14:35:11Z

Sorry about the delay, option 2; ideographic is fine but any uses of bottom need to switch to top.

nirui · 2021-09-04T10:20:28Z

Based on the information on https://www.w3schools.com/Tags/canvas_textbaseline.asp, changing bottom to top requires some rather big changes to the text render (text alignment calculations etc), since bottom is basically the opposite of top in terms of vertical text positioning.

Also on the same page, it mentioned:

Note: The textBaseline property works differently in different browsers, especially when using "hanging" or "ideographic".

dstein64 · 2021-09-04T18:49:24Z

@Tyriar, can you provide details on how you generated the screenshot in your earlier comment? I am unable to reproduce that behavior with the update on this branch (where bottom is used for the textBaseline on Firefox).

Tyriar · 2021-10-22T18:13:30Z

Late response, but the screenshot above repro for me again:

Windows 10
Firefox 93.0
Default font (so courier new on my machine)

dstein64 · 2021-10-22T19:30:11Z

Thanks for the details @Tyriar!

I'm still not able to reproduce what you're encountering when using this branch.

I loaded this branch using Firefox 93 on Windows 11. I then ran yarn followed by yarn start.

On the demo page, I typed text and did not encounter any truncated characters.

I then incorporated the latest changes upstream (now force-pushed to this branch), and still don't encounter the issue.

In the example screenshot you posted, it looks like you've loaded cmd.exe with xterm.js. Perhaps the different ways we're testing is causing the difference in results. What steps can I take to match your testing procedure?

Tyriar · 2021-10-22T19:46:14Z

I just ran the demo, not sure why your demo isn't using the real backend which should connect to cmd.exe. Maybe you're missing deps to compile node-pty? But anyway, I doubt that would cause changes in rendering.

dstein64 · 2021-10-23T04:50:06Z

"not sure why your demo isn't using the real backend which should connect to cmd.exe"

I was using Firefox on Windows as a client for the demo, but the server was running on Linux.

I just tried running the demo directly from Windows, following the instructions here, but I'm encountering the following error when running yarn.

gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin\MSBuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\lib\build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (node:events:390:28)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
gyp ERR! System Windows_NT 10.0.19043
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd C:\Users\dan\xterm.js\node_modules\node-pty
gyp ERR! node -v v17.0.0
gyp ERR! node-gyp -v v8.2.0

Earlier in the output is another error:

C:\Users\dan\AppData\Local\node-gyp\Cache\17.0.0\include\node\v8-persistent-handle.h(10,10): fatal error C1083: Cannot open include file: 'v8-weak-callback-info.h': No such file or directory [C:\Users\dan\xterm.js\node_modules\node-pty\build\conpty.vcxproj]
  conpty_console_list.cc
C:\Users\dan\AppData\Local\node-gyp\Cache\17.0.0\include\node\v8-persistent-handle.h(10,10): fatal error C1083: Cannot open include file: 'v8-weak-callback-info.h': No such file or directory [C:\Users\dan\xterm.js\node_modules\node-pty\build\conpty_console_list.vcxproj]

Full Output

C:\Users\dan\xterm.js>yarn
yarn install v1.22.15
[1/4] Resolving packages...
[2/4] Fetching packages...
info [email protected]: The platform "win32" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
[1/3] ⡀ node-pty
[-/3] ⢀ waiting...
error C:\Users\dan\xterm.js\node_modules\node-pty: Command failed.
Exit code: 1
Command: node scripts/install.js
Arguments:
Directory: C:\Users\dan\xterm.js\node_modules\node-pty
Output:
C:\Users\dan\xterm.js\node_modules\node-pty>if not defined npm_config_node_gyp (node "C:\Program Files\nodejs\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "" rebuild )
gyp info it worked if it ends with ok
gyp info using [email protected]
gyp info using [email protected] | win32 | x64
gyp info find Python using Python version 3.9.7 found at "C:\Users\dan\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.9_qbz5n2kfra8p0\python.exe"
gyp info find VS using VS2019 (16.11.31729.503) found at:
gyp info find VS "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community"
gyp info find VS run with --verbose for detailed information
gyp info spawn C:\Users\dan\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.9_qbz5n2kfra8p0\python.exe
gyp info spawn args [
gyp info spawn args   'C:\\Program Files\\nodejs\\node_modules\\npm\\node_modules\\node-gyp\\gyp\\gyp_main.py',
gyp info spawn args   'binding.gyp',
gyp info spawn args   '-f',
gyp info spawn args   'msvs',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\dan\\xterm.js\\node_modules\\node-pty\\build\\config.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Program Files\\nodejs\\node_modules\\npm\\node_modules\\node-gyp\\addon.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\dan\\AppData\\Local\\node-gyp\\Cache\\17.0.0\\include\\node\\common.gypi',
gyp info spawn args   '-Dlibrary=shared_library',
gyp info spawn args   '-Dvisibility=default',
gyp info spawn args   '-Dnode_root_dir=C:\\Users\\dan\\AppData\\Local\\node-gyp\\Cache\\17.0.0',
gyp info spawn args   '-Dnode_gyp_dir=C:\\Program Files\\nodejs\\node_modules\\npm\\node_modules\\node-gyp',
gyp info spawn args   '-Dnode_lib_file=C:\\\\Users\\\\dan\\\\AppData\\\\Local\\\\node-gyp\\\\Cache\\\\17.0.0\\\\<(target_arch)\\\\node.lib',
gyp info spawn args   '-Dmodule_root_dir=C:\\Users\\dan\\xterm.js\\node_modules\\node-pty',
gyp info spawn args   '-Dnode_engine=v8',
gyp info spawn args   '--depth=.',
gyp info spawn args   '--no-parallel',
gyp info spawn args   '--generator-output',
gyp info spawn args   'C:\\Users\\dan\\xterm.js\\node_modules\\node-pty\\build',
gyp info spawn args   '-Goutput_dir=.'
gyp info spawn args ]
gyp info spawn C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin\MSBuild.exe
gyp info spawn args [
gyp info spawn args   'build/binding.sln',
gyp info spawn args   '/clp:Verbosity=minimal',
gyp info spawn args   '/nologo',
gyp info spawn args   '/p:Configuration=Release;Platform=x64'
gyp info spawn args ]
Building the projects in this solution one at a time. To enable parallel build, please add the "-m" switch.
  conpty.cc
C:\Users\dan\AppData\Local\node-gyp\Cache\17.0.0\include\node\v8-persistent-handle.h(10,10): fatal error C1083: Cannot open include file: 'v8-weak-callback-info.h': No such file or directory [C:\Users\dan\xterm.js\node_modules\node-pty\build\conpty.vcxproj]
  conpty_console_list.cc
C:\Users\dan\AppData\Local\node-gyp\Cache\17.0.0\include\node\v8-persistent-handle.h(10,10): fatal error C1083: Cannot open include file: 'v8-weak-callback-info.h': No such file or directory [C:\Users\dan\xterm.js\node_modules\node-pty\build\conpty_console_list.vcxproj]
  Agent.cc
  AgentCreateDesktop.cc
  ConsoleFont.cc
  ConsoleInput.cc
  ConsoleInputReencoding.cc
  ConsoleLine.cc
  DebugShowInput.cc
  DefaultInputMap.cc
  EventLoop.cc
  InputMap.cc
  LargeConsoleRead.cc
  NamedPipe.cc
  Scraper.cc
  Terminal.cc
  Win32Console.cc
  Win32ConsoleBuffer.cc
  main.cc
  BackgroundDesktop.cc
  Buffer.cc
  DebugClient.cc
  GenRandom.cc
  OwnedHandle.cc
  StringUtil.cc
  WindowsSecurity.cc
  WindowsVersion.cc
  WinptyAssert.cc
  WinptyException.cc
  WinptyVersion.cc
  win_delay_load_hook.cc
  Generating code
  Previous IPDB not found, fall back to full compilation.
C:\Users\dan\xterm.js\node_modules\node-pty\deps\winpty\src\agent\Agent.cc(231): warning C4722: 'Agent::~Agent': destructor never returns, potential memory leak [C:\Users\dan\xterm.js\node_modules\node-pty\build\deps\winpty\src\winpty-agent.vcxproj]
  All 1772 functions were compiled because no usable IPDB/IOBJ from previous compilation was found.
  Finished generating code
  winpty-agent.vcxproj -> C:\Users\dan\xterm.js\node_modules\node-pty\build\Release\\winpty-agent.exe
  AgentLocation.cc
  winpty.cc
  BackgroundDesktop.cc
  Buffer.cc
  DebugClient.cc
  GenRandom.cc
  OwnedHandle.cc
  StringUtil.cc
  WindowsSecurity.cc
  WindowsVersion.cc
  WinptyAssert.cc
  WinptyException.cc
  WinptyVersion.cc
  win_delay_load_hook.cc
     Creating library C:\Users\dan\xterm.js\node_modules\node-pty\build\Release\winpty.lib and object C:\Users\dan\xterm.js\node_modules\node-pty\build\Release\winpty.exp
  Generating code
  Previous IPDB not found, fall back to full compilation.
  All 1056 functions were compiled because no usable IPDB/IOBJ from previous compilation was found.
  Finished generating code
  winpty.vcxproj -> C:\Users\dan\xterm.js\node_modules\node-pty\build\Release\\winpty.dll
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin\MSBuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\lib\build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (node:events:390:28)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
gyp ERR! System Windows_NT 10.0.19043
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd C:\Users\dan\xterm.js\node_modules\node-pty
gyp ERR! node -v v17.0.0
gyp ERR! node-gyp -v v8.2.0

dstein64 · 2021-10-24T04:00:06Z

I was able to get the xterm.js demo running directly from Windows. The problem mentioned in my last comment was from using Node v17.0.0 (nodejs/Release#704), and was no longer present after upgrading to Node v17.0.1. After upgrading, I encountered another error, digital envelope routines::unsupported (webpack/webpack#14532), which was resolved by running set NODE_OPTIONS=--openssl-legacy-provider prior to running yarn.

With the demo running directly from Windows, using the branch from this PR, I was still unable to reproduce the cutoff.

Next, I ran yarn package to create an xterm.js file with the update from this PR. I also created an xterm.js file without the update from this PR (at 1a697bd, the commit prior to the rebased update). I've created a page that compares before versus after, accessible at https://dstein64.github.io/xterm.js-3370/. @Tyriar, when viewing that page on Firefox, are the characters being truncated in both scenarios? For me, I'm only seeing characters truncated without the update from this PR.

nirui · 2021-10-24T05:08:03Z

I was still unable to reproduce the cutoff.

Which could be the expected result, since ideographic is actually more "aggressive" than bottom across all browsers.

Also, based on the information provided by this little demo (that I grabbed from W3CSchool website, thanks!), the cutoff problem in this screenshot looked like it was actually caused by the ideographic value.

If you visit the demo with Chrome, you'll found that buttom and ideographic are displayed in almost the same way. Firefox on the other hand, displays the text by the specs.

So my guess is, the solution of changing textBaseline to ideographic was based on an illusion created by our overlord, Chrome the greatest and wisest, to tech us the importance of reading specs.

dstein64 · 2021-10-24T20:47:39Z

Here are the images from @nirui's demo.

Chrome

Firefox

The difference across browsers for textBaseline=ideographic is visible.

textBaseline=bottom appears to be consistent across browsers. My original attempt at addressing #3353 set textBaseline=bottom unconditionally. However, this resulted in Powerline fonts becoming slightly unaligned on Chrome, which led to the approach in this PR that sets the textBaseline conditionally.

Tyriar

Looks good using your URL:

Just retesting after a git clean and it works in the demo as well. Guess maybe my build was stale somehow. Thanks for being persistent 🙂

dstein64 · 2021-10-25T15:58:26Z

Thanks @Tyriar!

I just noticed in my original comment from June that with fontFamily=DejaVu Sans Mono, the underscore was still truncated, even after the update in this PR. That is, the update appeared to improve the situation but not fully resolve it, since some characters were (seemingly) no longer truncated but the underscore still was. The following image is from the earlier comment. There should be an underscore between "codespaces" and "8f6821".

The issue was not present with fontFamily=monospace.

I just tried again using fontFamily=DejaVu Sans Mono along with the update in this PR, and the underscore was visible. The truncated underscore doesn't appear to currently be an issue. Perhaps other changes in the interim, either to the browser itself or to xterm.js, can explain the difference.

nirui · 2021-10-26T02:46:14Z

@dstein64 It seems that the two screenshot in the last comment was captured in different screen resolutions? If you encounter this problem again in the future, can you do more tests (for example changing the font size)? Maybe that will make the "_" symbol pop up somehow (And if it did, maybe something else is at play, not just textBaseline).

Sorry for been annoying, I would try these myself but I'm stuck at yarn for a completely different problem (it seems I cannot get the dependencies to download). I been trying it the entire Sunday (#FunWeekEnd) without success, now I'm kind sad.

dstein64 · 2021-10-26T05:54:55Z

@nirui, the difference in screen resolution was due to me zooming in my browser when capturing the first image, to make it easier to draw a rectangle when creating a screenshot for a region of the screen. I had unintentionally not done that for the more recent capture.

At the zoomed-in resolution, the underscore currently appears. That is, zooming-in doesn't have an effect on whether the underscore is out-of-bounds.

"If you encounter this problem again in the future, can you do more tests (for example changing the font size)?"

It appears that the out-of-bounds underscore was addressed in #3426. I've confirmed that under the scenario with textBaseline=bottom and fontFamily=DejaVu Sans Mono in Firefox, the underscore is out-of-bounds prior to that PR's commit, and visible starting at that PR's commit. That explains the difference I mentioned in my comment above: "Perhaps other changes in the interim, either to the browser itself or to xterm.js, can explain the difference"

Tyriar · 2021-10-26T15:34:50Z

When you zoom the underscore might get spread across multiple pixels and we only expand to the first pixel with content which could be very faint. That might have been the problem. Zooming the page will lead to worse rendering in general.

Tyriar mentioned this pull request Jun 30, 2021

Terminal font crops bottom of lines in Firefox microsoft/vscode#127594

Closed

Tyriar requested changes Jul 13, 2021

View reviewed changes

Tyriar mentioned this pull request Aug 31, 2021

Text shifted down with some characters truncated on Firefox with xtermjs 4.12.0 #3353

Closed

Set text baseline conditionally by browser.

59f940c

Tyriar approved these changes Oct 25, 2021

View reviewed changes

Tyriar added this to the 4.15.0 milestone Oct 25, 2021

Tyriar merged commit 394d7a2 into xtermjs:master Oct 25, 2021

cdanielw mentioned this pull request Nov 29, 2021

Underscores are not being displayed in the terminal when using Firefox openforis/sepal#130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set text baseline conditionally by browser. #3370

Set text baseline conditionally by browser. #3370

dstein64 commented Jun 16, 2021 •

edited

Loading

chkaos commented Jul 12, 2021

dstein64 commented Jul 13, 2021

Tyriar left a comment

dstein64 commented Jul 13, 2021 •

edited

Loading

Tyriar commented Aug 31, 2021

nirui commented Sep 4, 2021 •

edited

Loading

dstein64 commented Sep 4, 2021 •

edited

Loading

Tyriar commented Oct 22, 2021

dstein64 commented Oct 22, 2021

Tyriar commented Oct 22, 2021

dstein64 commented Oct 23, 2021

dstein64 commented Oct 24, 2021 •

edited

Loading

nirui commented Oct 24, 2021

dstein64 commented Oct 24, 2021 •

edited

Loading

Tyriar left a comment

dstein64 commented Oct 25, 2021 •

edited

Loading

nirui commented Oct 26, 2021

dstein64 commented Oct 26, 2021 •

edited

Loading

Tyriar commented Oct 26, 2021

Set text baseline conditionally by browser. #3370

Set text baseline conditionally by browser. #3370

Conversation

dstein64 commented Jun 16, 2021 • edited Loading

chkaos commented Jul 12, 2021

dstein64 commented Jul 13, 2021

Tyriar left a comment

Choose a reason for hiding this comment

dstein64 commented Jul 13, 2021 • edited Loading

Tyriar commented Aug 31, 2021

nirui commented Sep 4, 2021 • edited Loading

dstein64 commented Sep 4, 2021 • edited Loading

Tyriar commented Oct 22, 2021

dstein64 commented Oct 22, 2021

Tyriar commented Oct 22, 2021

dstein64 commented Oct 23, 2021

dstein64 commented Oct 24, 2021 • edited Loading

nirui commented Oct 24, 2021

dstein64 commented Oct 24, 2021 • edited Loading

Tyriar left a comment

Choose a reason for hiding this comment

dstein64 commented Oct 25, 2021 • edited Loading

nirui commented Oct 26, 2021

dstein64 commented Oct 26, 2021 • edited Loading

Tyriar commented Oct 26, 2021

dstein64 commented Jun 16, 2021 •

edited

Loading

dstein64 commented Jul 13, 2021 •

edited

Loading

nirui commented Sep 4, 2021 •

edited

Loading

dstein64 commented Sep 4, 2021 •

edited

Loading

dstein64 commented Oct 24, 2021 •

edited

Loading

dstein64 commented Oct 24, 2021 •

edited

Loading

dstein64 commented Oct 25, 2021 •

edited

Loading

dstein64 commented Oct 26, 2021 •

edited

Loading