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

Add requirements for region identifier to exist. #201

Closed
wants to merge 79 commits into from

Conversation

Projects
None yet
6 participants
@silviapfeiffer
Copy link
Member

commented Jun 28, 2015

@silviapfeiffer silviapfeiffer force-pushed the silviapfeiffer:bug22727 branch from 97dd7ca to cd3b42c Jun 28, 2015

@foolip

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

The spec bug is from before #31

To clarify the handling of region IDs, I would instead make the id->region mapping a parser-internal concern, which would remove the "WebVTT region identifier" concept entirely, and a text track would also not need to have a "text track list of regions" since that is no longer exposed in any way.

WDYT?

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2015

How can you make the id->region mapping an internal concern?

@foolip

This comment has been minimized.

Copy link
Member

commented Jul 22, 2015

VTTCue has an attribute VTTRegion? region and neither the VTTCue nor VTTRegion interfaces exposes the identifier. The parser is the only place that actually deals with the region identifiers, so it should be possible to maintain the mapping in the parser and then forget about it.

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2015

Hmm I think there's merely a mistake in the definition of VTTRegion: the line that reads

"Let region's WebVTT region identifier be the empty string."

should be removed, since VTTRegion doesn't actually expose its identifier. So, I think it already does what you suggest .

As for removing "WebVTT Region identifier" - if we do that, we have to add a optional region to the "WebVTT Cue" model so we can maintain the reference. It's one or the other in the model and I can live with how it is right now..

@foolip

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

Yep, I think that going all the way, removing the "WebVTT Region identifier" concept and making region identifiers a parser-internal concern is the way to go here. The cue model already has a region so I think we're almost there.

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2015

Don't we want to allow for regions to be specified via script and cues to be associated with regions via script?

@foolip

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

Yes, but you can do it simply by assigning cue.region = new VTTRegion() or similar, instead of having the indirection of an ID.

zcorpan and others added some commits Oct 8, 2015

@zcorpan

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

It seems to me it would be useful to be able to style different regions differently with CSS (feedback from Timed Text meeting at TPAC in Sapporo), which suggests the id should be exposed to Selectors at least, and I think in the API as well so you can create a region that can be identified from CSS.

Currently ::cue-region can't take an id.

Also see https://www.w3.org/Bugs/Public/show_bug.cgi?id=28473

@zcorpan

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

zcorpan and others added some commits Feb 17, 2016

Make position alignment saner
* Make align:start/end not affect position or position alignment.
* Rename start/end for position alignment to line-left/line-right.
* Clarify what align:left/right do for vertical text.
* Require 'position' if align:start/end and size != 100%.

Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28257.
Fix #234: Use a default background for <rt>
The ruby text is rendered outside the background box, so by default,
if the video is white or very bright, the ruby text would be unreadable.
Clarify line and snap-to-lines and how they can be invalid
Also be consistently use true or false for the snap-to-lines flag.

Fixes #289.
Editorial: Remove unused header syntax
Since 13e4a16 there are no defined
headers, so there being syntax defined for headers is confusing.

Fixes #304.
Change parsing of 'line' setting to accept non-integers
Non-integers can already enter the data model via the DOM API.
However, a valid WebVTT file can still only use integers.

Fixes #307.

@silviapfeiffer silviapfeiffer force-pushed the silviapfeiffer:bug22727 branch from cd3b42c to 074059b Oct 15, 2016

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

Hmm, just recreated the patch from this bug on the latest codebase - not sure why I get a list of all the changes since...

Just inspect 074059b

@zcorpan

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

I suspect you should use git rebase rather than git merge, but don't worry for this PR 🙂

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2016

Git and I ... Best friends, but sometimes... Sigh

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2017

Only relevant commit on this PR is 074059b

@silviapfeiffer

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2017

PR #349 replaces this, closing

@silviapfeiffer silviapfeiffer deleted the silviapfeiffer:bug22727 branch Jun 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.