Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Interactive Graph Editor] Stop page scrolling on number text field focused scroll #1481

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

nishasy
Copy link
Contributor

@nishasy nishasy commented Aug 2, 2024

Summary:

Consider an <input> element with type="number". The default behavior in React (except Firefox)
if it's focused and wheel over it is that it does BOTH (a) change the number and (b) scroll
the entire page.

There is a specific request from content authors to make it so that only one of those
bevahiors happens.

In this PR:

  • Make a new component called ScrolllessNumberTextField that uses stopPropagation
    to stop the page scroll. The number changes on scroll (except in Firefox).
  • Stories to test this
  • Updates to all the type="number" text fields in the perseus editor to use
    the new ScrolllessNumberTextField component.

Notes:

  • Even with this custom implementation, Firefox inputs scroll the page and not the number.
    This is in line with native (non-React) HTML input behavior. As this still only executes
    one of the actions, this is still fine.
  • It doesn't seem to be possible to test the input change on wheel event.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2121

Test plan:

yarn jest packages/perseus-editor/src/components/__tests__/scrollless-number-text-field.test.tsx

Storybook

Videos:

Before

Screen.Recording.2024-08-02.at.3.01.02.PM.mov

After

Screen.Recording.2024-08-02.at.3.01.46.PM.mov

@nishasy nishasy self-assigned this Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

Size Change: +8 B (0%)

Total Size: 853 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 273 kB +547 B (+0.2%)
packages/perseus/dist/es/index.js 412 kB -539 B (-0.13%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.3 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.8 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-linter/dist/es/index.js 21.6 kB
packages/perseus/dist/es/strings.js 3.23 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

@nishasy nishasy requested review from jeremywiebe, benchristel and a team August 2, 2024 22:22
@nishasy nishasy marked this pull request as ready for review August 2, 2024 22:26
@khan-actions-bot
Copy link
Contributor

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/purple-dots-beam.md, packages/perseus-editor/src/components/angle-input.tsx, packages/perseus-editor/src/components/coordinate-pair-input.tsx, packages/perseus-editor/src/components/scrollless-number-text-field.tsx, packages/perseus-editor/src/components/start-coords-circle.tsx, packages/perseus-editor/src/components/__stories__/scrollless-number-text-field.stories.tsx, packages/perseus-editor/src/components/__tests__/scrollless-number-text-field.test.tsx, packages/perseus-editor/src/components/graph-locked-figures/locked-polygon-settings.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.35%. Comparing base (b4e0b60) to head (a1d54f4).
Report is 3 commits behind head on main.

Files Patch % Lines
...or/src/components/scrollless-number-text-field.tsx 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1481      +/-   ##
==========================================
+ Coverage   69.94%   70.35%   +0.41%     
==========================================
  Files         509      512       +3     
  Lines      105489   105537      +48     
  Branches     7591    11437    +3846     
==========================================
+ Hits        73780    74255     +475     
+ Misses      31520    31282     -238     
+ Partials      189        0     -189     

Impacted file tree graph

Files Coverage Δ
...ages/perseus-editor/src/components/angle-input.tsx 100.00% <100.00%> (ø)
...us-editor/src/components/coordinate-pair-input.tsx 100.00% <100.00%> (ø)
...s/graph-locked-figures/locked-polygon-settings.tsx 100.00% <100.00%> (ø)
...seus-editor/src/components/start-coords-circle.tsx 93.42% <100.00%> (-0.09%) ⬇️
...or/src/components/scrollless-number-text-field.tsx 97.77% <97.77%> (ø)

... and 168 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@nishasy nishasy requested a review from mark-fitzgerald August 2, 2024 22:35
Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I feel like this is really a bug in React, but since we probably won't get it fixed there, we might consider it a bug in Wonder Blocks. I don't think we'd ever want a scroll gesture to both change the input and scroll the page.

* without scrolling the page.
*
* NOTE 1: Native HTML number inputs do not update the number value on scroll,
* they only scroll the page. For some reason, inputs in React do NOT work
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this happens is explained here: https://stackoverflow.com/a/68266494

Might be worth linking to that answer so future maintainers know what's up.

@nishasy
Copy link
Contributor Author

nishasy commented Aug 5, 2024

@benchristel I did explore updating this in Wonder Blocks, but I don't think it's feasible to change this everywhere across Khan Academy. (a) Because this behavior is the exact opposite of expected native behavior, and (b) because it's inconsistent across browsers. I think this is fine for the content authors as it was specifically requested, but it seems like it would warrant a much larger discussion if we wanted to address this elsewhere. 😞

Copy link
Contributor

github-actions bot commented Aug 5, 2024

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (a1d54f4) and published it to npm. You
can install it using the tag PR1481.

Example:

yarn add @khanacademy/perseus@PR1481

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1481

@nishasy nishasy merged commit a35be71 into main Aug 5, 2024
11 checks passed
@nishasy nishasy deleted the input-text-field-scroll branch August 5, 2024 19:37
Myranae pushed a commit that referenced this pull request Aug 9, 2024
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.


# Releases
## @khanacademy/[email protected]

### Major Changes

-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead

### Minor Changes

-   [#1455](#1455) [`8b0268215`](8b02682) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon graphs to have weighted lines only on keyboard focus


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1428](#1428) [`eb9f3f9c0`](eb9f3f9) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Drop katex dependency - no longer used


-   [#1472](#1472) [`4c2ace57a`](4c2ace5) Thanks [@benchristel](https://github.com/benchristel)! - Add keyboard controls to Mafs angle graphs


-   [#1487](#1487) [`53031f8df`](53031f8) Thanks [@Myranae](https://github.com/Myranae)! - Make graphs non-interactive via keyboard when their question has been answered correctly


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1474](#1474) [`a8aaac339`](a8aaac3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor type fixes in the `ImageLoader` and `SvgImage` components


-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1475](#1475) [`6a218bcc1`](6a218bc) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - add non-visual text as a description for expression widget

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Minor Changes

-   [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor


-   [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs


-   [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs


-   [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)

### Patch Changes

-   [#1491](#1491) [`395b44f29`](395b44f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Correct flag logic for polygon graph start coords UI


-   [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements


-   [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead


-   [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer


-   [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons


-   [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords


-   [#1471](#1471) [`b4e0b60dc`](b4e0b60) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the dropdowns overlapping labels in locked figures settings


-   [#1481](#1481) [`a35be719f`](a35be71) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Stop page scrolling on number text field focused scroll


-   [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip


-   [#1477](#1477) [`653b520c6`](653b520) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate ItemExtrasEditor to WB checkboxes

-   Updated dependencies \[[`a8aaac339`](a8aaac3), [`de13fd337`](de13fd3), [`8b0268215`](8b02682), [`2ebbc1978`](2ebbc19), [`6c6ff52f4`](6c6ff52), [`18f38fca4`](18f38fc), [`ef96cdd54`](ef96cdd), [`342a72211`](342a722), [`de2883b3f`](de2883b), [`7fd586ef4`](7fd586e), [`f920a4cc7`](f920a4c), [`eb9f3f9c0`](eb9f3f9), [`4c2ace57a`](4c2ace5), [`53031f8df`](53031f8), [`808956098`](8089560), [`5e66539e6`](5e66539), [`0b625f560`](0b625f5), [`6a218bcc1`](6a218bc), [`0bec013e8`](0bec013)]:
    -   @khanacademy/[email protected]
    -   @khanacademy/[email protected]

## @khanacademy/[email protected]

### Patch Changes

-   [#1495](#1495) [`6c6ff52f4`](6c6ff52) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove old buttons that we weren't using anymore


-   [#1475](#1475) [`342a72211`](342a722) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update wonder blocks popover versions


-   [#1496](#1496) [`5e66539e6`](5e66539) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove unused buttons from MathInput; add Lato

## @khanacademy/[email protected]

### Patch Changes

-   Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]:
    -   @khanacademy/[email protected]

Author: khan-actions-bot

Reviewers: Myranae

Required Reviewers:

Approved By: Myranae

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️  Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants