Drop line: and position: settings with invalid linealign/colalign #337
Conversation
index.bs
Outdated
| substring of |value| up to and excluding the first U+002C COMMA character (,) in that string | ||
| and let |linealign| be the trailing substring of |value| starting from the character | ||
| immediately after the first U+002C COMMA character (,) in that string.</p></li> | ||
| <li><p>If |value| contains a U+002C COMMA character (,) that is not the last character in |
There was a problem hiding this comment.
What's the reason behind making this more complicated? The substring after the , when its at the end should be empty anyway, right?
There was a problem hiding this comment.
I wanted line:50, to be dropped. But maybe can set linealign to null instead of empty string, and check for that, instead.
There was a problem hiding this comment.
If we want the comma version to take priority over the non comma version, we should allow this as one where the parameters take the default value, I.e. linealign=start, seeing as if we make this extensible with further parameters, we will not always expect all the parameters to be present.
E.g. line:50,XYZ=blah should still take precedent over line:50 and result in linealign getting the default value.
There was a problem hiding this comment.
No. ^_^ This is ugly as it is, let's not consider making it worse. But even so, XYZ is invalid now and should still cause the whole thing to be dropped.
|
LGTM otherwise. Other than the step 1 changes, I've already changed the parser in wpt to match, pending changes to other tests. |
|
Fixed step 1, PTAL |
|
WFM |
Could you upload this as a PR to web-platform-tests, even if incomplete? |
|
Created here: web-platform-tests/wpt#5113 |
|
Bug for Gecko: |
Update parser for w3c/webvtt#337 Update parser to drop `line:1,` and update tests
zcorpan
force-pushed
the
zcorpan/drop-invalid-linealign
branch
2 times, most recently
from
d9a2a9f
to
f1ea632
Compare
Fixes #336.