Skip to content

Commit

Permalink
Update WebVTT region setting parser algorithm to match the spec
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=264736

Reviewed by Eric Carlson.

The current WebVTT parser incorrectly interprets the region id setting string `'id: foo'`
as a region with an id of `''`. However, this behavior deviates from the specification.
According to the spec, the parser should initially split the input string based on ascii spaces.
Then if the first `':'` in a setting string is either the first or last character of that string,
it should proceed to the next string. In the case of `'id: foo'`, it should be parsed as `'id:'` and `'foo'`,
both of which are considered invalid id strings.

This patch aligns the parser algorithm with the spec, which processes each setting string split by spaces.

* LayoutTests/imported/w3c/web-platform-tests/webvtt/parsing/file-parsing/tests/header-regions.html:
* LayoutTests/imported/w3c/web-platform-tests/webvtt/parsing/file-parsing/tests/regions-id-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/webvtt/parsing/file-parsing/tests/support/header-regions.vtt:
* Source/WebCore/html/track/VTTRegion.cpp:
(WebCore::VTTRegion::setRegionSettings):
(WebCore::VTTRegion::scanSettingName):

Canonical link: https://commits.webkit.org/270868@main
  • Loading branch information
cola119 authored and annevk committed Nov 17, 2023
1 parent 7258436 commit 30dd17f
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
var testTrack = document.createElement('track');
testTrack.onload = t.step_func_done(function() {
var track = testTrack.track;
assert_equals(track.cues.length, 9);
assert_equals(track.cues.length, 10);
checkCueRegions(track.cues);
});
testTrack.src = 'support/header-regions.vtt';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@

FAIL regions, id null is not an object (evaluating 'region3.lines')
PASS regions, id

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ REGION
invalid_settings values but region still created
: Invalid Header

REGION
id:region_split_by_ascii_whitespace width:10%
lines:5 regionanchor:40%,20% viewportanchor:30%,80% scroll:up

00:00:00.000 --> 00:00:02.500 region:someregionattributeid
"no region"

Expand All @@ -50,3 +54,6 @@ invalid_settings values but region still created

00:00:07.000 --> 00:00:08.000 region:
"no region"

00:00:08.000 --> 00:00:09.000 region:region_split_by_ascii_whitespace
{"width":10,"lines":5,"regionAnchorX":40,"regionAnchorY":20,"viewportAnchorX":30,"viewportAnchorY":80,"scroll":"up"}
35 changes: 18 additions & 17 deletions Source/WebCore/html/track/VTTRegion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,41 +143,42 @@ void VTTRegion::updateParametersFromRegion(const VTTRegion& other)

void VTTRegion::setRegionSettings(const String& inputString)
{
// https://w3c.github.io/webvtt/#region-settings-parsing
// 6.2. WebVTT region settings parsing
m_settings = inputString;
VTTScanner input(inputString);

while (!input.isAtEnd()) {
input.skipWhile<isTabOrSpace>();
if (input.isAtEnd())
break;
// Step 1 - Split input on spaces.
input.skipWhile<isASCIIWhitespace<UChar>>();
VTTScanner::Run valueRun = input.collectUntil<isASCIIWhitespace<UChar>>();
VTTScanner setting(input.extractString(valueRun));

// Scan the name part.
RegionSetting name = scanSettingName(input);
// Step 2.2 - Let name be the leading substring of setting up to and excluding the first U+003A COLON character (:) in that string.
RegionSetting name = scanSettingName(setting);

// Verify that we're looking at a ':'.
if (name == None || !input.scan(':')) {
input.skipUntil<isASCIIWhitespace<UChar>>();
// Step 2.1 - If the first ':' in setting is the last character of setting, then jump to the next setting.
if (name == None || setting.isAtEnd())
continue;
}

// Scan the value part.
parseSettingValue(name, input);
// Steps 2.3-2.4 - Scan the value part.
parseSettingValue(name, setting);
}
}

VTTRegion::RegionSetting VTTRegion::scanSettingName(VTTScanner& input)
{
if (input.scan("id"))
if (input.scan("id:"))
return Id;
if (input.scan("lines"))
if (input.scan("lines:"))
return Lines;
if (input.scan("width"))
if (input.scan("width:"))
return Width;
if (input.scan("viewportanchor"))
if (input.scan("viewportanchor:"))
return ViewportAnchor;
if (input.scan("regionanchor"))
if (input.scan("regionanchor:"))
return RegionAnchor;
if (input.scan("scroll"))
if (input.scan("scroll:"))
return Scroll;

return None;
Expand Down

0 comments on commit 30dd17f

Please sign in to comment.