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.

Sign up for GitHub

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

Jump to bottom

State that LWSPs are permitted in tts:fontFamily #290

Merged
merged 4 commits into from
Jan 4, 2018
Merged

State that LWSPs are permitted in tts:fontFamily #290

merged 4 commits into from
Jan 4, 2018

Conversation

palemieux
Copy link
Contributor

Close #248

@palemieux palemieux added this to the 3rd Ed milestone Dec 14, 2017
@palemieux palemieux self-assigned this Dec 14, 2017
nigelmegitt
nigelmegitt approved these changes Dec 14, 2017
View reviewed changes
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.

For preference I think I'd have added this note after the existing note, but the order of notes is not really important here.

skynavga
skynavga requested changes Dec 15, 2017
View reviewed changes
spec/ttml1.xml Outdated
@@ -4182,6 +4182,11 @@ as a style property only to those element types indicated in the following table
</tbody>
</table>
<note role="clarification">
<p>While permitted, the use of linear whitespace (LWSP) around the comma delimiters in the value of this
attribute is not recommended for maximal compatibilty with processor implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a processor for which this is known to cause a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't actually know of an incompatible processor, then please remove "... for maximal compatibility with processor implementations".

@andreastai andreastai self-requested a review December 17, 2017 13:29
andreastai
andreastai approved these changes Dec 17, 2017
View reviewed changes
Copy link

@andreastai andreastai 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 Does it resolve your concern if "with processor implementation"? If not, what is your change proposal?

skynavga
skynavga requested changes Dec 26, 2017
View reviewed changes
spec/ttml1.xml Outdated
@@ -4182,6 +4182,11 @@ as a style property only to those element types indicated in the following table
</tbody>
</table>
<note role="clarification">
<p>While permitted, the use of linear whitespace (LWSP) around the comma delimiters in the value of this
attribute is not recommended for maximal compatibilty with processor implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't actually know of an incompatible processor, then please remove "... for maximal compatibility with processor implementations".

@palemieux
Copy link
Contributor Author

@skynavga This PR reflects the consensus text at #248, which was the result on lengthy discussions which you were part of. Unless information has changed since the drafting of the text and/or you plan on filing a formal objection against the text of the PR, I do not see a compelling reason for the group to spend valuable time reopening this issue.

@skynavga
Copy link
Contributor

I do not accept this change as stated. If you can't point out an affected implementation, then it is mere speculation to suggest there is an incompatibility. While I accept the recommendation against use, I do not accept a motivation based on speculation.

@palemieux
Copy link
Contributor Author

@skynavga The question is whether you can still live with the consensus text at #248 , which would participated in. If not, i.e. you plan on formally objecting, the group will need to reopen the debate, which will undoubtedly take time away from closing other issues. What say you?

@skynavga
Copy link
Contributor

Feel free to put this on the WG agenda. I do not accept the current text.

@nigelmegitt
Copy link
Contributor

@skynavga you reacted with a thumbs up to #248 (comment) to indicate your preference for the option:

Permit LWSP in document instances but note that some existing processors may not be tolerant of them.

If you have a wording change that would achieves this option in a more satisfactory way to you, please propose it.

@skynavga
Copy link
Contributor

@nigelmegitt how many times shall I repeat myself?

please remove "... for maximal compatibility with processor implementations".

@nigelmegitt
Copy link
Contributor

@skynavga Right, but that's then incomplete, since removing that text means the pull request no longer addresses the consensus "but note that some existing processors may not be tolerant of them" - that's the bit that needs replacement wording please.

@skynavga
Copy link
Contributor

If nobody can point at such a processor, then that statement is factually wrong.

@palemieux
Copy link
Contributor Author

If nobody can point at such a processor, then that statement is factually wrong.

These and other many other issues were taken into account while crafting the consensus text at #248. One aspect of the consensus cannot be reopened without reopening it completely. @skynavga What say you?

@andreastai
Copy link

Could we add for example some text that says something along the line "...because of some ambiguity in TTML 2nd edition that could have led to a different interpretation" and than remove the contentious part "for maximal compatibilty with processor implementations"? Would this work for you @skynavga.

I agree with @nigelmegitt that without an explaination why this is not recommended it would be incomplete. But I also think that if we re-word it and omit the "processor part" the informative value of the note is still there.

@skynavga
Copy link
Contributor

skynavga commented Jan 3, 2018 •
edited
Loading

Firstly, I reject the notion that there has ever been an ambiguity here, or that an implementation that failed to study the relevant specifications and reached the wrong conclusion should be permitted to continue to exist.

Secondly, to my knowledge, nobody has actually pointed out the existence of such an implementation, which to me, means that the contention that a problem exists is merely conjecture.

My conclusion is that no change is necessary; however, I do not object to adding a note (for those readers who fail to do their research elsewise) that makes the most minimum disrecommendation on the off-chance that such an implementation exists, but I am not willing to make reference to the speculated existence of such an implementation.

Please show me such an implementation, and I would be willing to change my mind.

skynavga
skynavga approved these changes Jan 4, 2018
View reviewed changes
Copy link
Contributor

@skynavga skynavga left a comment

Choose a reason for hiding this comment

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

Approve with the changed text, and comment yet again that it is not a requirement, and in my opinion, poor practice to document rationale.

@palemieux palemieux merged commit 470a998 into master Jan 4, 2018
@skynavga skynavga deleted the issue-248-LWSP-fontFamily branch March 11, 2018 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skynavga skynavga skynavga approved these changes

@andreastai andreastai andreastai approved these changes

@nigelmegitt nigelmegitt nigelmegitt approved these changes

Assignees
No one assigned
Labels
editorial pr merged
Projects
None yet
Milestone
3rd Ed CR
Development

Successfully merging this pull request may close these issues.
4 participants
@palemieux @skynavga @nigelmegitt @andreastai