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

Exclude implied animation of region via tts:origin on div and p (#710). #742

Merged
merged 4 commits into from
Jun 5, 2018

Conversation

skynavga
Copy link
Collaborator

Closes #710.

Note that tts:origin does not directly apply to div and p; rather, it applies to region via an implicit redirection (of styles) on div and p.

@skynavga skynavga added this to the CR2 milestone May 14, 2018
@skynavga skynavga self-assigned this May 14, 2018
@skynavga skynavga requested a review from palemieux May 14, 2018 01:59
Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Requesting a change to the editorial formulation, and also noting that this change does not define #origin-block as per #710 (comment).

spec/ttml2.xml Outdated
@@ -20810,6 +20810,11 @@ attribute.</p>
the <loc
href="#style-attribute-origin"><att>tts:origin</att></loc>
attribute.</p>
<p>Notwithstanding the above, support for the <code>#origin</code> feature does not require support for the
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this formulation - it seems confusing to contradict the definition immediately after it. I would prefer to qualify the definition in the first place, for example by changing:

supports the #origin feature if it recognizes and is capable of transforming the tts:origin attribute.

to:

supports the #origin feature if it recognizes and is capable of transforming the tts:origin attribute as applied to the region element.

Copy link
Collaborator Author

@skynavga skynavga May 14, 2018

Choose a reason for hiding this comment

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

We use the same formulation throughout in the same manner, so your suggestion would create an inconsistency. [I just counted 11 instances of this formulaic language in master.]

palemieux
palemieux previously approved these changes May 14, 2018
@skynavga
Copy link
Collaborator Author

Closing without merge as the changes proposed here have been superseded by #794.

@skynavga skynavga self-assigned this Jun 5, 2018
@skynavga skynavga dismissed stale reviews from nigelmegitt and palemieux June 5, 2018 00:32

Dismissing stale review; please re-review.

Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@skynavga skynavga merged commit f60f914 into master Jun 5, 2018
@skynavga skynavga removed their assignment Jun 5, 2018
@skynavga skynavga deleted the issue-0710-origin-feature branch June 28, 2018 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants