Implementation of new nested SVGs and Tick Labels for Interactive Graph to solve Safari rendering issues. #1452

SonicScrewdriver · 2024-07-25T23:06:08Z

Screenshot:

Summary:

This PR introduces a new approach to solving the issues related to re-rendering Axis Ticks Labels in Safari, along with several other related issues related to SVG rendering in Safari.

We are now overriding the overflow rules from Mafs in order to allow our Axis Tick Labels to render outside the bounds of the cartesian grid. This required the addition of two nested SVG elements in order to contain the interactive figures, locked figures, and protractor within the new graph bounds. The elements are bound to the graph edges by adjusting the viewBox attribute on the nested SVGs according to the graph's range.

While researching this issue, we were able to discover that there's a key issue with the MathJax font itself, which has required us to change the Axis Tick Labels back to using the original KaTeX font.

Several stories were updated or added in the Interactive Graph Regression Tests in order to help protect against regressions for key issues related to off-center ranges with locked figures.

This approach also allowed us to reintroduce the text stroke around the axis tick labels in order to improve general legibility.

History:

This nested SVG approach has become necessary in order to solve numerous reported issues related to the rendering of the axis tick labels when using earlier versions of Safari (14-16). Unfortunately, Safari has many long standing bugs related to re-rendering SVGs, particularly when dealing with absolutely positioned elements or more modern features.

Issue: LEMS-2035

Test plan:

Significant manual testing using new Interactive Graph Regression Tests in Storybook
Several rounds of Play testing
Manually testing in Webapp on a ZND

khan-actions-bot · 2024-07-25T23:09:16Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/wet-students-float.md, packages/perseus/src/widgets/interactive-graphs/interactive-graph-regression.stories.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-styles.css, packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-arrows.tsx, packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts, packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx, packages/perseus/src/widgets/interactive-graphs/backgrounds/grid.tsx

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

github-actions · 2024-07-25T23:12:15Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1452

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

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

github-actions · 2024-07-25T23:13:40Z

Size Change: -1.29 kB (-0.15%)

Total Size: 859 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	277 kB	-375 B (-0.14%)
`packages/perseus/dist/es/index.js`	416 kB	-911 B (-0.22%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.36 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

SonicScrewdriver · 2024-08-27T22:44:03Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-tick-labels.tsx

This file is no longer needed, as the labels were able to be reintegrated with the axis-ticks themselves.

codecov · 2024-08-27T23:20:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.04%. Comparing base (bdb382f) to head (2b2e503).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1452      +/-   ##
==========================================
- Coverage   70.52%   70.04%   -0.48%     
==========================================
  Files         548      572      +24     
  Lines      107317   111560    +4243     
  Branches     7827    11819    +3992     
==========================================
+ Hits        75687    78146    +2459     
- Misses      31514    33414    +1900     
+ Partials      116        0     -116

see 1119 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 bdb382f...2b2e503. Read the comment docs.

SonicScrewdriver · 2024-09-04T17:16:51Z

packages/perseus/src/widgets/interactive-graphs/mafs-styles.css

@@ -214,53 +216,9 @@
 .MafsView pattern g {
    stroke: rgba(33, 36, 44, 0.32);
 }
-.axis-tick-labels {


All of these styles are no longer needed as we're using attributes to ensure proper compatibility with older versions of Safari.

SonicScrewdriver · 2024-09-04T17:19:16Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/grid.tsx

-    const isMax = n === max;
-    const shouldRender = isOnStep && !isNegativeOne && !isMin && !isMax;
-    return shouldRender ? `${n}` : "";
-};


This was already unused prior to these changes, and is safe to delete.

SonicScrewdriver · 2024-09-04T17:19:51Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/grid.test.ts

@@ -1,72 +0,0 @@
-import {lineLabelText} from "./grid";


This is testing a function that was not being used, and has been deleted as part of this PR.

SonicScrewdriver · 2024-09-05T22:10:19Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

+                                    <>
+                                        <AxisTicks />
+                                        <AxisArrows />
+                                    </>


Both the Axis Ticks and Axis Arrows need to be allowed to render outside of the bounds of the graph, but ALSO need to render below the protractor/interactive/locked figures AND above the gridlines.

Unfortunately this requires the use of two separate nested SVGs so that we can render these elements in the correct order.

SonicScrewdriver · 2024-09-05T22:33:13Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx

 const tickSize = 10;
+const tickLabelSize = 14;



The font size is important for calculating positioning, so we're handling it within the component.

SonicScrewdriver · 2024-09-05T22:34:59Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-tick-labels.tsx

This file is no longer necessary, as we're pairing the ticks and labels back together within the SVG.

SonicScrewdriver · 2024-09-05T22:37:05Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx

+    if (range[X][MIN] < -1 && range[X][MAX] > 0 && number === -1) {
+        showLabel = false;
+    }
+


I split this into a separate function as I had a feeling that we may come up with more scenarios in the future for when we may want to conditionally hide labels. It also helps keep the code a little more organized.

SonicScrewdriver · 2024-09-05T22:41:25Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

@@ -84,7 +102,7 @@ export const MafsGraph = (props: MafsGraphProps) => {
                        padding: "25px 25px 0 0",
                        boxSizing: "content-box",
                        marginLeft: "20px",
-                        marginBottom: "20px",
+                        marginBottom: "30px",
                        pointerEvents: props.static ? "none" : "auto",


We needed a little more breathing room for the labels now that they're within the SVG

SonicScrewdriver · 2024-09-05T22:50:20Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx

+    // whether the x-axis is above, within, or below the graph
+    const xAdjustment =
+        range[X][MAX] <= 0 ? tickLabelSize * 1.5 : -tickLabelSize * 1.1;
+    const xPositionText = xPosition + xAdjustment;


I'm not a big fan of using 1.1 here, but I found it was the best value to visually line up the labels for our most used gridStep/ranges

SonicScrewdriver · 2024-09-09T19:54:40Z

packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx

+    // Adjust the X position of the x-axis labels based on
+    // whether the label is positive or negative, in order to
+    // account for the width of the negative sign
+    const xAdjustment = x < 0 ? -2 : 0;


Magic number fix?

nicolecomputer · 2024-09-09T19:57:10Z

packages/perseus/src/widgets/interactive-graphs/mafs-styles.css

+
+.MafsView .axis-tick-label {
+    /* There is a bug in the MathJax font for Safari, so we use KaTeX */
+    font-family: "KaTeX_Main";


Specify a fallback font

SonicScrewdriver · 2024-09-09T19:57:25Z

packages/perseus/src/widgets/interactive-graphs/mafs-styles.css

+
+.MafsView .axis-tick-label {
+    /* There is a bug in the MathJax font for Safari, so we use KaTeX */
+    font-family: "KaTeX_Main";


Could we use mathjax for all fonts EXCEPT the one media query with safari to use KaTeX

… be working! Just rewriting comments and tidying up calculateNestedSVGViewBox

…backgrounds" This reverts commit 98ec076.

@SonicScrewdriver

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] ### Minor Changes - [#1452](#1452) [`3980a36fa`](3980a36) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Implementation of SVG-based Axis Tick Labels for Interactive Graph ### Patch Changes - [#1609](#1609) [`981047211`](9810472) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph] Remove the start-coords-ui flags - [#1610](#1610) [`e9b317ca0`](e9b317c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Remove the start coords UI if the graph is static - [#1608](#1608) [`737fe30b4`](737fe30) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph] Remove the interactive-graph-locked-feature-m2b flag ## @khanacademy/[email protected] ### Patch Changes - [#1609](#1609) [`981047211`](9810472) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph] Remove the start-coords-ui flags - [#1607](#1607) [`1b340b197`](1b340b1) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Use Wonder Blocks TextArea in the graph description settings UI - [#1610](#1610) [`e9b317ca0`](e9b317c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Remove the start coords UI if the graph is static - [#1608](#1608) [`737fe30b4`](737fe30) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph] Remove the interactive-graph-locked-feature-m2b flag - Updated dependencies \[[`981047211`](9810472), [`e9b317ca0`](e9b317c), [`737fe30b4`](737fe30), [`3980a36fa`](3980a36)]: - @khanacademy/[email protected]

SonicScrewdriver self-assigned this Jul 25, 2024

SonicScrewdriver changed the title ~~Testing prototype of new SVG axis tick labels + clipping mask~~ [Prototype] Testing new SVG labels + clipping mask in Safari Jul 25, 2024

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from 28e4835 to d021668 Compare July 25, 2024 23:09

khan-actions-bot requested a review from a team July 25, 2024 23:09

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from a82a5a6 to b04aca0 Compare August 15, 2024 20:12

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from f9cec28 to bc29621 Compare August 23, 2024 19:33

SonicScrewdriver commented Aug 27, 2024

View reviewed changes

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from b283d18 to 8a8d3ce Compare August 27, 2024 23:05

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from 8d1eb80 to cde544d Compare August 28, 2024 21:35

SonicScrewdriver commented Sep 4, 2024

View reviewed changes

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from e35ce3e to 5f553a5 Compare September 4, 2024 19:21

SonicScrewdriver changed the title ~~[Prototype] Testing new SVG labels + clipping mask in Safari~~ Implementation of new nested SVGs and labels for Interactive Graph to solve Safari rendering issues. Sep 5, 2024

SonicScrewdriver changed the title ~~Implementation of new nested SVGs and labels for Interactive Graph to solve Safari rendering issues.~~ Implementation of new nested SVGs and Tick Labels for Interactive Graph to solve Safari rendering issues. Sep 5, 2024

SonicScrewdriver commented Sep 5, 2024

View reviewed changes

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from 86066de to 0f1d96e Compare September 5, 2024 22:44

SonicScrewdriver requested a review from mark-fitzgerald September 5, 2024 22:46

SonicScrewdriver commented Sep 5, 2024

View reviewed changes

SonicScrewdriver commented Sep 9, 2024

View reviewed changes

nicolecomputer reviewed Sep 9, 2024

View reviewed changes

SonicScrewdriver commented Sep 9, 2024

View reviewed changes

SonicScrewdriver added 22 commits September 10, 2024 13:39

Updating prototype to solve a couple small bugs before recording demo

f3f1bc3

wip

cdb2d4b

You put the svg in the svg and you line it all up

4c802a2

Solved ALL of the clipping mask issues!

034e2b3

Added more edge cases, and started refactoring. Everything appears to…

ae564a9

… be working! Just rewriting comments and tidying up calculateNestedSVGViewBox

Committing stories

f86a725

Clean up!

5c05560

Refactoring + cleanup

cfdc26b

Tidying up comment errors

b915554

Fixing "magic numbers" as much as possible

a1e668a

Fussing about comments

cab392f

Removing old tests

d06b55c

Cleaning up storybook + css comment

97048f4

Fixing minor negative 1 bug as we still want the tick

e572d62

Adding a test for a simple function

71f449a

Fixing weird editor save bugs + renaming function

a6b7bd7

Fixing PR to fully use X/Y and MIN/MAX

88f0b47

Clean up + Fixing Safari Zoom bug + using KaTeX font

c90b86c

Adjusting spacing as KaTeX seems to have a shorter xHeight

b9e6109

Cleanup + Added tests

eb4f90c

Retested SVG w/ CSS + minor visual improvements

bc5f425

Clean up storybook

8d82012

SonicScrewdriver force-pushed the the-grid-behind-the-mask branch from 0f1d96e to 8d82012 Compare September 10, 2024 20:39

nishasy and others added 3 commits September 10, 2024 16:04

[Locked Labels] Make a mafs sandwich so that labels can have backgrounds

98ec076

Revert "[Locked Labels] Make a mafs sandwich so that labels can have …

e89727e

…backgrounds" This reverts commit 98ec076.

fallback fonts

2b2e503

SonicScrewdriver merged commit 3980a36 into main Sep 11, 2024
9 of 10 checks passed

SonicScrewdriver deleted the the-grid-behind-the-mask branch September 11, 2024 17:44

khan-actions-bot mentioned this pull request Sep 11, 2024

Version Packages #1611

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of new nested SVGs and Tick Labels for Interactive Graph to solve Safari rendering issues. #1452

Implementation of new nested SVGs and Tick Labels for Interactive Graph to solve Safari rendering issues. #1452

SonicScrewdriver commented Jul 25, 2024 •

edited

Loading

khan-actions-bot commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

SonicScrewdriver Aug 27, 2024

codecov bot commented Aug 27, 2024 •

edited

Loading

SonicScrewdriver Sep 4, 2024

SonicScrewdriver Sep 4, 2024

SonicScrewdriver Sep 4, 2024

SonicScrewdriver Sep 5, 2024

SonicScrewdriver Sep 5, 2024

SonicScrewdriver Sep 5, 2024

SonicScrewdriver Sep 5, 2024

SonicScrewdriver Sep 5, 2024

SonicScrewdriver Sep 5, 2024

SonicScrewdriver Sep 9, 2024

nicolecomputer Sep 9, 2024

SonicScrewdriver Sep 9, 2024

Implementation of new nested SVGs and Tick Labels for Interactive Graph to solve Safari rendering issues. #1452

Implementation of new nested SVGs and Tick Labels for Interactive Graph to solve Safari rendering issues. #1452

Conversation

SonicScrewdriver commented Jul 25, 2024 • edited Loading

Screenshot:

Summary:

History:

Test plan:

khan-actions-bot commented Jul 25, 2024 • edited Loading

Gerald

github-actions bot commented Jul 25, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver commented Jul 25, 2024 •

edited

Loading

khan-actions-bot commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading