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

Address draft CR2 comments #417

Merged
merged 4 commits into from Jul 12, 2018
Merged

Address draft CR2 comments #417

merged 4 commits into from Jul 12, 2018

Conversation

palemieux
Copy link
Contributor

@palemieux palemieux commented Jul 3, 2018

Closes #421

@palemieux palemieux added this to the imsc1.1 CR2 milestone Jul 3, 2018
@palemieux palemieux self-assigned this Jul 3, 2018
@palemieux palemieux requested a review from cconcolato July 3, 2018 03:38
@@ -2863,6 +2853,9 @@ <h3>Style Resolution</h3>

<li><code>ebutts:multiRowAlign</code></li>
</ul>

<p class="note">The style properties above can be specified as an attribute of the <code>initial</code> element specified at

Choose a reason for hiding this comment

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

Maybe replace "as an attribute" with "as attributes" ?

@@ -2863,6 +2853,9 @@ <h3>Style Resolution</h3>

<li><code>ebutts:multiRowAlign</code></li>
</ul>

<p class="note">The style properties above can be specified as attributes of the <code>initial</code> element specified at
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this moved to an informative note rather than staying as a MAY at line 2844-5? Seems better to me to say "and MAY be specified as attributes of the initial element...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because foreign attributes are already permitted by TTML2, so no permission needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, in that case we need to be clearer still, that not only are those attributes permitted, but that the semantics of initial apply to them when used in that context. There needs to be a normative statement in there somewhere. Otherwise there's no processing requirement that actually does something with the extension style attributes, they could conformantly just be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current prose states The following style properties are subject to the Style Resolution procedures specified at Section 10.4 of [[!ttml2-20180628]]. The latter includes tt:initial processing. Are you suggesting the prose should say:

The following style properties shall be subject to the Style Resolution procedures specified at Section 10.4 of [[!ttml2-20180628]]

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the long pause, @palemieux , yes, that would be an excellent change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nigelmegitt See update.

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.

Thank you!

@css-meeting-bot
Copy link
Member

The Working Group just discussed Address draft CR2 comments imsc#417.

The full IRC log of that discussion <nigel> Topic: Address draft CR2 comments imsc#417
<nigel> github: https://github.com//pull/417
<nigel> Pierre: I addressed your comment Nigel
<nigel> Nigel: That looks great, thank you!
<nigel> .. [approves pull request]

@palemieux palemieux merged commit 4d10178 into master Jul 12, 2018
@palemieux palemieux deleted the cr2-review-comments branch July 17, 2018 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants