Add initial value fallback to non-inheritable properties #298
Conversation
spec/ttml1.xml
Outdated
| type <el>region</el>, then set <emph>P′</emph> to the initial value of | ||
| next style property;</p></item> | ||
| <item><p>if <emph>P</emph> is inheritable and <emph>E</emph> is a <el>region</el> element | ||
| and, or if <emph>P</emph> is not inheritable and applies to <emph>E</emph>, then set <emph>P′</emph> to the initial value of |
There was a problem hiding this comment.
seems to be an unnecessary "and" at the beginning of this line.
There was a problem hiding this comment.
Some properties do not have an initial value specified, and that's not a spec error.
spec/ttml1.xml
Outdated
| property <emph>P</emph>, where the initial value of a property is | ||
| determined according to the specific property definition found above | ||
| in <specref ref="styling-attribute-vocabulary"/>;</p></item> | ||
| <item><p>if the element type of <emph>E</emph> is a <loc href="#element-vocab-type-content">Content</loc> element | ||
| <item><p>if if <emph>P</emph> is inheritable and the element type of <emph>E</emph> is a <loc href="#element-vocab-type-content">Content</loc> element |
| @@ -7070,20 +7070,20 @@ set of <emph>E</emph>, <emph>SSS(E)</emph>;</p></item> | |||
| <item> | |||
| <p><phrase role="strong">[implicit inheritance]</phrase> if the | |||
There was a problem hiding this comment.
I would prefer as a matter of style not to mix the setting of initial values of non-inheritable style properties into the implicit inheritance steps. Rather, I think the resolution to this issue would be better as a new step 7, such as [apply initial property values] along the lines of:
7. [apply initial non-inheritable style property values] if the element type of E is not the styling element type style, then for each non-inheritable style property P in the set of style properties defined above in 8.2 Styling Attribute Vocabulary, perform the following ordered sub-steps:
a. if P is present in the specified style set of E, SSS(E), then continue to the next style property;
b. if P applies to E, then set P′ to the initial value of property P, where the initial value of a property is determined according to the specific property definition found above in 8.2 Styling Attribute Vocabulary, and is undefined if no initial value is defined;
c. if the value of P′ is not undefined, then merge P′ into the specified style set of E, SSS(E).
There was a problem hiding this comment.
@nigelmegitt This would not match TTML2 ED. I think we should keep this structure, and file a separate ticket to consider updating both TTML2 and TTML1 to match your suggested structure.
There was a problem hiding this comment.
OK I hadn't spotted that @palemieux , happy to go with your suggestion. Thanks for the pointer.
spec/ttml1.xml
Outdated
| <item><p>if the element type of <emph>E</emph> is the layout element | ||
| type <el>region</el>, then set <emph>P′</emph> to the initial value of | ||
| next style property;</p></item> | ||
| <item><p>if <emph>P</emph> is inheritable and <emph>E</emph> is a <el>region</el> element, or if <emph>P</emph> is not inheritable and applies to <emph>E</emph>, then set <emph>P′</emph> to the initial value of | ||
| property <emph>P</emph>, where the initial value of a property is | ||
| determined according to the specific property definition found above | ||
| in <specref ref="styling-attribute-vocabulary"/>;</p></item> |
There was a problem hiding this comment.
Even if not using my suggestion at https://github.com/w3c/ttml1/pull/298/files#r157463436 right now, I think it would still be useful to note that if an initial value is absent from the property definition, then the specified value falls into the undefined class.
There was a problem hiding this comment.
Do you mean replace then set P′ to the initial value of property P, with then set P′ to the initial value of property P if one exists, ?
There was a problem hiding this comment.
That would work but it would be a bigger change, since it would also mean that the step "if the value of P′ is not undefined, then merge P′ into the specified style set of E, SSS(E)." would need to be changed as well.
The smaller (and therefore usually preferable) change is simply to add something like the following to the end of the line this comment thread begins on:
(for properties that do not define an initial value, the value of P′ is undefined)
There was a problem hiding this comment.
@nigelmegitt if no initial value of property P exist than P' is not set, therefore P' remains undefined.... or is there something I missed?
There was a problem hiding this comment.
@palemieux The point I'm trying to make is that the text directs the reader to the initial value of the property but for some properties no initial value is defined. In that case, what to do? I think it would be a helpful addition to clarify a) that such cases exist deliberately and b) that when they arise, P' remains undefined.
There was a problem hiding this comment.
@nigelmegitt then set P′ to the initial value of property P if one exists seems to cover the case where no initial value is defined, since the clause would not be applied if no initial value exists, which would leave P' undefined... or am I missing a corner case... or is it a personal style issue?
There was a problem hiding this comment.
@palemieux Maybe I've misunderstood, but I thought that if you do that ["if one exists"] then SSS(E) would not contain P', whereas if you adopt my approach SSS(E) would contain P', with P' set to an "undefined" class. So it seemed like a subtle but substantive difference.
There was a problem hiding this comment.
@nigelmegitt There is no "undefined" class in SSS(E). AFAIK "undefined" refers to the variable P' never having been set.
There was a problem hiding this comment.
@palemieux OK, I see what you mean. If P' is undefined then it never gets added to SSS(E). So we should say:
set P′ to the initial value of property P if such an initial value is defined
I'm wary of saying "if one is defined" in case of ambiguity about what the "one" refers to.
There was a problem hiding this comment.
@nigelmegitt I have updated the PR per #298 (comment)
There was a problem hiding this comment.
Thanks, looks good to me.
A minor nit would be that we've ended up with two "ifs" rather than simply an additional "and an initial value is defined for P" clause. I think that may have been partially my fault...
Still, this is better than what was there before.
Closes #200
See also #217