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

Introducing media overlay highlighter events #32

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Introducing media overlay highlighter events #32

wants to merge 1 commit into from

Conversation

yuriyp
Copy link
Contributor

@yuriyp yuriyp commented Jul 7, 2014

Adding events for media overlay highligher. Those are necessary in case one wants to follow the (de-)highlighted elements with some applications' graphical elements (fade-out effects, adjustable audio button etc.)
Note that the events currently are triggered only by 'highlightElement' logic (w/o CFI). That's intentional, as with CFI we basically won't have an element in DOM when highlight is removed.

@danielweck
Copy link
Member

I do not fully understand the application-level use cases (and ; from a content-level perspective ; authors can trigger actions based on CSS class changes), but you obviously have given this some thought already, so I feel confident that this feature will be useful to others.

So, I agree to merge on principle, assuming there is no performance penalty due to event notification during fine-grain / word-level highlighting.

One thing: can we please use the term "Active" instead of "Highlight" (the latter is agnostic to the consequence of the "CSS class injection", the latter implies a specific visual effect, e.g. colour highlight). The term "highlight" was used internally for historical reasons, but in public APIs I would rather use the more appropriate spec-relevant term.

Thanks!

@danielweck danielweck added this to the v1+ milestone Jul 31, 2014
@rkwright rkwright modified the milestones: m1.2, m1.1 Oct 24, 2014
@danielweck
Copy link
Member

Having looked at this PR again, I feel that it would be a shame not to support event notifications of CFI character-range highlighting, and also that the API client side only receives a reference to a DOM element. So, I suggest that we pass the corresponding "active" SMIL tree node (seq or par, depending on synchronisation granularity) in addition to the affected DOM element, and of course in the case of CFI highlighting such DOM element would be nil (which would indicate the presence of a CFI highlight, so no need to clutter the ReadiumSDK namespace with distinct element/CFI event names).
I will create a separate feature branch under "readium" (this one is under "irls"), and I will produce a new Pull Request.
Dan

@rkwright rkwright removed this from the m1.2 milestone May 16, 2016
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.

4 participants