fix: incorrect cursor position Firefox #12423

betodealmeida · 2021-01-11T22:42:15Z

SUMMARY

When using SQL Lab in Firefox, the position of the cursor will drift to the left progressively along the row. As the cursor moves to the right, it's shifted more and more to the left, to a point where it shows up in the incorrect position.

This seems to be a known bug: https://stackoverflow.com/questions/20931029/ace-editor-monospace-fonts-issues-with-cursor-spacing

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Here's a query in Firefox, with the cursor moved to the end of the line:

Note how it looks like the cursor is to the left of the semi-colon — it's not, and if the user presses delete the semi-colon will be deleted instead!

Here's the same query with this fix:

TEST PLAN

ADDITIONAL INFORMATION

betodealmeida · 2021-01-11T23:09:39Z

cc: @eschutho

etr2460

can you verify that this still works as expected in chrome and safari? We've fixed this same bug a number of times in various browsers.

etr2460 · 2021-01-11T23:57:49Z

superset-frontend/src/SqlLab/main.less

@@ -208,6 +208,10 @@ div.Workspace {
  .ace_content {
    height: 100%;
  }
+
+  .ace_editor * {
+    font: inherit!important;


ugh, !important 🤮 I guess if this is the only way to fix it then it's ok

Although, do we need to apply this to all elements that are descendants of .ace_editor or can we be more specific here? Also, could you add a space between inherit and !important?

It seems that the issue is around the bolded typeface in Fira Code, with the same issue with Inter and monospace. The only one I was able to get to work was "Courier New" so far. Targeting just the bold text seems to work (nested under .ace_editor.ace_editor) if you don't want to change everything:

.ace_keyword { font-family: 'Courier New'; }

we could try out a few different common system fonts if you want to try to find a sans serif.

"Menlo" works and is sans serif, but for Mac only if you want to add that and fall back to Courier.

codecov-io · 2021-01-12T00:35:58Z

Codecov Report

Merging #12423 (1bd9d37) into master (e9d66e9) will decrease coverage by 2.97%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12423      +/-   ##
==========================================
- Coverage   66.12%   63.15%   -2.98%     
==========================================
  Files        1015      486     -529     
  Lines       49554    29956   -19598     
  Branches     5079        0    -5079     
==========================================
- Hits        32767    18918   -13849     
+ Misses      16647    11038    -5609     
+ Partials      140        0     -140

Flag	Coverage Δ
cypress	`?`
javascript	`?`
python	`63.15% <ø> (-0.94%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/examples/world_bank.py	`30.98% <0.00%> (-69.02%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/db_engine_specs/hive.py	`54.61% <0.00%> (-29.24%)`	⬇️
superset/examples/helpers.py	`85.36% <0.00%> (-12.20%)`	⬇️
superset/db_engine_specs/sqlite.py	`65.62% <0.00%> (-9.38%)`	⬇️
superset/databases/commands/create.py	`83.67% <0.00%> (-8.17%)`	⬇️
superset/databases/commands/update.py	`85.71% <0.00%> (-8.17%)`	⬇️
superset/connectors/sqla/models.py	`84.31% <0.00%> (-6.28%)`	⬇️
superset/views/datasource.py	`89.39% <0.00%> (-5.61%)`	⬇️
... and 556 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9d66e9...1bd9d37. Read the comment docs.

betodealmeida · 2021-01-13T02:25:08Z

I switched to @eschutho's approach, which seems more conservative.

@etr2460, It works fine in Firefox:

And Chrome:

But Safari is broken in master:

etr2460

stamping, could you also file an issue for the Safari UX bug?

Finally, does this need to be changed in other places where we use the code editor as well? Dataset edit modal, adhoc metric popover, etc.?

etr2460 · 2021-01-13T17:14:36Z

superset-frontend/src/SqlLab/main.less

@@ -378,7 +378,9 @@ div.tablePopover {
  //double class is better than !important
  border: 1px solid @gray-light;
  font-feature-settings: @font-feature-settings;
-  font-family: @font-family-monospace;
+  // Fire Code causes problem with Ace under Firefox


sp nit, i think it's Fira Code

eschutho · 2021-01-13T17:49:34Z

superset-frontend/src/SqlLab/main.less

-  font-family: @font-family-monospace;
+  // Fire Code causes problem with Ace under Firefox
+  font-family: 'Menlo', 'Courier New', 'Ubuntu Mono', 'Consolas',
+    'source-code-pro', monospace;


Two small things..We could target just the bold typeface by using the selector .ace_editor.ace_editor .ace_keyword if we wanted to just change the bold typeface. Also talking with @Steejay can we try 'Lucida Console' instead of 'Courier New'? Here's the comparison for reference:

rusackas · 2021-01-13T18:22:58Z

Ugh... not again! ACE Editor is so painful with this issue, it seems to keep popping up like whack-a-mole. My general feelings on the issue are:

I'd hate to see Fira Code die off due to this bug (but I understand)
I fear if we use other platform-specific fonts (Menlo, etc) we'll see this continue to manifest itself in other ways.
I hope we can target altering just the bold words as Elizabeth suggests.

The way it calculates the position is just super error prone... I'm not sure if something like CodeMirror has all the features we need, but my temptation to dive into that rabbit hole is growing.

etr2460 · 2021-01-14T03:42:01Z

@ktmud mentioned using the code editor that vscode uses a while back (https://github.com/microsoft/monaco-editor). If after 1.0.0 anyone wanted to do some tech debt cleanup/refactor, moving to Monaco editor might be reasonable

Edit: and here's a react wrapper: https://github.com/suren-atoyan/monaco-react

eschutho · 2021-01-15T02:52:55Z

@ktmud mentioned using the code editor that vscode uses a while back (https://github.com/microsoft/monaco-editor). If after 1.0.0 anyone wanted to do some tech debt cleanup/refactor, moving to Monaco editor might be reasonable

Edit: and here's a react wrapper: https://github.com/suren-atoyan/monaco-react

Nice, we'll look into those options. Thanks @etr2460 and @rusackas for the feedback.

betodealmeida · 2021-01-15T19:05:51Z

I hope we can target altering just the bold words as Elizabeth suggests.

@rusackas we talked with @Steejay and he recommended using a single font for the whole query.

Steejay · 2021-01-15T19:17:23Z

I think keeping the font consistent would be ideal especially if its within the same line.

betodealmeida · 2021-01-15T22:00:16Z

stamping, could you also file an issue for the Safari UX bug?

#12565

* fix: incorrect cursor position Firefox * Use different font * Fix lint * Use Lucida Console

pull-request-size bot added the size/XS label Jan 11, 2021

betodealmeida requested a review from rusackas January 11, 2021 22:42

etr2460 reviewed Jan 11, 2021

View reviewed changes

betodealmeida force-pushed the fix_cursor_firefox branch from 51fe19a to 752dcb7 Compare January 13, 2021 00:12

betodealmeida added 2 commits January 12, 2021 17:25

fix: incorrect cursor position Firefox

5b360bf

Use different font

fcf009e

betodealmeida force-pushed the fix_cursor_firefox branch from 752dcb7 to fcf009e Compare January 13, 2021 01:25

Fix lint

c108f31

etr2460 approved these changes Jan 13, 2021

View reviewed changes

eschutho reviewed Jan 13, 2021

View reviewed changes

Use Lucida Console

1bd9d37

betodealmeida merged commit 39c6ef2 into apache:master Jan 15, 2021

villebro added the v1.0.1 label Jan 25, 2021

villebro pushed a commit that referenced this pull request Jan 25, 2021

fix: incorrect cursor position Firefox (#12423)

bc1a2a2

* fix: incorrect cursor position Firefox * Use different font * Fix lint * Use Lucida Console

junlincc added the preset-io label Mar 18, 2021

rumbin mentioned this pull request Mar 27, 2021

Move to monaco-editor #13838

Closed

mapshen mentioned this pull request Jul 29, 2021

Cursor falling behind in SQL Lab #15749

Closed

3 tasks

MichaelHintz mentioned this pull request Sep 7, 2022

Cursor falls behind in SQL Lab #9253

Closed

3 tasks

mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: incorrect cursor position Firefox #12423

fix: incorrect cursor position Firefox #12423

betodealmeida commented Jan 11, 2021 •

edited

Loading

betodealmeida commented Jan 11, 2021

etr2460 left a comment

etr2460 Jan 11, 2021

eschutho Jan 12, 2021

eschutho Jan 12, 2021

codecov-io commented Jan 12, 2021 •

edited

Loading

betodealmeida commented Jan 13, 2021

etr2460 left a comment

etr2460 Jan 13, 2021

betodealmeida Jan 15, 2021

eschutho Jan 13, 2021 •

edited

Loading

rusackas commented Jan 13, 2021

etr2460 commented Jan 14, 2021 •

edited

Loading

eschutho commented Jan 15, 2021

betodealmeida commented Jan 15, 2021

Steejay commented Jan 15, 2021

betodealmeida commented Jan 15, 2021

fix: incorrect cursor position Firefox #12423

fix: incorrect cursor position Firefox #12423

Conversation

betodealmeida commented Jan 11, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

betodealmeida commented Jan 11, 2021

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Jan 11, 2021

Choose a reason for hiding this comment

eschutho Jan 12, 2021

Choose a reason for hiding this comment

eschutho Jan 12, 2021

Choose a reason for hiding this comment

codecov-io commented Jan 12, 2021 • edited Loading

Codecov Report

betodealmeida commented Jan 13, 2021

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Jan 13, 2021

Choose a reason for hiding this comment

betodealmeida Jan 15, 2021

Choose a reason for hiding this comment

eschutho Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

rusackas commented Jan 13, 2021

etr2460 commented Jan 14, 2021 • edited Loading

eschutho commented Jan 15, 2021

betodealmeida commented Jan 15, 2021

Steejay commented Jan 15, 2021

betodealmeida commented Jan 15, 2021

betodealmeida commented Jan 11, 2021 •

edited

Loading

codecov-io commented Jan 12, 2021 •

edited

Loading

eschutho Jan 13, 2021 •

edited

Loading

etr2460 commented Jan 14, 2021 •

edited

Loading