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-6176: Highlighting text that is being self voiced #892

Closed
wants to merge 17 commits into from

Conversation

Projects
None yet
3 participants
@jobara
Copy link
Member

commented Apr 3, 2018

Will highlight the visible text that is being self voiced. This should follow along word-by-word as it is being synthesized. The highlighting should work in the various contrast modes. To test the self voicing, please use the prefs framework demo, and make sure to run grunt buildStylus to ensure the necessary styles have been generated.

NOTE: that text that wouldn't be included in the accessibility tree (e.g. display: none, visibility: hidden, aria-hidden: true) will not be added to the self voicing and highlighting.

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

@jobara

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

@amb26 would you like to review this PR to add the highlighting of text as it is being self voiced. If I remember correctly we were talking about implementation ideas for this several months ago when I first started to look into it.

fluid.prefs.enactor.selfVoicing.handleUtteranceEndEvent = function (that) {
that.parseQueue.shift();
that.parseIndex = 0;
fluid.prefs.enactor.selfVoicing.unWrap(that.locate("mark"));

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

Very awkward mix of "model-based" action on the queues and dispatch to a global function. The last action should be implemented by a suitably-named invoker, and this handler should be split into two separate listeners

if (altText) {
that.tts.queueSpeech(altText);
/**
* Determines if there is rendered text to in an element.

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

Typo here

fluid.each(words, function (word) {
if (fluid.prefs.enactor.selfVoicing.isWord(word)) {
parsed.push({
blockIndex: blockIndex,

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

Factor these two pushes into a common function

endOffset: {Number}, // the start offset of the current `word` relative to the closest enclosing DOM element
node: {node}, // the current child node being parsed
childIndex: {Number}, // the index of the child node being parsed relative to its parent
parentNode: {node}, // the parent DOM node

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

Inconsistent use of "node" in some places and "element" in others. It looks like in our core docs we use "DomElement" https://docs.fluidproject.org/infusion/development/ViewAPI.html#fluidgetidelement

This comment has been minimized.

Copy link
@jobara

jobara Apr 12, 2018

Author Member

@amb26 part of the variances may be because in some cases it may refer to a child node of a DOM element which may just be a textnode.

*
* @return {Array} - An array of data points, objects of the with the following structure.
* {
blockIndex: {Number}, // the index into the entire block of text being parsed

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

We tend to be more specific when we can with "Integer" when we know that the value can't be an arbitrary number

fluid.prefs.enactor.selfVoicing.hasRenderedText = function (elm) {
elm = fluid.unwrap(elm);

return elm && !!elm.offsetHeight && fluid.prefs.enactor.selfVoicing.isWord(elm.innerText) && !$(elm).closest("[aria-hidden=\"true\"]").length;

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

This is a very long line - I think in other contexts we have been talking about some kind of line limit like 120 or 132? I can't remember the exact figure, perhaps @simonbates can remember. The current git review interface shows me 132 characters.

This comment has been minimized.

Copy link
@simonbates

simonbates Apr 12, 2018

Member

I think for GPII, we talked about using a 120 char limit.

/**
* 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 Apr 12, 2018

Member

This should read something like {ParseQueueElement[]} and the structure defined in a block comment somewhere

return currentIndex;
}

return fluid.prefs.enactor.selfVoicing.getClosestIndex(parseQueue, nextIndex, boundary);

This comment has been minimized.

Copy link
@amb26

amb26 Apr 12, 2018

Member

JavaScript still does not have "tail recursion elimination" so I suggest a clearer implementation would just be iterative - especially since there is a lot of setup that you do at the beginning of every function call which is simply duplicated each time

This comment has been minimized.

Copy link
@jobara

jobara Apr 12, 2018

Author Member

@amb26 i'll explore that. However, i do have to down into sibling and descendant DOM Elements to pull out all of the data.

This comment has been minimized.

Copy link
@jobara

jobara Apr 13, 2018

Author Member

@amb26 I posted a question in the fluid-work IRC channel about this. ( https://botbot.me/freenode/fluid-work/2018-04-12/?msg=98918443&page=1 ).

In short, I haven't found a simple way to make this purely iterative, because we don't know ahead of time the nesting of DOM elements. I tried using querySelectorAll or jQuery's find method to get all of the DOM elements, but this would still require dealing with nested elements to preserve the order of the text.

This comment has been minimized.

Copy link
@amb26

amb26 Apr 13, 2018

Member

The DOM may be nested, but as far as I can see this method can be purely linear since it works only on the parseQueue structure which is an array, and the only varying coordinate is the 2nd "index" argument which is just an integer. As far as I can see this body could be replaced by one or two "while" loops, with a great increase in clarity

This comment has been minimized.

Copy link
@jobara

jobara Apr 16, 2018

Author Member

@amb26 the method navigates through a nested structure to output a flattened one. I'm still struggling to think of how best to achieve this with the while loops given that the childnodes of an element may be text nodes or elements themselves. And these sub elements may have one or more sub elements and so on. Perhaps if you could sketch out some pseudo code for what you are envisioning it would help?

@jobara

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

@amb26 thank you for the review comments. However, between filing this PR and now I've made some major refactoring to move this into a component as part of work for FLUID-6175. Would you mind if just make the recommend changes above in that branch? If that's okay, I'll close off this PR and will file one for FLUID-6175 that includes the work for FLUID-6176, when things are ready.

@jobara

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

Closing in favour of #898

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.