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

Let position have an "auto" value and the same interpretation for LTR and RTL #69

Closed
wants to merge 61 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented May 13, 2014

No description provided.

@foolip
Copy link
Member Author

foolip commented May 13, 2014

To make the discussion less abstract, this is what I would be willing to implement. It is based on commit d393064 just before it would conflict. Merging it would require some discussion about how to resolve the conflicts, but one thing at a time.

@silviapfeiffer
Copy link
Member

I think that I can be convinced that we need "auto" values. I won't have time to review this properly until next Monday, sorry.

@foolip foolip changed the title Let position have an "auto" value and the same interpretation for LRT and RTL Let position have an "auto" value and the same interpretation for LTR and RTL May 15, 2014
@foolip
Copy link
Member Author

foolip commented May 21, 2014

Will you have time to look at this this week?

@silviapfeiffer
Copy link
Member

Very keen to. I have a customer deliverable on Monday, but am trying to deliver by the end of the week. If not, I'll take off Wednesday.

@silviapfeiffer
Copy link
Member

Can you rebase? I can't find half of these changes in the latest spec.

@foolip
Copy link
Member Author

foolip commented May 26, 2014

I based this on the first commit before your positioning rewrite because otherwise the diff becomes unreadable. Here's a branch with all the changes that would conflict: https://github.com/foolip/webvtt/commits/revert-position-size-align

Some of that (in particular lineAlign) would have to be re-reverted. I could prepare a branch with both of these branches merged to master in the final state I want the spec to be, but that's a lot of conflicts to resolve if you don't want to change the positioning model at all, so please first consider that question.

@foolip
Copy link
Member Author

foolip commented May 26, 2014

So are you OK with the actual model that this pull request ends up with, if done as a smaller diff on master?

@silviapfeiffer
Copy link
Member

I don't think our positioning models are that different. As I said above: I'd prefer two patches: one that adds the "auto" cue alignment and one that re-introduces the x-position calculations for "text track cue text position". They should be smaller than what your patches currently are.

@silviapfeiffer
Copy link
Member

In particular I do not like that you removed the changes about the "cue box". There is now independence between the box positioning and the text alignment inside the box and I want to retain that, since it makes a lot more sense than what was there beforehand.

@foolip
Copy link
Member Author

foolip commented May 27, 2014

Yes, I want to get rid of positionAlign, as per https://www.w3.org/Bugs/Public/show_bug.cgi?id=25632, because I still haven't heard of a good reason to have it at this time. If there are some future changes which would make it more than syntactic sugar that would be the time to introduce it.

@silviapfeiffer
Copy link
Member

You have to store the state of the box alignment somewhere, e.g. for a cue with position:20% align:middle . The "auto" value needs to be stored somewhere.

@foolip
Copy link
Member Author

foolip commented May 27, 2014

Currently only line has an "auto" value and I want position to be "auto" by default, but in this case it would be 20%. Which "auto" value is left to be stored somewhere?

@silviapfeiffer
Copy link
Member

Position would be 20%, text align middle, but how do you specify that the box is middle aligned over the 20% mark and not left aligned there?

@foolip
Copy link
Member Author

foolip commented May 28, 2014

align:middle means that the text is centered around 20%, there isn't anything more to specify, is there? If you want the text to be left aligned at 20%, just use "position:20% align:left".

I realize that you don't like that align influences the interpretation of position, but it's already been implemented and shipped. If decoupling them were really a better default we could investigate if that's a change we can make without breaking too much, but adding a fourth knob to the model doesn't make any sense to me.

@foolip
Copy link
Member Author

foolip commented Jun 3, 2014

I've looked closer at commit c84ec77, which resulted in nitpicks in #82, so I'm going to remove that from my revert branch to hopefully get closer to something that is merge-able.

foolip added 13 commits June 3, 2014 13:46
…es for the cue box (line and text position)."

This reverts commit 1fdced7.
This reverts commit 86ba250.

Conflicts:
	Overview.html
This reverts commit 8d5fe32.

Conflicts:
	Overview.html
…box as"

This reverts commit 797554b.

Conflicts:
	Overview.html
foolip added 22 commits June 4, 2014 00:11
Use the 'ltr' code paths in rendering, remove the 'rtl' paths,
then simplify the switches by merging similar conditions.
@foolip
Copy link
Member Author

foolip commented Jun 4, 2014

I've finished bringing the branches into sync with master and have prepared a copy of the spec that reflects the positioning model I actually want to implement: http://foolip.github.io/webvtt/

lineAlign was collateral damage here, if that's the only thing standing in the way of a merge (I suspect not) I can bring it back.

@foolip
Copy link
Member Author

foolip commented Jun 10, 2014

I'm closing this, I'll keep maintaining the branch and submit smaller patches until it's close enough.

@foolip foolip closed this Jun 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants