Bug 107214

Created attachment 186208 [details] Proposed fix 0.1

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.

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 .

(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?

(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.

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?

(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" ?

Works for me! Thanks!

Created attachment 186452 [details] Proposed fix 0.2

<rdar://problem/13147072>

Comment on attachment 186452 [details] Proposed fix 0.2 Clearing flags on attachment: 186452 Committed r141817: <http://trac.webkit.org/changeset/141817>

All reviewed patches have been landed. Closing bug.