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

Clarify use of font* properties in resolving default line height (#785). #799

Merged
merged 6 commits into from Jun 7, 2018

Conversation

Projects
None yet
4 participants
@skynavga
Copy link
Collaborator

commented May 25, 2018

Closes #785.

@skynavga skynavga added this to the CR2 milestone May 25, 2018

@skynavga skynavga self-assigned this May 25, 2018

@nigelmegitt
Copy link
Contributor

left a comment

Why doesn't tts:fontVariant also get used in the calculation of tts:lineHeight="normal"? The example suggests that it potentially could do.

This pull request leaves tts:fontShear applying to p but the conversation in #784 suggests it ought only to apply to span (and be inheritable). @skynavga do you intend a different pull request to address that?

In reviewing this, I see that tts:fontKerning only meaningfully applies to span but is still shown as applying to p. It does not obviously appear to affect the calculation of tts:lineHeight, unless kerning applied to tate chu yoko in vertical mode should affect the line height calculation. Would appreciate your thoughts on that @skynavga .

@skynavga

This comment has been minimized.

Copy link
Collaborator Author

commented May 25, 2018

@nigelmegitt addressed comments

@palemieux
Copy link
Contributor

left a comment

TTML2 should follow the same pattern as TTML1, and the following style properties should apply to span only and not p. Unless TTML1 was broken, and we need to understand why.

@skynavga

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2018

@nigelmegitt request approval for early merger at upcoming meeting

@skynavga skynavga added the agenda label May 29, 2018

@cconcolato

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

@@ -9963,6 +9963,9 @@ determines whether and how a shear transformation is applied to <loc href="#term
</tr>
</tbody>
</table>
<p>For the purpose of determining applicability of this style property,
each character child of a <el>p</el> element is considered to be enclosed in an anonymous
span.</p>

This comment has been minimized.

Copy link
@cconcolato

cconcolato May 29, 2018

Contributor

#803 removes such sentence. We should be careful not to reintroduce it.

This comment has been minimized.

Copy link
@skynavga

skynavga May 29, 2018

Author Collaborator

That is a side effect of having multiple PRs open that affect the same text. It will trigger a manual merge process, where this will be dealt with.

<p>For the purpose of determining applicability of this style property,
each character child of a <el>p</el> element is considered to be enclosed in an anonymous
span.</p>
<p>In addition to applying to <loc href="#content-vocabulary-span"><el>span</el></loc>, the computed value of this property

This comment has been minimized.

Copy link
@cconcolato

cconcolato May 29, 2018

Contributor

The sentence is hard for me to read. I would rephrase as follows:

In addition to applying to span, the computed value of this property on a p element is used to determine the computed value of the style property associated with the tts:lineHeight attribute when the value of this attribute is normal as indicated in the definition of the tts:lineHeight attribute.

This comment has been minimized.

Copy link
@skynavga

skynavga May 29, 2018

Author Collaborator

I can't use "this attribute" to refer to tts:lineHeight, since we consistently use "this attribute" or "this property" to refer to the attribute/property being defined, which, in this case, is tts:fontFamily. I can, however, do two things: (1) change "resolve the value of the normal value" to simply "resolve the normal value", and (2) add a cross reference to the algorithm that resolves the normal value (under tts:lineHeight).

This comment has been minimized.

Copy link
@skynavga

skynavga May 29, 2018

Author Collaborator

See update at e7db8c7.

@skynavga

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2018

@palemieux I don't understand your comment #799 (review) since this PR removes the application to p; see, e.g., https://github.com/w3c/ttml2/pull/799/files#diff-5ac39ab39265717d36dfd789d254ce1aL9660.

@palemieux

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

@skynavga Thanks for the pointer. I had missed it.

@cconcolato

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Actually, I'm wondering how fontShear can affect how the lineHeight is computed, given that shearing only changes the size of glyphs in the inline progression direction. So what does the following mean?

adjusted as required to satisfy the computed value of the tts:fontShear style property of P.

@skynavga

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2018

@cconcolato you are correct; I will remove

@skynavga

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2018

@cconcolato addressed in dd9d190

@skynavga skynavga removed the agenda label May 30, 2018

@nigelmegitt

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

@cconcolato @skynavga is it not possible for fontShear to be applied to tate chu yoko within a span, thus affecting the horizontal extent of the horizontal text and therefore modifying the line height of the enclosing vertical text?

@nigelmegitt

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

Built version here: https://rawgit.com/w3c/ttml2/issue-0785-font-property-semantics-p-build/index.html - @cconcolato the link you posted in #799 (comment) was for a single specific build, not the latest build. Confused me for a while ;-)

@nigelmegitt

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

I've moved my comments above to #806 (comment) to prevent them from blocking this pull request.

@cconcolato

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

@cconcolato @skynavga is it not possible for fontShear to be applied to tate chu yoko within a span, thus affecting the horizontal extent of the horizontal text and therefore modifying the line height of the enclosing vertical text?

@nigelmegitt You're right

@nigelmegitt

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

@nigelmegitt You're right

@cconcolato I feel validated :-)

@skynavga skynavga merged commit b2e86bf into master Jun 7, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
ipr PR deemed acceptable.
Details

@skynavga skynavga removed their assignment Jun 7, 2018

@skynavga

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 7, 2018

Merged early per WG resolution.

@skynavga skynavga deleted the issue-0785-font-property-semantics-p branch Jun 28, 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.