Bug 117008 – [GTK] add support for subtitles on webkitGTK

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Danilo de Paula]

There's no way to enable subtitles support on webkit2gtk.

Subtitle support is a feature already supported by webcore that can be enabled by WebPreferences->shouldDisplaySubtitles();

[Danilo de Paula]

Created attachment 203283 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Danilo de Paula]

Created attachment 203285 [details]
Patch

[Danilo de Paula]

Created attachment 203286 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 203286 [details]
Patch

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

In principle I'm OK with this setting, I can think of browsers letting users disable subtitles even if the site wants to show them, in particular in cases in which the built-in controls (which should have a cc button that would enable/disable subtitles AFAIK) are not used, but we have been taking an approach of only adding stuff that API users are actually using, do you have a use case for this setting already?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1134
> +

Missing *

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1136
> +     * Since: 2.3

Should be 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1143
> +            FALSE,

Any particular reason why we should not turn it on by default? I think working by default would be better.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2842
> +    bool currentValue = priv->preferences->shouldDisplaySubtitles();
> +    if (currentValue == enabled)

No reason to create a variable just for this check.

[Danilo de Paula]

The goal of this patch changed to "Adding support to <video><track> subtitles on webkit2gtk" since it's not only about enable/disable the methods.

[Danilo de Paula]

Created attachment 208724 [details]
Patch

[Martin Robinson]

Is it really useful to have subtitles disabled?

[Danilo de Paula]

Created attachment 208946 [details]
Patch

[EFL EWS Bot]

Comment on attachment 208946 [details]
Patch

Attachment 208946 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1471594

[EFL EWS Bot]

Comment on attachment 208946 [details]
Patch

Attachment 208946 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1430428

[Danilo de Paula]

(In reply to comment #11)
> (From update of attachment 208946 [details])
> Attachment 208946 [details] did not pass efl-ews (efl):
> Output: http://webkit-queues.appspot.com/results/1430428(In reply to comment #11)
> (From update of attachment 208946 [details])
> Attachment 208946 [details] did not pass efl-ews (efl):
> Output: http://webkit-queues.appspot.com/results/1430428


This build issue should be fixed by https://bugs.webkit.org/show_bug.cgi?id=119978

[Danilo de Paula]

*** Bug 117649 has been marked as a duplicate of this bug. ***

[Danilo de Paula]

Created attachment 209030 [details]
Patch

[EFL EWS Bot]

Comment on attachment 209030 [details]
Patch

Attachment 209030 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1511093

[EFL EWS Bot]

Comment on attachment 209030 [details]
Patch

Attachment 209030 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1478479

[Martin Robinson]

Comment on attachment 209030 [details]
Patch

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

> Source/WebCore/css/mediaControlsGtk.css:227
> +    background-image: url('data:image/svg+xml,<svg id="svg7384" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="16" width="16" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/"><metadata id="metadata90"><rdf:RDF><cc:Work rdf:about=""><dc:format>image/svg+xml</dc:format><dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/><dc:title>Gnome Symbolic Icon Theme</dc:title></cc:Work></rdf:RDF></metadata><g id="layer9" transform="translate(-101,-357)"><path id="path12148" style="block-progression:tb;color:#bebebe;direction:ltr;text-indent:0;text-align:start;enable-background:accumulate;text-transform:none;" fill="#bebebe" d="m104.75,357.06c-2.0602,0-3.75,1.6898-3.75,3.75v4.4375c0,2.0602,1.6898,3.75,3.75,3.75h4.9375l3.75,2.6562,1.5938,1.125v-1.9688l-0.0313-2.5c1.1106-0.59715,1.9688-1.6526,1.9688-3.0625v-4.4375c0-2.0602-1.6898-3.75-3.75-3.75h-8.4688zm0,2,8.4688,0c0.9868,0,1.75,0.7632,1.75,1.75v4.4375c0,0.86273-0.63508,1.541-1.125,1.625l-0.84,0.12v0.84375,1.0312l-2.4062-1.6875-0.25-0.1875h-0.3125-5.2812c-0.9868,0-1.75-0.7632-1.75-1.75v-4.4375c0-0.9868,0.7632-1.75,1.75-1.75z"/></g></svg>');

Are other ports putting the icon in the CSS itself?

> Source/WebCore/css/mediaControlsGtk.css:232
> +    position: absolute;

Absolute positioning?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1155
> +    WebKitSettings* settings = WEBKIT_SETTINGS(g_object_new(WEBKIT_TYPE_SETTINGS, NULL));
> +    settings->priv->preferences->setShouldDisplaySubtitles(TRUE);
> +    return settings;

It probably makes sense to do this in webKitSettingsConstructed instead.

[Danilo de Paula]

(In reply to comment #17)
> (From update of attachment 209030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209030&action=review
> 
> > Source/WebCore/css/mediaControlsGtk.css:227
> > +    background-image: url('data:image/svg+xml,<svg id="svg7384" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="16" width="16" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/"><metadata id="metadata90"><rdf:RDF><cc:Work rdf:about=""><dc:format>image/svg+xml</dc:format><dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/><dc:title>Gnome Symbolic Icon Theme</dc:title></cc:Work></rdf:RDF></metadata><g id="layer9" transform="translate(-101,-357)"><path id="path12148" style="block-progression:tb;color:#bebebe;direction:ltr;text-indent:0;text-align:start;enable-background:accumulate;text-transform:none;" fill="#bebebe" d="m104.75,357.06c-2.0602,0-3.75,1.6898-3.75,3.75v4.4375c0,2.0602,1.6898,3.75,3.75,3.75h4.9375l3.75,2.6562,1.5938,1.125v-1.9688l-0.0313-2.5c1.1106-0.59715,1.9688-1.6526,1.9688-3.0625v-4.4375c0-2.0602-1.6898-3.75-3.75-3.75h-8.4688zm0,2,8.4688,0c0.9868,0,1.75,0.7632,1.75,1.75v4.4375c0,0.86273-0.63508,1.541-1.125,1.625l-0.84,0.12v0.84375,1.0312l-2.4062-1.6875-0.25-0.1875h-0.3125-5.2812c-0.9868,0-1.75-0.7632-1.75-1.75v-4.4375c0-0.9868,0.7632-1.75,1.75-1.75z"/></g></svg>');
> 
> Are other ports putting the icon in the CSS itself?

As far as I know only apple is doing that.
I would love to use a Gtk themed image, but there's no gtk images related to subtitling. The SVG I'm using is a stripped version of a chat image inside Gtk code.

> 
> > Source/WebCore/css/mediaControlsGtk.css:232
> > +    position: absolute;
> 
> Absolute positioning?
Well, yes. We're using the same approach for the volume slider.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1155
> > +    WebKitSettings* settings = WEBKIT_SETTINGS(g_object_new(WEBKIT_TYPE_SETTINGS, NULL));
> > +    settings->priv->preferences->setShouldDisplaySubtitles(TRUE);
> > +    return settings;
> 
> It probably makes sense to do this in webKitSettingsConstructed instead.

I will push a new patch fixing that.

[Danilo de Paula]

Created attachment 209033 [details]
Patch

[Martin Robinson]

Comment on attachment 209033 [details]
Patch

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

Looks good, but please consider fixing a few nits before landing...

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:270
> +    if (m_closedCaptionsContainer) {

Maybe an early return here instead?

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:272
> +        if (m_closedCaptionsContainer->isShowing())
> +            hideClosedCaptionTrackList();

Ditto.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:291
> +    m_closedCaptionsContainer->show();
> +
> +    m_panel->setInlineStyleProperty(CSSPropertyPointerEvents, CSSValueNone);

You can probably remove this empty line.

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:301
> +    m_closedCaptionsContainer->hide();
> +
> +    m_panel->removeInlineStyleProperty(CSSPropertyPointerEvents);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:147
> +    prefs->setShouldDisplaySubtitles(TRUE);
>      ExperimentalFeatures features;

On the other hand, this probably deserves an empty newline. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1136
>              readWriteConstructParamFlags));
> -
>  }

I think this change is left over.

[Danilo de Paula]

Created attachment 209082 [details]
Patch

[Martin Robinson]

Comment on attachment 209082 [details]
Patch

Do we also need to update pixel results for tests that use the media player?

[Danilo de Paula]

(In reply to comment #22)
> (From update of attachment 209082 [details])
> Do we also need to update pixel results for tests that use the media player?

As far as I could see, the pixel tests do not include the captions/subtitles button. So the player shouldn't change.

However, might be a good idea to activate the tests on media/video-controls-captions-trackmenu.html instead of media/video-controls-captions.html

I can change that when I start with the missing bits so the new tests can pass.

[Danilo de Paula]

Comment on attachment 209082 [details]
Patch

asking for r+ and cq+

[Martin Robinson]

(In reply to comment #23)

> I can change that when I start with the missing bits so the new tests can pass.

What's still missing in the implementation?

[Danilo de Paula]

(In reply to comment #25)
> (In reply to comment #23)
> 
> > I can change that when I start with the missing bits so the new tests can pass.
> 
> What's still missing in the implementation?

Right now if you click on the CC button you need to select an option so the menu can close. Apple solved this by capturing all events on the whole document and closing the dialog when the user clicks anywhere.

Implementing this EventHandler is already on my task list for the next week. But since the subtitling support was totally disable on webkit2gtk, I imagined that it was ok to split that and push it separately.

[Danilo de Paula]

Created attachment 209487 [details]
Patch

[Martin Robinson]

Comment on attachment 209487 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlsGtk.cpp:46
> +        return listener->type() == GObjectEventListenerType
> +            ? static_cast<const MediaControlsGtkEventListener*>(listener)
> +            : 0;

This should really be one line.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:146
> +    prefs->setShouldDisplaySubtitles(TRUE);

TRUE -> true

> LayoutTests/platform/gtk/TestExpectations:793
> +webkit.org/b/101670 media/video-controls-captions-trackmenu.html [ Failure Pass ]
> +webkit.org/b/101670 media/video-controls-captions-trackmenu-sorted.html [ Pass ]
> +webkit.org/b/101670 media/video-controls-captions-trackmenu-localized.html [ Pass ]
> +webkit.org/b/101670 media/video-controls-captions-trackmenu-hide-on-click.html [ Pass ]
> +webkit.org/b/101670 media/video-controls-captions-trackmenu-hide-on-click-outside.html [ Pass ]

I believe that marking them as passing here is like marking them flaky. Are these flaky?

[Danilo de Paula]

Created attachment 209494 [details]
Patch

[Danilo de Paula]

Created attachment 209644 [details]
Patch

[WebKit Commit Bot]

Attachment 209644 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlElements.cpp', u'Source/WebCore/html/shadow/MediaControlElements.h', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/page/CaptionUserPreferences.cpp', u'Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp']" exit_code: 1
LayoutTests/platform/gtk/TestExpectations:1212:  expecting "[", "#", or end of line instead of "[Failure"  [test/expectations] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Created attachment 209647 [details]
Patch

[Danilo de Paula]

Created attachment 209902 [details]
Patch

[Danilo de Paula]

Comment on attachment 209902 [details]
Patch

the latest patch fix a small problem with the event handler blocking clicks on on the menu.

Since the feature wasn't accepted upstream yet, I sending it inside this patch.

[Gustavo Noronha (kov)]

Comment on attachment 209902 [details]
Patch

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

> Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:793
> -    return String::fromUTF8(C_("Subtitles", "Menu section heading for subtitles"));
> +    return String::fromUTF8(_("Subtitles"));

I think you should keep the context string, just put it in the proper place (first argument).

> Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:798
> -    return String::fromUTF8(C_("Off", "Menu item label for the track that represents disabling closed captions"));
> +    return String::fromUTF8(_("Off"));

ditto.

> Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:803
> -    return String::fromUTF8(C_("No label", "Menu item label for a closed captions track that has no other name"));
> +    return String::fromUTF8(_("No label"));

ditto.

[Danilo de Paula]

Created attachment 210159 [details]
Patch

[Danilo de Paula]

(In reply to comment #35)
> (From update of attachment 209902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209902&action=review
> 
> > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:793
> > -    return String::fromUTF8(C_("Subtitles", "Menu section heading for subtitles"));
> > +    return String::fromUTF8(_("Subtitles"));
> 
> I think you should keep the context string, just put it in the proper place (first argument).
> 
> > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:798
> > -    return String::fromUTF8(C_("Off", "Menu item label for the track that represents disabling closed captions"));
> > +    return String::fromUTF8(_("Off"));
> 
> ditto.
> 
> > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:803
> > -    return String::fromUTF8(C_("No label", "Menu item label for a closed captions track that has no other name"));
> > +    return String::fromUTF8(_("No label"));
> 
> ditto.


Thanks for the comments kov...
Fixed all those things...

[kov's GTK+ EWS bot]

Comment on attachment 210159 [details]
Patch

Attachment 210159 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/1627908

[kov's GTK+ EWS bot]

Comment on attachment 210159 [details]
Patch

Attachment 210159 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1624962

[Danilo de Paula]

Created attachment 210281 [details]
Patch

[Danilo de Paula]

(In reply to comment #40)
> Created an attachment (id=210281) [details]
> Patch

Changing document()-> to document().

[Danilo de Paula]

Created attachment 210348 [details]
Patch

[Danilo de Paula]

(In reply to comment #42)
> Created an attachment (id=210348) [details]
> Patch

Removed the shouldDisplaySubtitles part from WebSettings since it tries to force webkit2 to use subtitles.

[Danilo de Paula]

Created attachment 210396 [details]
Patch

[Danilo de Paula]

Comment on attachment 210396 [details]
Patch

This version should work on webkit1 and webkit2, "Off" and "Auto" buttons included.

[Gustavo Noronha (kov)]

Danilo and I were worried about the addition of the off and auto menu items to the core code, Eric says it seems fine to him, so I'm good with that =)

[WebKit Commit Bot]

Comment on attachment 210396 [details]
Patch

Clearing flags on attachment: 210396

Committed r155048: <http://trac.webkit.org/changeset/155048>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.