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

FLUID-6175: Improved self voicing #898

Closed
wants to merge 52 commits into from

Conversation

Projects
None yet
2 participants
@jobara
Copy link
Member

commented May 1, 2018

Adds an Orator component which provides a UI for playing/pausing the self voicing. The orator can be used on its own or as part of the selfVoicing enactor in the prefs framework.

This PR also includes the text highlighting from FLUID-6176

https://issues.fluidproject.org/browse/FLUID-6175
https://issues.fluidproject.org/browse/FLUID-6176

jobara added some commits Aug 2, 2017

FLUID-6176: Partial first pass at adding text highlight to Self Voicing.
Still need to strip out non-rendered text from being read, and add unit
tests.
FLUID-6176: Fixing up highlighting and cleanup
The highlighting should be following the utterances to highlight the
word currently being voiced.
FLUID-6175: Refactored selfVoicing enactor to a component.
This is done in prepartion of adding in a ui widget for controlling the
synthesis.
FLUID-6175: Refactoring highlighting.
* renamed from mark to highlight
* added invokers for adding and removing the highlight
* cleaned up the listeners to use the new invokers
FLUID-6175: Split orator into a domReader grade. Started a widget.
Started the widget to control the orator, play/pause. Currently not
wired up or tested.
FLUID-6175: First pass at orator parent component.
Need to modify so that pausing and playing doesn't restart the reading
but continues from where it left off.

jobara added some commits Apr 30, 2018

FLUID-6175: Modified simplify to work with self voicing.
Moved responsibility of resytling the page for simplification to CSS.
Because this is just a demo of simplification and was already hard coded
to the demo page, it made sense to simplify the interaction.
@jobara

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

@amb26 could you please review this PR. It includes changes from #892; which has now been closed in favour of this one.

jobara added some commits May 4, 2018

FLUID-6176: renamed hasRenderedText
Renamed to hasTextToRead as the check does not account for psuedo
elments and could include offscreen text.
FLUID-6175: Fixing tests to work in IE 11.
While the orator component does not support IE 11. It is useful to be
able to run the tests and not have them fail. Particularly in the case
of the all-tests.
// This is necessary so that the speech synthesis stops when navigating to a new page and so that paused
// speaking doesn't prevent new self voicing after a page reload.
fluid.textToSpeech.cleanupOnUnload = function () {
window.onbeforeunload = function () {

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

Rather than binding to this directly, create a fluid.window.onBeforeUnload global event that this is added to as a namespaced listener - this will give other integrators some control over this policy if they need to. Preferably also bind to this declaratively rather than programmatically.

playing: false
},
members: {
container: "@expand:fluid.orator.controller.container({that}.options.container, {that}.options.markup.defaultContainer, {that}.options.scope)"

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

This is somewhat abuse of the expected workflow of "newViewContainer" which is effectively the standard one in the upcoming framework. The linkage between options.container and members.container should be standard in the base grade, and all actions which are necessary to establish it pushed into the evaluation of options.container, for example like this: https://github.com/fluid-project/fluid-authoring/blob/FLUID-4884/src/js/ComponentGraph.js#L581

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

@amb26 and I discussed this in more detail today. The definition of the "container" should be in the base grade and not overridden. Rather we can have the expander refer to an invoker where any necessary changes can be applied.

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

Align the structure more with what's in "fluid.author.containerRenderingView", see link above. And add an additional invoker for rendering into the DOM.. e.g to allow for prepending or appending into the parentContainer.

@@ -890,7 +890,7 @@ var fluid = fluid || fluid_3_0_0;
/**
* Copied from Underscore.js 1.4.3 - see licence at head of this file
*
* Will execute the passed in function after the specified about of time since it was last executed.
* Will execute the passed in function after the specified ammout of time since it was last executed.

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

Typo amount

dom: "@expand:fluid.initDomBinder({that}, {that}.options.selectors, {that}.container)"
// The container must be supplied by a concrete implementation, however the below example using
// fluid.container can be used in most cases.
// container: "@expand:fluid.container({that}.options.container)"

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

This should be uncommented and be the standard definition, not expected to be overridden by derived grades. See comment above

priority: "first",
args: ["{that}.stubs", "{that}", "{that}.options.methods"]
},
"onDestroy.restore": {

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

This namespace should be somehow complementary to the one added to onCreate. But if we are being destroyed, why do we restore something?

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

The restore on destroy can be removed because we are just adding spies to our own methods.


// Accepts a speechFn (either a function or function name), which will be used to perform the
// underlying queuing of the speech. This allows the SpeechSynthesis to be replaced (e.g. for testing)
fluid.orator.domReader.queueSpeech = function (that, speechFn, text, interrupt, options) {

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

No public docs for this function, including the interesting "interrupt" argument. Also its argument list has become very bulky. Better to structure this entirely as a (that, options) structure with named arguments.
Also, it would be better to structure this activity as an event pipeline with namespaced listeners, which will make it easier to chop and change implementations for mocking. In that way we should be able to get rid of the peculiar "speechFn" argument

This comment has been minimized.

Copy link
@jobara

jobara Jul 5, 2018

Author Member

Talked about the event pipeline in the channel.
https://botbot.me/freenode/fluid-work/2018-07-05/?msg=101795899&page=1


$.each(childNodes, function (childIndex, childNode) {
if (childNode.nodeType === fluid.orator.domReader.nodeType.TEXT_NODE) {
var words = childNode.textContent.split(/(\s+)/); // split on whitespace, and capture whitespace

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

Stick this "split" action into an invoker so that it can be a configurable policy

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

In speaking with @amb26 today it was suggested to make a domParser subcomponent on the domReader. We'd put in all the specific parsing functions into that subcomponent.

fluid.orator.domReader.addParsedData(parsed, word, childNode, blockIndex, charIndex, childIndex);
blockIndex += word.length;
// if the current `word` is not an empty string and the last parsed `word` is not whitespace
} else if (word && fluid.orator.domReader.isWord(fluid.get(parsed, [(parsed.length - 1), "word"]))) {

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

Invoke this as an invoker too


var currentBlockIndex = parseQueue[currentIndex].blockIndex;
var maxBoundary = parseQueue[maxIdx].blockIndex + parseQueue[maxIdx].word.length;

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

Probably excessive vertical whitespace throughout this function

/**
* Combines the parsed text into a String.
*
* @param {Array} parsed - An array of parsed data points

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

Array of what? Make a JSDocs @typedef if it is some widely used structure


/**
* Adds a data point to an array of parsed DOM elements.
* Structure of each data point is as follows:

This comment has been minimized.

Copy link
@amb26
// only execute if there are nodes to read from
if (elm.length) {
var parsedFromElm = fluid.orator.domReader.parse(elm[0]);
that.parseQueue = parsedFromElm;

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

I worry about this wholesale obliteration of a mutable structure. I think we need a bit more formality and modelisation of this structure even if we do not store the parseQueue itself in a model for reasons of efficiency.

fluid.orator.domReader.highlight = function (that, boundary) {
that.removeHighlight();

if (that.parseQueue.length) {

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

This should be a modelListener to something rather than an arbitrarily scheduled invoker

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

should probably modelise the boundary index as well.

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

May be able to setup a modelRelay to a closestIndex model value which has a relay rule that contains the functionality currently in getClosestIndex. Then an updateHighlight method could be used to highlight the appropriate portion using this index.

model: "{tts}.model",
members: {
parseQueue: [],
parseIndex: 0,

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

I think a reasonable compromise would be to modelise i) parseIndex, and ii) parseQueue.length and key most of this component's activities off that - even if the current ChangeApplier doesn't make it a great idea to stash the entire queue in the model.

events: {
onReadFromDOM: null
},
model: "{tts}.model",

This comment has been minimized.

Copy link
@amb26

amb26 Jun 26, 2018

Member

This will then end up being nested one level deeper.

/**
* Returns the index of the closest data point from the parseQueue based on the boundary provided.
*
* @param {Array} parseQueue - An array data points generated from parsing a DOM structure

This comment has been minimized.

Copy link
@amb26

amb26 Jun 27, 2018

Member

An array "of" - also, write out something like Array < ParseQueueElement >

* cannot be located within the parseQueue, `undefined` is returned.
*/
fluid.orator.domReader.getClosestIndex = function (parseQueue, currentIndex, boundary) {
var maxIdx = Math.max(parseQueue.length - 1, 0);

This comment has been minimized.

Copy link
@amb26

amb26 Jun 27, 2018

Member

I don't see a need for the inconsistency wrt. "Idx" and "Index"

}

if (currentBlockIndex > boundary) {
return fluid.orator.domReader.getClosestIndex(parseQueue, prevIdx, boundary);

This comment has been minimized.

Copy link
@amb26

amb26 Jun 27, 2018

Member

I still don't see any reason for this function to be recursive, least of all doubly recursive, and would be much clearer if it was rewritten so that all the initial setup happens just once

This comment has been minimized.

Copy link
@jobara

jobara Jul 4, 2018

Author Member

Explore re-writing this as a while loop with break conditions.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

The comments on this PR have been addressed in the work for FLUID-6177 and have been included in its PR. This PR is being closed in favour of #903

@jobara jobara closed this Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.