WebVTT <i>, <b> and <u> elements should have default styles

• Component: Media
• Status: RESOLVED FIXED

[Dima Gorbik]

We shouldn't reuse HTMLElements but set css properties on span elements as specs require.

[Dima Gorbik]

Created attachment 186208 [details]
Proposed fix 0.1

[Elliott Sprehn]

Comment on attachment 186208 [details]
Proposed fix 0.1

View in context: https://bugs.webkit.org/attachment.cgi?id=186208&action=review

> Source/WebCore/css/mediaControls.css:270
> +video::-webkit-media-text-track-container ruby > rt {

Can we leave the ruby changes out of this patch since there's no tests? The test above doesn't have anything ruby related.

> Source/WebCore/rendering/RenderObject.cpp:167
> +    if (element->hasTagName(rubyTag) || element->hasTagName(WebVTTElement::rubyElementTagName())) {

Lets leave all the changes in this file out until you have some tests.

[Silvia Pfeiffer]

What makes you think that they should by <span>s ? The spec says otherwise: http://dev.w3.org/html5/webvtt/#webvtt-cue-text-dom-construction-rules .

[Dima Gorbik]

(In reply to comment #2)
> (From update of attachment 186208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186208&action=review
> 
> > Source/WebCore/css/mediaControls.css:270
> > +video::-webkit-media-text-track-container ruby > rt {
> 
> Can we leave the ruby changes out of this patch since there's no tests? The test above doesn't have anything ruby related.
> 
> > Source/WebCore/rendering/RenderObject.cpp:167
> > +    if (element->hasTagName(rubyTag) || element->hasTagName(WebVTTElement::rubyElementTagName())) {
> 
> Lets leave all the changes in this file out until you have some tests.

The problem here is that first of all we should implement display: ruby property, but Hixie said that they will rewrite all ruby specs but for now I still should place rt on top of rb. It works with this patch, though you can't see rt's text because it overflows the top bound of the cue box. I think the only proper way to test ruby/rt is to do a DRT of the cue. Would you suggest doing this? I have broken all elements from being rendered at all with my previous patch (on purpose) but those were not covered by test. That's why I decided to fix this regression though there will be no new tests (except for i, b and u). Do you think that's not a good idea?

[Dima Gorbik]

(In reply to comment #3)
> What makes you think that they should by <span>s ? The spec says otherwise: http://dev.w3.org/html5/webvtt/#webvtt-cue-text-dom-construction-rules .

I only used 'spans' in the bug description because specs say something like "These represent spans of underline text ". I don't use html spans of course,  only plain elements. I should probably change the bug description and the changelog so that it's not confusing.

[Silvia Pfeiffer]

Ah, right, in the meaning of "span of text". I agree that it's an unfortunate choice of word in the spec and confusing. Could you use "span of text" rather than just "span"? I might suggest replacing the word "span" with "snippet" or "passage" in the spec?

[Dima Gorbik]

(In reply to comment #6)
> Ah, right, in the meaning of "span of text". I agree that it's an unfortunate choice of word in the spec and confusing. Could you use "span of text" rather than just "span"? I might suggest replacing the word "span" with "snippet" or "passage" in the spec?

How about just "i, b, u WebVTT elements should have default styles" ?

[Silvia Pfeiffer]

Works for me! Thanks!

[Dima Gorbik]

Created attachment 186452 [details]
Proposed fix 0.2

[Radar WebKit Bug Importer]

<rdar://problem/13147072>

[WebKit Review Bot]

Comment on attachment 186452 [details]
Proposed fix 0.2

Clearing flags on attachment: 186452

Committed r141817: <http://trac.webkit.org/changeset/141817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.