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

Add enable/disable options to document handlers for a11y and menu #499

Merged
merged 3 commits into from
Jun 25, 2020

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented May 19, 2020

This PR adds enable/disable document options for the a11y and menu document handlers so that these options can be controlled programmatically more easily. These are the enableExplorer, enableMenu, etc. options in the various MathDocument mixins.

This makes it unnecessary to override the explorer(), enrich() (and similar methods) in the menu code, which simplifies the MenuHandler considerably. The testing that was done in these routines has been moved to the checkLoading() render action. This makes the menu handler cleaner all around; I was never happy about the overriding that was being done there.

I also normalized the handling of the state and process bits in some places, so that the states are set even if the action is skipped (so that attempting the actions that are skipped won't be repeated in the future). This causes some changes in indentation, and makes some of the diffs seem more substantial than they are.

Finally, I added force parameters to force the processing even when the enable options are off (so, for example, the complexity computations can force enrichment even if enrichment is disabled).

@dpvc dpvc added this to the v3.1.0 milestone May 19, 2020
@dpvc dpvc requested a review from zorkow May 19, 2020 21:22
@dpvc
Copy link
Member Author

dpvc commented May 19, 2020

Remember that you can turn off the white-space differences to ignore the indentation changes, which makes it easier to see the "real" changes.

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.

I like that the code becomes a lot more concise.
However, there is one part I do not fully understand.
While enableMenu allows me to disable the menu as an option settings, enableEnrich and enableExplorer can not be set in options (even with enableMenu true and while I can see the on the document.options).

ts/a11y/assistive-mml.ts Outdated Show resolved Hide resolved
@@ -156,6 +160,7 @@ B extends MathDocumentConstructor<AbstractMathDocument<N, T, D>>>(
*/
public static OPTIONS: OptionList = {
...BaseDocument.OPTIONS,
enableAssistiveMml: true,
Copy link
Member

Choose a reason for hiding this comment

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

We wanted to set this to false in 3.1 and instead have accessibility switched on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to do that in a separate PR. I always want to do some testing to see what the performance impact is before making that change.

@dpvc
Copy link
Member Author

dpvc commented Jun 16, 2020

While enableMenu allows me to disable the menu as an option settings, enableEnrich and enableExplorer can not be set in options

These options are only available when the corresponding component file is loaded. The menu component is included in the main combined components (like tex-chtml) automatically, but the other a11y components are loaded on demand by the menu. So their options are not available unless you explicitly load them yourself. They do not force the loading of the components.

Perhaps I misunderstood what you wanted in terms of control of these components. This was intended to give programmatic control over the components once they are loaded (like from the menu), and to allow you to disable a component if it is included in a combined component, for example. It was not intended to control the loading of the component, which is done via the loader.load array, nor was it meant as a means of controlling the menu settings, which is done through the menuOptions.settings object (e.g, menuOptions.settigns.explorer controls whether the explorer is loaded and enabled).

If you have a different behavior in mind, we can revisit this.

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 9fdd1e2 into develop Jun 25, 2020
@dpvc dpvc deleted the a11y-options branch June 25, 2020 19:40
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