Skip to content

Conversation

@zorkow
Copy link
Member

@zorkow zorkow commented Aug 1, 2025

The PR fixes a number of issues for the explorer, when the semantic structure is not aligned with the syntactic tree. As example consider the following equation with the empheq package:

\begin{empheq}[left=\empheqlbrace, right=\empheqrbrace]{align} 
  E&=mc^2 \\ 
  Y&= \sum_{n=1}^\infty \frac{1}{n^2} 
\end{empheq}

In the current version the left brace is a "child" of the first row. Conversely, the right brace has the first row as "parent". The issue is that semantic children do not necessarily have to live in the DOM tree rooted in their semantic parent. Likewise, the semantic root is not necessarily the first speech element in the DOM. We now compute:

  • rootNode by using the root id given in the data-semantic-structure element
  • firstChild by iterating throw the data-semantic-owns list until we find a node which has data-speech-node
  • parentNode (in moveUp) by looking for the actual parent id in the tree.

In addition the nextSibling and prevSibling methods are fixed to actually ignore elements that are not speech nodes. As an example consider the mc^2 above in MathSpeak, where the elided times is now ignored.

@zorkow zorkow requested a review from dpvc August 1, 2025 00:11
@codecov
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.71%. Comparing base (e68c42f) to head (3bba375).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1325   +/-   ##
========================================
  Coverage    86.71%   86.71%           
========================================
  Files          337      337           
  Lines        84074    84074           
  Branches      4758     4758           
========================================
  Hits         72904    72904           
+ Misses       11170    11147   -23     
- Partials         0       23   +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zorkow
Copy link
Member Author

zorkow commented Aug 1, 2025

As an aside, here is the "native" rendering of the above equation

$$ \begin{empheq}[left=\empheqlbrace, right=\empheqrbrace]{align} E&=mc^2 \\ Y&= \sum_{n=1}^\infty \frac{1}{n^2} \end{empheq} $$

Github clearly still uses MathJax to convert to MathML, as the result is still exactly our structure including data-mjx attributes. But the rendering simply looks atrocious. What a terrible implementation.

@dpvc
Copy link
Member

dpvc commented Aug 1, 2025

What a terrible implementation.

Yes, and because they are now using the browser's native MathML that means they are relying on MathML-Core, so things like equation numbers won't work:

$$E=mc^2\tag{1}$$

and the various table attributes aren't supported, so you get things like:

$$\begin{align} a+b &= c + d\\\ e &= f + g + h \end{align}$$

which has two much space before the equal signs, and (depending on the browser), the left-hand side can be misaligned.

The worst, however, is that since MathML-Core doesn't allow mathvariant attributes (except for <mi mathvariant="normal"> for a single-letter content to prevent it from being italic), things like \mathbb{A} will be $\mathbb{A}$, which in some browsers will be just $A$.

@dpvc
Copy link
Member

dpvc commented Aug 1, 2025

The math experience on GitHub is now very much browser and font dependent, which is a shame. It has been a pretty problematic implementation from the very beginning.

Copy link
Member

@dpvc dpvc left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for fixing these issues. I left a possible alternative loop structure to reduce redundancy, and one question, but have approved as is.

@zorkow zorkow merged commit c8756f1 into develop Aug 4, 2025
1 check passed
@zorkow zorkow deleted the fix/navigation_issues branch August 4, 2025 11:28
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.

2 participants