TTML1 3ED tests - #361
Merged
Merged
Conversation
nigelmegitt
reviewed
Aug 9, 2018
| <body> | ||
| <div> | ||
| <p begin="0s" end="1s" timeContainer="seq" tts:backgroundColor="blue"> | ||
| No text should be displayed. |
Contributor
There was a problem hiding this comment.
Fine point: maybe change to "This text should not be displayed" so if it is displayed that's clearly an error.
skynavga
reviewed
Aug 9, 2018
Contributor
There was a problem hiding this comment.
Regarding line height tests, the new text that drives this test, "When a single relative <length> ...", was previously implied in TTML1 2e, as evidenced by the second sentence in the fourth comment in 8.2.9 of 2e.
Member
|
The Working Group just discussed
The full IRC log of that discussion<nigel> Topic: TTML1 3ED tests ttml1#361<nigel> github: https://github.com//pull/361/files <nigel> Pierre: I just pushed it a couple of hours ago so I don't expect anyone to have had a <nigel> .. thorough look. I've gone through all the substantive tests since 2ed and created tests <nigel> .. for everything that can be tested and highlighted what I think cannot be tested. <nigel> .. The tests are inspired by the IMSC tests so they will seem familiar. <nigel> .. I'd like a review especially on the things marked as non-testable. <nigel> .. From a practical perspective, if Glenn could try to render or check them using TTPE and <nigel> .. TTX that would be great. Last time we spoke the plan was for TTPE and IMSC.js to be the <nigel> .. two implementations for the TTML1 3ed tests. <nigel> Cyril: How many of these tests are relevant for IMSC 1.1? <nigel> Pierre: First, there may be zero IMSC 1.1 tests because everything in IMSC 1.1 is already <nigel> .. in TTML2 or IMSC 1.0.1, so there will be no additional tests needed to meet the IMSC 1.1 <nigel> .. exit criteria. <nigel> Cyril: I understand that, but how many of the tests for TTML1 3ed are relevant for IMSC 1.1 <nigel> .. features? <nigel> Pierre: All but one are covered by IMSC 1.1. But I think the purpose of those tests if I recall <nigel> .. correctly was specifically to convince the Director that the substantive changes were in <nigel> .. fact implemented. <nigel> Nigel: That's right. <nigel> Pierre: I created those tests specifically to demonstrate that. All but one are already <nigel> .. exercised by IMSC1 tests and TTML1 tests in fact. <nigel> Cyril: Which is the exception? <nigel> Pierre: Anamorphic fonts. There's one test that is triggered by anamorphic fonts. <nigel> Cyril: 2 value relative font size? <nigel> Pierre: Exactly. That one is not part of IMSC1 or 1.1 and I'm not even sure it was part of <nigel> .. TTML1 test suite either. <nigel> Cyril: Ok, thank you. <nigel> Glenn: Q: what did we change in the spec that that particular test is used to demonstrate? <nigel> Pierre: If you recall, we added a bunch of text that discussed inheritance. <nigel> .. Example, p fontSize = "1c", then child span fontSize="1em". The font size is calculated <nigel> .. to be 1c, that's boring. <nigel> .. There are two examples in the TTML1 text that describe this. <nigel> -> https://www.w3.org/TR/ttml1/#style-attribute-fontSize fontSize in TTML1 <nigel> Nigel: The examples are in Notes? <nigel> Pierre: Yes <nigel> .. The 3rd and 4th note in that section. <nigel> .. "1em 1em" can result in an anamorphic font size. <nigel> .. The relative font size is relative to the computed parent font size which can be anamorphic. <nigel> Glenn: What normative text changed that drove adding that test? <nigel> Pierre: The entire text of that section was heavily changed. <nigel> Glenn: I'm just wondering if we went too far in creating that test or if the original test <nigel> .. suite was under-represented. <nigel> Pierre: The 5th paragraph, "When a single relative <length> value is specified, ..." <nigel> .. If that is in TTML1 2nd Ed then I agree there's no need for tests, but I'm thinking it was not. <nigel> Glenn: I'll check. <nigel> Pierre: The two that can't be tested or don't need to be tested are: <nigel> .. 1. 'should' regarding the tts:lineHeight. <nigel> .. 2. application defaults for frame rate and sub-frame rate. <nigel> .. I don't think that's testable. <nigel> Nigel: Presumably we could include the application settings to apply for testing frame rate <nigel> .. and sub-frame rate, for example in text outside the TTML document instance, then the <nigel> .. same instance would have a different evaluation in some way? <nigel> Pierre: Sure, but that in itself, the decision to apply an application default, has no requirement <nigel> .. so I don't think it's testable. <nigel> Nigel: In other words we have merely made explicit the already existing option for an <nigel> .. implementation to do its own thing? <nigel> Pierre: Yes, that's right. <nigel> Nigel: That seems like a reasonable argument to me. What about lineHeight. <nigel> .. Why not be able to test that? <nigel> Pierre: It's a should. <nigel> Nigel: But the semantic is still testable. <nigel> Pierre: Yes but applications can be conformant without doing it. <nigel> Nigel: Yes but we can still test the semantic. <nigel> Pierre: The syntax is unchanged, and it's a should. <nigel> Nigel: Yes but the test needs to demonstrate implementability, so there needs I think to be <nigel> .. some test that shows the should behaviour can be implemented. <nigel> Glenn: If normal already appears in any of the TTML1 tests then we don't need a new test <nigel> .. for this. <nigel> Nigel: Why not? <nigel> Glenn: If normal is already there then that test for how normal is used, with no normative <nigel> .. or exemplar images... <nigel> Pierre: I don't understand - say tts:lineHeight="normal" and an implementation returns to <nigel> .. you something with a line height that is double the font size, what do you conclude? <nigel> Nigel: Depending on the algorithm in the spec and the font resources, on balance most <nigel> .. likely that implementation is not demonstrating that the spec can be implemented. <nigel> Pierre: It's only a recommendation. <nigel> Nigel: If the spec said "should go back in time by 10 minutes" then the Director would want <nigel> .. a test to show that, but of course no time machine exists, so I think that text would <nigel> .. lead to trouble. <nigel> Pierre: Good news, there already is a test for lineHeight="normal". <nigel> Glenn: As I said. <nigel> Pierre: I wanted to agree the scope first. <nigel> Glenn: I wonder if it would be consistent if we add an exemplar to a small subset of TTML1 <nigel> .. tests. <nigel> Nigel: I don't think we need to worry about that. <nigel> Pierre: My proposal was just to check in the test files without exemplars, but file the outputs <nigel> .. generated from our implementations under the implementation report. <nigel> Glenn: Ok. When is the last date for this? <nigel> Nigel: Thanks to Thierry, <nigel> -> https://www.w3.org/AudioVideo/TT/specs-timeline.html Timeline <nigel> Nigel: Shows that implementation report needs to be finalised by 24th September. <nigel> Pierre: Can we give ourselves until August 30th to review those tests and make changes, <nigel> .. then freeze them then create the implementation report, and then start submitting results. <nigel> .. Just a suggestion. <nigel> Glenn: TTPE implements normal as described as well as anamorphic fonts so I think we're <nigel> .. good there but I'd need to run the tests to verify that. I don't think there's any problem <nigel> .. in using TTPE as one of the implementations to verify those. <nigel> Nigel: I like Pierre's suggested plan, any issues with that? <nigel> group: [silence] <nigel> Nigel: Okay, then agenda+ for 30th August to confirm the TTML1 3ed test suite so we can <nigel> .. begin to create the implementation report. <glenn> +q <nigel> Nigel: [discusses tests, with comments on the pull request] <nigel> ack g <nigel> glenn: I just double checked the anamorphic font text, and it turns out that it is basically <nigel> .. present in 2nd Ed in the 4th note in §8.2.9, in the last sentence. So this is basically a <nigel> .. paragraph of text in a note already, now made normative whereas it was more an <nigel> .. explanation of an implication in 2nd Ed. <nigel> Pierre: So do we need the tests still? <nigel> Glenn: In my opinion the semantic was already there and we're not demonstrating a new <nigel> .. semantic. <nigel> Pierre: I'm not excited by it. <nigel> Glenn: I'd be willing to have no test and point to the 2nd Ed text for that. <nigel> github: https://github.com//pull/361 <nigel> Nigel: I think the Director was asking for tests to demonstrate the substantive changes, <nigel> .. and this counts as one because normative text clarifies what may have been ambiguous <nigel> .. before. <nigel> .. We need to look more carefully at this to see if an existing TTML1 test for two value fontSize <nigel> .. can be reused or already demonstrates this. <nigel> Glenn: I added a comment under this pull request. <nigel> Nigel: This needs further investigation - I see that the diff tool isn't helping us. <nigel> Nigel: in the direction test, shouldn't the direction on the first p be direction="ltr"? <nigel> Pierre: That is a subtle point that 3ed clarifies - without bidiOverride the "natural" direction <nigel> .. of the script is not overridden. <nigel> Glenn: Where that comes into effect is resolving the directionality of weakly directional <nigel> .. or neutral directional characters at the boundaries of the paragraph, like the period that <nigel> .. ends the paragraph is neutral directionality. If the paragraph embedding level is ltr then <nigel> .. a period at the end of a hebrew or arabic sentence takes on the direction of the previous <nigel> .. character, but there are scenarios where it doesn't work. On this point, previously in <nigel> .. 2ed and prior we didn't call out this semantic but probably many implementations <nigel> .. implemented it as we have now clarified. Since we did not say either way, some <nigel> .. implementations may have read between the lines and applied it to p which would not <nigel> .. have been non-conformant since we had no test suite examplars to follow. It could have <nigel> .. fallen through the cracks. <tmichel> FYI you may use the following diff tool <tmichel> http://www.aptest.com/standards/htmldiff/htmldiff.pl?oldfile=https%3A%2F%2Fwww.w3.org%2FTR%2F2013%2FREC-ttml1-20130924%2F&newfile=https%3A%2F%2Fwww.w3.org%2FTR%2F2018%2FCR-ttml1-20180424%2F <cyril> q+ <nigel> Glenn: The XSL spec says to apply it this. <nigel> ack c <nigel> Cyril: 2 comments about the test suite. <nigel> .. Some tests have a copyright. <nigel> Pierre: Yes, I thought I'd removed them and didn't. Please add a comment. <nigel> Cyril: Secondly, in the pull request you've done the work to link the issues to the tests, <nigel> .. but there's no link backwards. I wonder if we should put metadata in the tests to point <nigel> .. to the issues or the spec sections that it is trying to test. <nigel> Pierre: Yes that'd be awesome. <nigel> Nigel: Is that information available? <nigel> Pierre: Yes it's in the pull request. Most of the tests have a metadata section so it's a matter <nigel> .. of copying and pasting the pull request info into the metadata header. If someone wants <nigel> .. to spend 45 minutes doing it that'd be awesome. <nigel> Cyril: Going a bit further, referencing the spec itself? <nigel> .. I suppose the issue goes to the pull request goes to the section of the text. <nigel> Pierre: Exactly. Copying the bullet point is all we need to do. <nigel> Nigel: Any other points on this test suite? <nigel> SUMMARY: Test suite to be finalised August 30th, review to continue until then. <nigel> Glenn: I would note that August 30th is prior to when it is actually needed so we have <nigel> .. some room to slip that if necessary. <nigel> Nigel: Yes but there has to be time for the implementations to respond to the tests. <nigel> Glenn: Yes <nigel> Pierre: The goal is to freeze the tests so implementers can work on them. <nigel> Glenn: It's a good goal, just not an absolute hard deadline. <nigel> Nigel: It's a target, and as Cyril mentioned there is other work to do at the same time. <nigel> .. If we can freeze the tests on 30th then that gives 3 weeks for implementations, which <nigel> .. seems reasonable. <nigel> Pierre: If you can run the validator that would be good, Glenn. <nigel> Glenn: Sure I can do that, and check what TTPE does as well. |
nigelmegitt
reviewed
Aug 9, 2018
Contributor
There was a problem hiding this comment.
Partial review due to lack of time.
| @@ -0,0 +1,15 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <!--Modifications Copyright (c) 2017, Pierre-Anthony Lemieux (pal@palemieux.com)--> | |||
Contributor
There was a problem hiding this comment.
Really? I don't think I've seen a copyright notice on a test suite test before.
Contributor
Author
There was a problem hiding this comment.
Leftover from a long time ago. Fixed at 32d0ae1
| <div> | ||
| <p tts:direction="rtl"> | ||
| Left to right<br/> | ||
| משמאל לימין<br/> |
Contributor
There was a problem hiding this comment.
This text means "left to right" in English.
Contributor
Author
There was a problem hiding this comment.
מימין לשמאל works better?
Contributor
There was a problem hiding this comment.
Confirmed with a native Hebrew speaker, that's good.
Contributor
There was a problem hiding this comment.
It means "From right to left".
nigelmegitt
requested changes
Sep 5, 2018
Contributor
There was a problem hiding this comment.
Some changes requested, mainly these tests look good.
| <p begin="0s" end="1s" timeContainer="seq" tts:backgroundColor="blue"> | ||
| No text or linebreak<br/>should be displayed. | ||
| </p> | ||
| <p begin="0s" end="1s" tts:backgroundColor="blue"> |
Contributor
There was a problem hiding this comment.
Change to a different background color to distinguish from first paragraph please, as discussed.
| </head> | ||
| <body> | ||
| <div> | ||
| <p begin="0s" end="10s">The last words must be <span tts:fontFamily='InexistantFont, proportionalSerif'>using a proportional serif font</span>.</p> |
Contributor
There was a problem hiding this comment.
Unusual use of single quotes here, why not use double quotes as per other attributes and examples?
Member
|
The Timed Text Working Group just discussed
The full IRC log of that discussion<nigel> Topic: TTML1 3ED tests ttml1#361<nigel> github: https://github.com//pull/361 <nigel> Nigel: This was opened 29 days ago, and it took a while to review. Looking at the review <nigel> .. status, one person has requested changes, and that was me! <nigel> .. Most of the changes I requested are minor. <nigel> Pierre: Are they all necessary? <nigel> Nigel: We need to check we have a shared understanding of the intent of the zIndex text. <nigel> Nigel: I don't think the missing EOLs on the ends of the files are a big deal. <nigel> .. The use of single quotes around an attribute is not a big deal. <nigel> Pierre: I think that must have come from the original TTML1 test. <nigel> Glenn: If a font family name consists of more than one token then you need to quote it <nigel> .. so you might see both sets of quote characters in that attribute, the outer one for the <nigel> .. attribute itself and the inner one for the family name with multiple tokens. <nigel> .. In this case there are no multiple token family names. <nigel> Nigel: Conversely there's no requirement to change it. <nigel> Glenn: That's right, ok. <nigel> Pierre: Can you propose a pull request for the BrImplicitDuration.ttml test? <nigel> Nigel: It's just, as discussed, to change the backgroundColor of one of the paragraphs. <nigel> Pierre: You wanted to change the text too? <nigel> Nigel: Oh, that's another solution, I think the background color change is a more elegant <nigel> .. solution to the same issue, and we don't need to do both things. <nigel> Pierre: On Direction.ttml... <nigel> Nigel: We discussed this on 9th August, my proposal is just an XML comment that <nigel> .. explains that the test intentionally sets direction="rtl" and that the text "Left to right" <nigel> .. appears correctly that way because tts:direction only sets weak directionality. <nigel> Pierre: Can you propose the comment text? <nigel> Nigel: Sure. <nigel> Pierre: The next one is about the hebrew text. <nigel> Nigel: I just added that comment as a note for posterity. <nigel> Pierre: I have proposed some alternate text that says "Right to left" which is how the Hebrew <nigel> .. text should appear. <nigel> Nigel: Okay I'll check it out with an Israeli colleague. <nigel> s/appear/appear (taken from Google translate). <nigel> Nigel: In PlainSpanImplicitDuration.ttml I suggested a small wording change. Would that <nigel> .. be an issue to make that change? <nigel> Pierre: That's good, I'll make that change. <nigel> Nigel: ZIndex.ttml is the next one. <nigel> Pierre: The issue is only about the stacking relative to the root container so it does not <nigel> .. matter if the regions do not overlap. <nigel> Nigel: I don't really understand this test - what is it showing? <nigel> Pierre: "Does a region with tts:zIndex < 0 appear underneath the root container?" <nigel> Nigel: What does that even mean? <nigel> Pierre: I don't know, what does it mean if a region has a negative zIndex? <nigel> Glenn: The definition of zero is in CSS, a reference frame (don't have it here). Negative <nigel> .. indices are permitted in CSS. I don't remember what that meant. <nigel> Pierre: The spec modification is about the root container region establishing the root of <nigel> .. the stacking context. The previous spec said "auto" was defined by XSL 1.1 but did not <nigel> .. state that the semantics for values other than auto were also defined by XSL and it did <nigel> .. not say that the tt element established a root stacking context for scenarios other than <nigel> .. zIndex being "auto". The new text says the tt element always establishes a root stacking <nigel> .. context. <nigel> Glenn: By the way if you go back to CSS 2.1 it says that for integer types it always establishes <nigel> .. a new stacking context and for auto it does not unless it is a root element, so we needed <nigel> .. to say what the root element was. It was only adding useful information in the case of auto. <glenn> https://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#propdef-z-index <nigel> Pierre: We've had this discussion, the issue is the previous text did not state what was the <nigel> .. root context. <nigel> Nigel: What does the text show? <nigel> Pierre: It makes sure that all those values yield to a region that is displayed. <nigel> Nigel: Right, so the relative zIndex is not important, it is just that they should all appear? <nigel> Pierre: Yes, that is how it is constructed. <nigel> Nigel: Thank you, I will propose text in the metadata that states that, so people coming <nigel> .. to this in the future can understand what we were demonstrating. <nigel> .. Is that ok? <nigel> Pierre: Yes, absolutely. <nigel> .. The "left to right" data could be in the metadata too. <nigel> Nigel: Yes, I was thinking the same thing. <nigel> SUMMARY: Changes to be made over the next 24 hours. <nigel> Pierre: In terms of implementation, TTPE renders them all. <nigel> Glenn: I didn't check TTPE does too, but will do that ASAP. <nigel> Pierre: TTV already validates all of them, so the one that would be ideal would be to check <nigel> .. that TTPE does too, especially for two value font sizes. <nigel> Nigel: Ideally, since the changes don't affect validation, we should have two presentation <nigel> .. implementations for each change. <nigel> Pierre: We don't have a second implementation for two value fonts. <nigel> Glenn: In Geneva I was able to run the old DFXP Viewer which supported anamorphic fonts. <nigel> .. We signed off on that for the initial IR for first edition. <nigel> Nigel: If that shows the behaviour we want, it would be good enough. <nigel> Glenn: Why are we testing this? <nigel> Pierre: We made a substantive change in a way that we expect matches implementations. <nigel> .. I believe it should work with old implementations. <nigel> Glenn: Ok <nigel> .. By the way that was a different organisation than Skynav at that time. <nigel> .. It was Extensible Formatting Systems Inc, XFI, which has now closed its business. <nigel> .. Skynav inherited the source code for it. The team that implemented it is no longer with <nigel> .. Skynav, although I was one of the implementers. <nigel> Pierre: When can we determine if we have a real problem here? <nigel> Nigel: I'd like to check there is no validation change. <nigel> Pierre: There is no validation change. <nigel> Glenn: This begs the question why we have the test here. <nigel> Pierre: We discussed this, the change is to clarify the intent. <nigel> Nigel: The Director asked for tests on the substantive changes. <nigel> .. I wonder if the Flash player implements anamorphic fonts. <nigel> Glenn: You could ask Andrew Kirkpatrick. <nigel> Nigel: OK I'll send him a message. <nigel> Pierre: If there's no independent implementation of a presentation engine with <nigel> .. anamorphic fonts, what happens? <nigel> Nigel: We only need to show that existing implementations already exhibit the clarified <nigel> .. behaviour, that's why I've been asking about other implementations. <nigel> Pierre: I'll see if I can drive DFXP Viewer to work with this test. |
Add or improve descriptive metadata and in the case of `PlainSpanImplicitDuration.ttml`, clarify the text that should (or should not) appear.
nigelmegitt
approved these changes
Sep 7, 2018
Contributor
|
@palemieux please check you are happy with the changes made in b1753b0 as per the TTWG meeting yesterday. |
Contributor
Author
|
@skynavga Are you arguing at #361 (review) that |
Contributor
|
Reviewing #361 (review) and comparing 2e with 3e, the substantive change is essentially a movement of text from an informative note to a normative section. Have we got any evidence that the clarified behaviour is what was already implemented? I believe that is what the Director is seeking. The alternative would be that some implementation supports 2 value font sizes but uses a different basis for calculating the font size dimensions, for example always using the height of the cell as the basis even for the horizontal component. |
Contributor
|
Travis build stalled on this, I've restarted it. |
Contributor
|
Build succeeded. |
Changed xml extenstion to ttml
Contributor
Author
@skynavga Can you confirm that 3ED captures current TTPE behavior? |
nigelmegitt
approved these changes
Sep 13, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Provides tests for all testable changes from TTML1 2ED to TTML1 3ED, as follows:
TextOutline.ttml
LineHeight.ttml
FontSizeEmInherited.ttml
TwoValueRelativeFontSize.ttml
FontSizeEmRegion.ttml
brelements (Specify implicit duration of <br> #339)BrImplicitDuration.ttml
pelements (Backport text from TTML2 about the use of tts:direction at paragraph level #324)Direction.ttml
Color.html
UnknownTTAtribute.ttml
set(Implicit durations of <set> and <br> not specified #310, Inconsistent implicit duration of singleton span in sequential container. #193)AnonymousSpanImplicitDuration.ttml
PlainSpanImplicitDuration.ttml
SetImplicitDuration.ttml
FontFamily.ttml
NonInheritedInitial.ttml
brto elements to which style applies (style namespace attributes in br #249)Br.ttml
not testable
brelements (The [Construct Intermediate Document] process erroneously prunes empty <br> elements. #197)Br.ttml
ForwardInterop.ttml
Animation.ttml
LineHeight.ttml
Visibility.ttml
TextDecoration.ttml
TextOutline.ttml
ZIndex.ttml