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

Emphasize null style semantics in context of ruby container spans (#1100). #1101

Merged
merged 9 commits into from
Jul 11, 2019

Conversation

skynavga
Copy link
Collaborator

Closes #1100.

@skynavga skynavga added this to the 2ED-FPWD milestone May 26, 2019
@skynavga skynavga self-assigned this May 26, 2019
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.

I'm neutral on this one. I don't think there's any incorrect statement here, but at the same time it is not clear to me that the TTWG has consensus to provide this information solely in this way. I'd like views of others like @palemieux , @cconcolato , et al.

Copy link
Contributor

@palemieux palemieux left a comment

Choose a reason for hiding this comment

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

As detailed in meeting minutes and at #1069 and #1043, this note should be replaced by a positive statement in each affected style property that the style property does not apply to ruby containers.

spec/ttml2.xml Outdated Show resolved Hide resolved
@skynavga
Copy link
Collaborator Author

skynavga commented Jun 2, 2019

Although I am generally against repeating the same text when the reader has access to a common declaration, I have, by way of a compromise, added (in 9e2fe63) the text "; and see Note" to each of 15 style property's applies to declaration, where "Note" is a link to the note added by this PR to 10.2.35.1.

Note that three style properties that Pierre had proposed constraining cannot be constrained because these properties apply to ruby container spans as well:

  • tts:direction
  • tts:unicodeBidi
  • tts:wrapOption

Therefore, I did not qualify the applies to declaration for these properties.

@skynavga skynavga requested a review from nigelmegitt June 3, 2019 13:51
Copy link
Contributor

@palemieux palemieux left a comment

Choose a reason for hiding this comment

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

Requesting changes on all PRs to prevent their closing before 14-day windows

@skynavga
Copy link
Collaborator Author

skynavga commented Jun 7, 2019

@palemieux Re: #1101 (review), this is not a legitimate request as applied to editorial PRs, at least as far as our standing protocol applies. If you need more time to review a specific PR that may subject to early merger, then please state your reasons as to why more time is required. Thanks.

@palemieux
Copy link
Contributor

@skynavga The standing protocol only applies to those changes that are determined editorial by consensus, not by unilateral action. In any case, this PR as a much larger deficiency, as discussed at #1101 (review) .

@skynavga
Copy link
Collaborator Author

skynavga commented Jun 7, 2019

@palemieux if there is an approval on an issue marked as editorial, and there is no objection to it being marked as editorial, then it falls under the early merger policy; if you would like to review this policy, please raise it to the group in session; regarding this particular PR (#1101), I have no intention to merge it early, whether or not an approval has been given;

@palemieux
Copy link
Contributor

@skynavga I think you have two choices: take your time now to gather consensus over proposed resolutions, or faced potentially lengthy delays sooner or later.

@skynavga
Copy link
Collaborator Author

skynavga commented Jun 7, 2019

@palemieux qui tacit consentire videtur; if I have an approval on an editorial PR, I apply my own judgment as to whether sufficient time has passed and the level of potential controversy on a case by case basis; I believe this is consistent with this WGs operating policy and past history; if you want to propose a change to that policy, then feel free to do so;

@skynavga
Copy link
Collaborator Author

skynavga commented Jun 7, 2019

@palemieux I will leave your blocker in place for this issue; however, a simple request as a comment to allow this PR to go to full term would have sufficed, and would have been the more collegial approach

@nigelmegitt
Copy link
Contributor

It seems like now might be a good time to summarise, because the current state and the original direction may seem surprising (they do to me).

In response to a request to be more specific about the elements to which the style attributes apply, by changing the text in the Applies To column, @skynavga fought hard against doing that on the basis that (summarising from memory, apologies if this is inaccurate) it would add maintenance, and also that it is unnecessary.

It seems like everyone is agreed that the application of the relevant style attributes to the "ruby containers" (shorthand term) has no effect, so it is academic whether they are included in the "applies to" or not, except that the original issue said that it is helpful to implementers to know from the beginning that some style attributes never apply to those "ruby containers", because then they can be excluded from processing.

This pull request has now got to the point where there is additional text on the "applies to" rows (so the maintenance requirement has not been avoided) and the text it points to says that they "have no semantic affect" during presentation processing, confirming that we are all in agreement on that point.

The decision we need to make as a group is whether we want to express this non-effect as:

  1. an informative consequence of other normative text (as in this pull request) or
  2. as an additional normative categorisation as in Clarify which style properties do not apply to ruby containers #1071.

For myself, I'm not sure that there is any significant difference between these approaches, or that there is anything testable that would result from either one or the other approach being taken (which suggests the informative approach is adequate). As always, views welcome. I would like us to come to a conclusion on this fairly rapidly in Thursday's call.

@skynavga
Copy link
Collaborator Author

@nigelmegitt thanks for that summary, with which I largely agree; I would merely add the following remarks:

it is helpful to implementers to know from the beginning that some style attributes never apply to those "ruby containers", because then they can be excluded from processing

  • It is important to note that this "helpful" statement takes the form of suggesting an optimization that an implementer may or may not choose to employ. That is to say, an implementation would be fully compliant if it completely ignores such an optimization. This point is at the core of why adding a normative constraint to applies to is untestable.

This pull request has now got to the point where there is additional text on the "applies to" rows (so the maintenance requirement has not been avoided)

  • My preference is to add nothing to the applies to entries; however, I proposed the current text "see also Note" as a compromise in the case that the group feels something is needed. I can live with this note, but I do believe it has a (possibly unwarranted) maintenance overhead; for example, do we need to add the same note to other properties, such as tts:color? or future tts:font* or tts:text* properties?

@skynavga
Copy link
Collaborator Author

skynavga commented Jul 9, 2019

@nigelmegitt I should add that it appears to me that, by continuing to reopen this PR and not creating a new PR, you are attempting to circumvent my ability to review the changes now represented by this PR; that appears to completely circumvent our standing protocol and agreements that afford members the opportunity to review PRs they didn't author; even if you do create a new PR using the changes from this PR, and I post a "changes requested" review, you can still go through a process of dismissing my review without it being satisfied; however, I would expect that is an action you cannot take unilaterally as chair, but must have the WG collectively decide to dismiss a review.

@skynavga
Copy link
Collaborator Author

skynavga commented Jul 9, 2019

@nigelmegitt regarding #1101 (comment), I explicitly cite CSS2 in #1101 (comment); as far as I know, no subsequent version of CSS or a CSS module has redefined applies to as defined in CSS2; for example, CSS2.2, currently in FPWD status retains the same language (https://www.w3.org/TR/CSS22/about.html#applies-to);

regarding your second bullet "in another case", could you provide a link to the text you cite?

@skynavga
Copy link
Collaborator Author

skynavga commented Jul 9, 2019

@nigelmegitt regarding your claim in #1101 (comment)

I have requested an explanation multiple times and so far never been satisfied

Did I not ask in #1101 (review)

For example, what precisely is the interoperability problem with tts:color that these changes propose to solve? The same question applies for the other 15 properties. What is the interoperability problem?

I have not seen an answer to this question. How can I provide a technical counter-argument when the proponent of the change hasn't provided a technical argument for the change in the first place?

I have not seen any response from @palemieux to your question in #1101 (comment):

@palemieux is that correct, or is there an interop problem with the remaining attributes whose text is informatively modified by this pull request?

Is there an interop problem with these 15+1 properties or not? I would like to see an answer from @palemieux either way: yes or no. If yes, then what is the interop problem? If no, then what is the technical argument in favor of the change (to these 15+1 properties)?

Unless I have answers to these questions, then I can't begin to write a technical counter argument, and it is inconsistent of you to demand that I do so.

@nigelmegitt
Copy link
Contributor

@skynavga responses to multiple comments:

you are attempting to circumvent my ability to review the changes now represented by this PR

On the contrary, I explicitly noted your objection. The ability to review and object are not well supported by GitHub pull requests in the case that a pull request is modified by someone other than the original author, I agree, but you still had the opportunity to object, and took it. I asked you in #1101 (comment) what changes you wanted to make, and you did not answer that question.

regarding your second bullet "in another case", could you provide a link to the text you cite?

It's the same link, the two cases I mentioned were different entries in the table. The first case was for text-combine-upright, the second was for writing-mode.

Regarding interop, this was discussed on Thursday and it was my understanding from that call that the interop issue primarily affects the attributes that we agreed to move to a separate issue. Is that not correct @palemieux ?

@palemieux palemieux reopened this Jul 10, 2019
@skynavga
Copy link
Collaborator Author

@palemieux please do not reopen this PR; I opened this PR, therefore, I own this PR, and I am closing it; feel free to open another PR which makes use of the same branch as this PR;

@skynavga skynavga closed this Jul 10, 2019
@skynavga
Copy link
Collaborator Author

@palemieux note that you can continue to add or edit comments on this PR even while closed;

@palemieux
Copy link
Contributor

Regarding interop, this was discussed on Thursday and it was my understanding from that call that the interop issue
primarily affects the attributes that we agreed to move to a separate issue.

This pull request is intended to address issue #1043, which was filed following a concrete interop issue (#1043 (comment)): does tts:textDecoration apply to ruby span containers? The same question applied to style properties beyond tts:textDecoration.

I would personally prefer tackling tts:direction, tts:unicodeBidi and tts:wrapOption today, in addition to those style properties already addressed by this pull request, but do not object waiting until later (as discussed last Thursday) since:

  • these three properties are unlikely to be used with ruby span containers in practice (at least in the short-term)
  • communication with CSS WG might be useful
  • interaction between a style property like tts:wrapOption and ruby containers might require careful consideration

@palemieux palemieux reopened this Jul 10, 2019
@palemieux
Copy link
Contributor

please do not reopen this PR; I opened this PR, therefore, I own this PR, and I am closing it;

@skynavga This PR is no more yours than mine. It belongs to the group.

@skynavga
Copy link
Collaborator Author

@palemieux there is no longer an interop issue with tts:textDecoration since the merger of PR #1077; are you saying there is another interop issue that is outstanding? what interop issue remains unaddressed with any of the 15+1 properties modified by this PR?

@skynavga skynavga closed this Jul 10, 2019
@skynavga
Copy link
Collaborator Author

@palemieux it is silly of you to keep opening this PR when I am just going to keep closing it; please discuss with the chair before reopening again;

@skynavga
Copy link
Collaborator Author

skynavga commented Jul 11, 2019

After reviewing the state of the changes proposed in this PR, including those made by @palemieux, I have determined I can accept an additional (informative) qualification of the applies to entry of the 16 style property definition tables modified by this PR provided that the redundant paraphrase of the referenced note is removed in each case. Accordingly, I have updated this PR to include only the additional text "; see also Note" in each of the 16 modified property definition tables. I believe the removal of the redundant paraphrase of the Note "related to the applicability of certain style properties to ruby container spans" does not reduce the utility of the reference to the Note since a reader can merely follow the link to determine that it is indeed "related to the applicability of certain style properties to ruby container spans".

Accordingly I will re-open this PR for further review by the WG. I also want to note for the record that @palemieux has not provided any substance to back up his claims that any interoperability problem remains with respect to these 16 properties. Nevertheless, I recognize the potential utility in drawing the reader's attention to the Note that explains why there is no interoperability problem.

@skynavga skynavga reopened this Jul 11, 2019
@skynavga skynavga assigned skynavga and unassigned palemieux Jul 11, 2019
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.

Resolved to merge as is during today's call

@css-meeting-bot
Copy link
Member

The Timed Text Working Group just discussed TTML1 #1101.

The full IRC log of that discussion <plh> Topic: TTML1 #1101
<plh> s/TTML1/TTML2/
<nigel> github: https://github.com//pull/1101
<plh> Nigel: proposal is to move ahead with the changes but with shortening the text in the note
<plh> Proposa: make the changes in #1101 with a shortert note reference
<plh> Resolved
<plh> s/Proposa/Proposal/
<plh> s/shortert/shorter/
<plh> Glenn: I'll need someone to approve the PR
<plh> Nigel: sure
<plh> ... was just holding until we have group consensus. will merge as-is.
<plh> ... will close relevant issues as well
<plh> Pierre: I guess we'll close #1100 and part of #1043
<plh> ... do we want to keep #1043 but narrow the scope to the style properties?
<plh> ... not all style properties have been taken care of
<plh> ... so we open a new issue or narrow #1043
<plh> Nigel: let's open a new one
<glenn> q+
<plh> ... will need some discussion with the CSS WG
<plh> ... make sure to double check #0143 before closing it and open a new one.
<plh> ack glenn
<plh> Glenn: this doesn't close #1043. also we should have 3 separate issues for the style properties
<plh> ... will be a lot easier
<plh> ... I would prefer to be the one to create them
<plh> Pierre: sure.
<plh> Pierre: we should close #1071
<plh> Nigel: ok
<plh> s/this doesn't close/ok to close/
<plh> Pierre: I'll wait for #1100 to be closed and the 3 news ones to be open before closing #1071
<plh> Glenn: let's reopn #0189
<plh> s/reopn/reopen/
<plh> s/0189/1089/
<plh> ... I'll merge 1101 and 1089 after the call
<plh> Pierre: for bidi, you might want to create one issue instead of 2
<plh> Glenn: sure.
<plh> Nigel: looking #1127

@skynavga skynavga removed the agenda label Jul 11, 2019
@skynavga skynavga merged commit c3e2165 into master Jul 11, 2019
@skynavga skynavga deleted the issue-1100-ruby-container-span-styles branch July 11, 2019 16:27
@skynavga skynavga removed their assignment Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emphasize non-applicability of styles to ignored text nodes in ruby container spans.
5 participants