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

Create regions only when needed #1119

Merged
merged 3 commits into from
Jul 9, 2024
Merged

Create regions only when needed #1119

merged 3 commits into from
Jul 9, 2024

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Jul 8, 2024

In the current incarnation of the explorer, the live regions and other regions associated with an expression are all created and appended to the document body when the expression is processed. There are 6 such regions for each expression, so the number of these extra divs can get quite large, even for pages with only a moderate number of expressions. Furthermore, these regions are created during a convert() call (like those used in MathJax.tex2chtml() or other conversion methods), even when the result is never put into the document DOM. Finally, these regions are never removed from the page, even if the typeset expressions are removed. For example, in the situation where an editor has a live preview and re-typesets the input as a user types it, even if the previewer correctly removes the previous math before the new preview it processed, the 6 regions will remain for each of the original expressions, and 6 more will be created for each new one. So these regions can quickly balloon during the preview process.

This PR addresses each of these problems. The main change is to only create the region div elements when they are actually needed. This means that there will actually be only two or three regions total (regardless of the number of expressions) when one is being explored, and none when the explorers are not active. This prevents a buildup of divs, and means there is nothing extra to be removed from the DOM when an expression is removed.

These changes are in Region.ts, which you should look at first. The idea is to have functions that require the region, like Update() and Show(), call AddElement() to create it, but have AddElement() check if it already exists and return immediately if not, and to remove AddElement() from the constructor function. That way, the div will be created only when needed. Then Hide() can remove the div from the DOM rather than setting display: none. Other functions that use this.inner check to make sure that it exists first. The styles are simplified, since there is no display: none state. We still keep the _Show classes, since they are used in the stackRegions() function, even though they don't affect the CSS anymore. I also moved some CSS to the styles that were being applied directly to elements.

Several other issues are addressed here, as well. One important one is with the mouse-based magnification. It turns out that there was an unexpected depth to the HoverRegion that was putting an invisible box over top of the areas below the hover region. That means you can't click on the expression below the div, and it was causing unwanted mouseout events when the hover div shows up. The current code records the coordinates of the original mouseover event, and ignores the mouseout if it has the same coordinates. This allowed highlighting and the hover region to remain in place even though the mouse has moved away from the expression, and interfered with changing the magnified characters to the nearby ones, which were also covered by the invisible depth of the box.

This PR uses a clip-path on the hover region to remove the excess depth, which avoids the unwanted mouseout, allows the expression below the hover region to be clicked, and properly moves to the neighboring characters when the mouse moves over them. It also means there is no need to record the coordinates of the mouseover, so those are removed from MouseExplorer.ts.

The key explorer now positions the hover region over the selected characters, just as the mouse explorer does.

In explorer.ts, the initialization of the explorerRegions was being done in the document's explore() call, but that code is only reached if a page-based typeset occurs, and if there is actually math in the page. If neither of those is true (e.g., the initial typesetting is suppressed, or no math is on the page), and one of the convert() methods is called (like MathJax.tex2svg()), then an error occurs when explorerRegions is accessed by the MathItem during the conversion. The explorerRegions were originally instantiated in the document's explore() call because this created the region divs, and in node applications, that caused errors due to the lack of a DOM (and the direct DOM access used by some of the region and explorer code). This allowed node applications to set the preferences to avoid that. Now that the divs are not being created until needed, we can move the initialization of explorerRegions into the the document's constructor, so that they will be available to convert commands immediately.

Finally, there is a change in semantic-enrich.ts that prevents the speech loop from running if there are no MathItems in the documents math list (this caused an error in the loop, since we always try to process at least one MathItem).

@dpvc dpvc requested a review from zorkow July 8, 2024 14:39
@dpvc dpvc added this to the v4.0 milestone Jul 8, 2024
Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

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

The major issue is that the clip-path leads to a white magnification area overlaying the actual magnified expression, which is correctly underneath.
This does not occur on Firefox, however it happens on Chromium/Blink based browsers. I've tested with Brave, Chrome, and Edge in Linux.

ts/a11y/explorer/Region.ts Outdated Show resolved Hide resolved
@dpvc dpvc requested a review from zorkow July 9, 2024 11:06
Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

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

lgtm.

@dpvc dpvc merged commit 5ed95c0 into develop Jul 9, 2024
@dpvc dpvc deleted the fewer-regions branch July 9, 2024 17:30
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