Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
921833003:
Expose APIs for text track settings (Closed)
|
Created:
5 years, 10 months ago by srivats Modified:
5 years, 8 months ago CC:
blink-reviews, nessy, arv+blink, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-html_chromium.org, Inactive, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionExpose APIs for text track settings
This commit adds APIs to expose the following CSS properties of the VTT track
element:
Text color, background color, text shadow, font size, font family, font style
and font variant.
These CSS properties can be used to customize text tracks according to the
user's accessibility needs.
The user-specified caption settings when set override author-specified settings and the app-default settings.
Chromium side CL: https://codereview.chromium.org/1002473003
BUG=457850
TEST=media/track/track-css-matching-user-override-author-settings.html,
media/track/track-css-user-settings-override-internal-settings.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192627
Patch Set 1 #
Total comments: 34
Patch Set 2 : Addressed comments from fs #
Total comments: 8
Patch Set 3 : Addressed comments from fs and added a new test #
Total comments: 5
Patch Set 4 : Addressed comments in VTTCue #Patch Set 5 : Removed window color API as per Philip's suggestion #
Total comments: 1
Patch Set 6 : Rebase #
Total comments: 11
Patch Set 7 : Addressed Philip's comments #Patch Set 8 : Override author-specified caption settings with user-specified caption settings #
Total comments: 9
Patch Set 9 : Addressed Philip's comments #
Total comments: 3
Patch Set 10 : Rebase and added a test to verify behavior of internal settings #
Total comments: 14
Patch Set 11 : Addressed nits #Messages
Total messages: 47 (7 generated)
srivats@amazon.com changed reviewers: + philipj@opera.com
Hey Philip, This change adds APIs to set the CSS properties of the text track element in order to support system-wide preferences for Closed Captioning on Android. Here's a correlation of the preferences available in Android 4.4 platform caption settings and the APIs I have added. 1. Text size <--> textTrackTextSize 2. Font family <--> textTrackFontFamily, textTrackFontStyle, textTrackFontVariant 3. Text color and opacity <--> textTrackTextColor 4. Edge type <--> textTrackTextShadow 5. Background color and opacity <--> textTrackBackgroundColor 6. Caption window color and opacity <--> textTrackWindowColor Please take a look. Thanks!
fs@opera.com changed reviewers: + fs@opera.com
On 2015/02/13 00:36:50, srivats wrote: > Hey Philip, > This change adds APIs to set the CSS properties of the text track element in > order to support system-wide preferences for Closed Captioning on Android. > Here's a correlation of the preferences available in Android 4.4 platform > caption settings and the APIs I have added. > 1. Text size <--> textTrackTextSize > 2. Font family <--> textTrackFontFamily, textTrackFontStyle, > textTrackFontVariant > 3. Text color and opacity <--> textTrackTextColor > 4. Edge type <--> textTrackTextShadow > 5. Background color and opacity <--> textTrackBackgroundColor > 6. Caption window color and opacity <--> textTrackWindowColor > > Please take a look. Thanks! Philip is OOO today, but I did an initial review... Above you say "system-wide preferences for Closed Captioning", but this seems to make no difference between 'captions' and 'subtitles' - is that intentional? If it is then I suggest you mention/document that somewhere. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... File LayoutTests/media/track/track-css-matching-settings.html (right): https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:1: <!DOCTYPE html> Maybe get "user" and "override" in the filename (and the comment below) to make it more clear what settings this is about. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:2: <html> I realize you probably just copied the structure from some other TC, but I'm going to be bold and suggest to drop <html>, <head>, <meta> and <body> (use window.onload in the script instead) - and hope philipj agrees =) https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:11: internals.settings.setTextTrackTextColor("cyan"); I think you should also test this with a stylesheet that styles the same thing. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:21: function cleanup() I believe the test framework does this for you. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:37: if (testEnded) This shouldn't be needed. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:40: testExpected("getComputedStyle(textTrackDisplayElement(video, 'cue')).color", "rgb(0, 255, 255)"); This would be more readable as: cue = textTrackDisplayElement(video, 'cue'); testExpected("getComputedStyle(cue).color", "rgb(0, 255, 255)"); ... (the 'display' one would obviously need to be separate still.) https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:57: waitForEvent('seeked', seeked); The referenced VTT file has a cue active at 0s, so why seek at all? https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:64: <video controls > No need for 'controls'? https://codereview.chromium.org/921833003/diff/1/Source/core/frame/Settings.in File Source/core/frame/Settings.in (right): https://codereview.chromium.org/921833003/diff/1/Source/core/frame/Settings.i... Source/core/frame/Settings.in:320: # Closed captions settings "settings" -> "user style overrides"? Also, sort alphabetically within the block? https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:210: if (m_cue->element()->ownerDocument()->page()) { Use document() rather than ownerDocument(). Also use Document::settings() rather than going through page(). https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:212: m_cue->element()->setInlineStyleProperty(CSSPropertyColor, settings.textTrackTextColor(), true); I think it would make more sense to handle the styles that apply to the background box in VTTCue. Also, I'm assuming that "" (the empty string) is supposed to mean "no override"? While this is what the code does currently, it also (inadvertently?) depends on the behavior of "" when passed to setProperty - i.e. it ends up being a removeProperty. Seems better to make the intention of this code explicit by only calling setInline... if the relevant String is non-empty. https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:219: // Set background color for text track display "text track display" -> "the cue box."; and maybe "Set" -> "Override". https://codereview.chromium.org/921833003/diff/1/Source/core/testing/Internal... File Source/core/testing/InternalSettings.idl (right): https://codereview.chromium.org/921833003/diff/1/Source/core/testing/Internal... Source/core/testing/InternalSettings.idl:54: [RaisesException] void setTextTrackTextColor(DOMString color); I think the auto-generated versions should be fine for your uses - are they not? https://codereview.chromium.org/921833003/diff/1/Source/web/WebSettingsImpl.h File Source/web/WebSettingsImpl.h (right): https://codereview.chromium.org/921833003/diff/1/Source/web/WebSettingsImpl.h... Source/web/WebSettingsImpl.h:183: virtual void setTextTrackTextColor(const WebString&); Order alphabetically. https://codereview.chromium.org/921833003/diff/1/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/921833003/diff/1/public/web/WebSettings.h#new... public/web/WebSettings.h:254: virtual void setTextTrackTextColor(const WebString&) = 0; Ditto.
Patchset #2 (id:20001) has been deleted
Addressed feedback from fs.
https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:212: m_cue->element()->setInlineStyleProperty(CSSPropertyColor, settings.textTrackTextColor(), true); Yes the "" means no override and we would use it when the user selects "Use defaults". Would you recommend using removeInlineStyleProperty when empty strings are passed in? On 2015/02/13 13:13:15, fs wrote: > I think it would make more sense to handle the styles that apply to the > background box in VTTCue. > > Also, I'm assuming that "" (the empty string) is supposed to mean "no override"? > While this is what the code does currently, it also (inadvertently?) depends on > the behavior of "" when passed to setProperty - i.e. it ends up being a > removeProperty. Seems better to make the intention of this code explicit by only > calling setInline... if the relevant String is non-empty.
Is there a Chromium-side CL which would expose these settings to users? If not, I'll ask what the policy on settings that aren't used by Chromium is, I imagine it's discouraged in general.
The TEST=... lines now needs to be updated to reflect the new name of the test. https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:212: m_cue->element()->setInlineStyleProperty(CSSPropertyColor, settings.textTrackTextColor(), true); On 2015/02/16 22:50:14, srivats wrote: > > Yes the "" means no override and we would use it when the user selects "Use > defaults". Would you recommend using removeInlineStyleProperty when empty > strings are passed in? What I meant was that passing "" to this particular method (setInlineStyleProperty) will cause any existing property to be removed. This shouldn't be a problem here (since nothing else should set inline style on the element in question), but it should either be noted that "" is never intended to trigger that behavior - or explicitly check for "" and don't call setInlineStyleProperty in that case (to avoid unintended side-effects.) > On 2015/02/13 13:13:15, fs wrote: > > I think it would make more sense to handle the styles that apply to the > > background box in VTTCue. This has not been addressed. https://codereview.chromium.org/921833003/diff/1/Source/web/WebSettingsImpl.h File Source/web/WebSettingsImpl.h (right): https://codereview.chromium.org/921833003/diff/1/Source/web/WebSettingsImpl.h... Source/web/WebSettingsImpl.h:183: virtual void setTextTrackTextColor(const WebString&); On 2015/02/13 13:13:16, fs wrote: > Order alphabetically. This has not been addressed. (The new methods are now ordered, but not wrt all the other methods just above them.) https://codereview.chromium.org/921833003/diff/1/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/921833003/diff/1/public/web/WebSettings.h#new... public/web/WebSettings.h:254: virtual void setTextTrackTextColor(const WebString&) = 0; On 2015/02/13 13:13:16, fs wrote: > Ditto. See issue in WebSettingsImpl.h https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... File LayoutTests/media/track/track-css-matching-css-override-settings.html (right): https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... LayoutTests/media/track/track-css-matching-css-override-settings.html:2: <video> I'd still put this last rather than first. https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... LayoutTests/media/track/track-css-matching-css-override-settings.html:10: video::cue { What I meant for this test was really to have this _and_ to set settings, and hence check that the settings override this style. https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... File LayoutTests/media/track/track-css-matching-user-override-settings.html (right): https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... LayoutTests/media/track/track-css-matching-user-override-settings.html:2: <video> See issue in other test. https://codereview.chromium.org/921833003/diff/40001/Source/core/testing/Inte... File Source/core/testing/InternalSettings.h (right): https://codereview.chromium.org/921833003/diff/40001/Source/core/testing/Inte... Source/core/testing/InternalSettings.h:136: void setTextTrackBackgroundColor(const String&, ExceptionState&); These should exist on InternalSettingsGenerated already (i.e. they are autogenerated from Settings.in)
On 2015/02/17 02:51:00, philipj_UTC7 wrote: > Is there a Chromium-side CL which would expose these settings to users? If not, > I'll ask what the policy on settings that aren't used by Chromium is, I imagine > it's discouraged in general. Working on a chromium side CL. Waiting for this to settle a bit before uploading the chromium side changes.
Addressed comments from fs https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... File LayoutTests/media/track/track-css-matching-settings.html (right): https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:1: <!DOCTYPE html> On 2015/02/13 13:13:15, fs wrote: > Maybe get "user" and "override" in the filename (and the comment below) to make > it more clear what settings this is about. Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:2: <html> On 2015/02/13 13:13:15, fs wrote: > I realize you probably just copied the structure from some other TC, but I'm > going to be bold and suggest to drop <html>, <head>, <meta> and <body> (use > window.onload in the script instead) - and hope philipj agrees =) Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:11: internals.settings.setTextTrackTextColor("cyan"); On 2015/02/13 13:13:15, fs wrote: > I think you should also test this with a stylesheet that styles the same thing. Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:21: function cleanup() On 2015/02/13 13:13:15, fs wrote: > I believe the test framework does this for you. Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:37: if (testEnded) On 2015/02/13 13:13:15, fs wrote: > This shouldn't be needed. Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:40: testExpected("getComputedStyle(textTrackDisplayElement(video, 'cue')).color", "rgb(0, 255, 255)"); On 2015/02/13 13:13:14, fs wrote: > This would be more readable as: > > cue = textTrackDisplayElement(video, 'cue'); > testExpected("getComputedStyle(cue).color", "rgb(0, 255, 255)"); > ... > > (the 'display' one would obviously need to be separate still.) Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:57: waitForEvent('seeked', seeked); On 2015/02/13 13:13:15, fs wrote: > The referenced VTT file has a cue active at 0s, so why seek at all? Done. https://codereview.chromium.org/921833003/diff/1/LayoutTests/media/track/trac... LayoutTests/media/track/track-css-matching-settings.html:64: <video controls > On 2015/02/13 13:13:15, fs wrote: > No need for 'controls'? Done. https://codereview.chromium.org/921833003/diff/1/Source/core/frame/Settings.in File Source/core/frame/Settings.in (right): https://codereview.chromium.org/921833003/diff/1/Source/core/frame/Settings.i... Source/core/frame/Settings.in:320: # Closed captions settings On 2015/02/13 13:13:15, fs wrote: > "settings" -> "user style overrides"? > > Also, sort alphabetically within the block? Done. https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:210: if (m_cue->element()->ownerDocument()->page()) { On 2015/02/13 13:13:16, fs wrote: > Use document() rather than ownerDocument(). Also use Document::settings() rather > than going through page(). Done. https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:212: m_cue->element()->setInlineStyleProperty(CSSPropertyColor, settings.textTrackTextColor(), true); On 2015/02/17 09:43:46, fs wrote: > On 2015/02/16 22:50:14, srivats wrote: > > > > Yes the "" means no override and we would use it when the user selects "Use > > defaults". Would you recommend using removeInlineStyleProperty when empty > > strings are passed in? > > What I meant was that passing "" to this particular method > (setInlineStyleProperty) will cause any existing property to be removed. This > shouldn't be a problem here (since nothing else should set inline style on the > element in question), but it should either be noted that "" is never intended to > trigger that behavior - or explicitly check for "" and don't call > setInlineStyleProperty in that case (to avoid unintended side-effects.) > > > On 2015/02/13 13:13:15, fs wrote: > > > I think it would make more sense to handle the styles that apply to the > > > background box in VTTCue. > > This has not been addressed. Done. https://codereview.chromium.org/921833003/diff/1/Source/core/html/track/vtt/V... Source/core/html/track/vtt/VTTCue.cpp:219: // Set background color for text track display On 2015/02/13 13:13:15, fs wrote: > "text track display" -> "the cue box."; and maybe "Set" -> "Override". Done. https://codereview.chromium.org/921833003/diff/1/Source/core/testing/Internal... File Source/core/testing/InternalSettings.idl (right): https://codereview.chromium.org/921833003/diff/1/Source/core/testing/Internal... Source/core/testing/InternalSettings.idl:54: [RaisesException] void setTextTrackTextColor(DOMString color); On 2015/02/13 13:13:16, fs wrote: > I think the auto-generated versions should be fine for your uses - are they not? Done. https://codereview.chromium.org/921833003/diff/1/Source/web/WebSettingsImpl.h File Source/web/WebSettingsImpl.h (right): https://codereview.chromium.org/921833003/diff/1/Source/web/WebSettingsImpl.h... Source/web/WebSettingsImpl.h:183: virtual void setTextTrackTextColor(const WebString&); On 2015/02/17 09:43:46, fs wrote: > On 2015/02/13 13:13:16, fs wrote: > > Order alphabetically. > > This has not been addressed. (The new methods are now ordered, but not wrt all > the other methods just above them.) Done. https://codereview.chromium.org/921833003/diff/1/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/921833003/diff/1/public/web/WebSettings.h#new... public/web/WebSettings.h:254: virtual void setTextTrackTextColor(const WebString&) = 0; On 2015/02/17 09:43:46, fs wrote: > On 2015/02/13 13:13:16, fs wrote: > > Ditto. > > See issue in WebSettingsImpl.h Done. https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... File LayoutTests/media/track/track-css-matching-css-override-settings.html (right): https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... LayoutTests/media/track/track-css-matching-css-override-settings.html:2: <video> On 2015/02/17 09:43:46, fs wrote: > I'd still put this last rather than first. Done. https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... LayoutTests/media/track/track-css-matching-css-override-settings.html:10: video::cue { On 2015/02/17 09:43:46, fs wrote: > What I meant for this test was really to have this _and_ to set settings, and > hence check that the settings override this style. Done. https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... File LayoutTests/media/track/track-css-matching-user-override-settings.html (right): https://codereview.chromium.org/921833003/diff/40001/LayoutTests/media/track/... LayoutTests/media/track/track-css-matching-user-override-settings.html:2: <video> On 2015/02/17 09:43:47, fs wrote: > See issue in other test. Done. https://codereview.chromium.org/921833003/diff/40001/Source/core/testing/Inte... File Source/core/testing/InternalSettings.h (right): https://codereview.chromium.org/921833003/diff/40001/Source/core/testing/Inte... Source/core/testing/InternalSettings.h:136: void setTextTrackBackgroundColor(const String&, ExceptionState&); On 2015/02/17 09:43:47, fs wrote: > These should exist on InternalSettingsGenerated already (i.e. they are > autogenerated from Settings.in) My bad! I was under the impression that this was required for the layout test.
Please let us know where there's a Chromium-side CL. https://codereview.chromium.org/921833003/diff/60001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/921833003/diff/60001/public/web/WebSettings.h... public/web/WebSettings.h:239: virtual void setTextTrackWindowColor(const WebString&) = 0; I think this bit should wait until it's possible to style it with CSS per spec. Please file a bug via http://dev.w3.org/html5/webvtt/ if you want it. It's very unlikely that the per-spec concept would be called anything with "window".
VTTCue changes lg now. Two minor nits. https://codereview.chromium.org/921833003/diff/60001/Source/core/html/track/v... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/60001/Source/core/html/track/v... Source/core/html/track/vtt/VTTCue.cpp:144: if (value != "") { Nit: !value.isEmpty() https://codereview.chromium.org/921833003/diff/60001/Source/core/html/track/v... Source/core/html/track/vtt/VTTCue.cpp:146: } Nit: Drop {} (not needed for single statements)
On 2015/02/18 03:33:14, philipj_UTC7 wrote: > Please let us know where there's a Chromium-side CL. > > https://codereview.chromium.org/921833003/diff/60001/public/web/WebSettings.h > File public/web/WebSettings.h (right): > > https://codereview.chromium.org/921833003/diff/60001/public/web/WebSettings.h... > public/web/WebSettings.h:239: virtual void setTextTrackWindowColor(const > WebString&) = 0; > I think this bit should wait until it's possible to style it with CSS per spec. > Please file a bug via http://dev.w3.org/html5/webvtt/ if you want it. It's very > unlikely that the per-spec concept would be called anything with "window". Thanks for the suggestion Philip. I will remove the textTrackWindowColor API from this CL and make sure to follow up on creating a bug to get this added in the spec. We won't have full support for the Android closed captions accessibility settings until the caption window color API lands. Is this acceptable?
Addressed comments in VTTCue.cpp. https://codereview.chromium.org/921833003/diff/60001/Source/core/html/track/v... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/60001/Source/core/html/track/v... Source/core/html/track/vtt/VTTCue.cpp:144: if (value != "") { On 2015/02/18 09:36:46, fs wrote: > Nit: !value.isEmpty() Done. https://codereview.chromium.org/921833003/diff/60001/Source/core/html/track/v... Source/core/html/track/vtt/VTTCue.cpp:146: } On 2015/02/18 09:36:46, fs wrote: > Nit: Drop {} (not needed for single statements) Done.
On 2015/02/18 03:33:14, philipj_UTC7 wrote: > Please let us know where there's a Chromium-side CL. > > https://codereview.chromium.org/921833003/diff/60001/public/web/WebSettings.h > File public/web/WebSettings.h (right): > > https://codereview.chromium.org/921833003/diff/60001/public/web/WebSettings.h... > public/web/WebSettings.h:239: virtual void setTextTrackWindowColor(const > WebString&) = 0; > I think this bit should wait until it's possible to style it with CSS per spec. > Please file a bug via http://dev.w3.org/html5/webvtt/ if you want it. It's very > unlikely that the per-spec concept would be called anything with "window". Hey Philip, I added a link to the Chromium side CL in the description. PTAL Thanks, Sunitha
Ah, so there's an Android API which gives the defaults of these values. Makes sense.
Hi Philip, The Chromium-side CL has received two lgtms. Can you take another look at this? Thanks, Sunitha
Great, let's figure this out!
My biggest concern is how this will interact with author style. If we have ::cue
{ background-color white; color: black; } in the page's style and only the color
is overridden using this API, the results will not be good.
Is it expected that only one of the setting can be set, or must all of them be
set? Documenting expectations in Settings.in and asserting them somewhere would
be helpful.
The size setting seems a bit odd to me. Does font scale 1 correspond to a
concrete size on Android? Only scaling the font size won't get the user a
consistent look, if that's the intention.
Thanks for working on this!
https://codereview.chromium.org/921833003/diff/120001/Source/core/frame/Setti...
File Source/core/frame/Settings.in (right):
https://codereview.chromium.org/921833003/diff/120001/Source/core/frame/Setti...
Source/core/frame/Settings.in:336: textTrackTextSize type=String
What is the expected format of this string? In the Chromium CL it's a
percentage, if that's the only intended usage maybe this should be a type=double
initial=1.0 with some asserts?
https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.cpp (right):
https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:141: static void
setInlineStylePropertyIfNotEmpty(PassRefPtrWillBeRawPtr<HTMLDivElement> element,
Just Element& as the argument type is OK.
https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:1145: Settings* settings =
m_cueBackgroundBox->document().settings();
You can use just document(), see VTTCue::document(),
https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.cpp:1146: if (settings) {
if (!settings) return;
https://codereview.chromium.org/921833003/diff/120001/Source/web/WebSettingsI...
File Source/web/WebSettingsImpl.cpp (right):
https://codereview.chromium.org/921833003/diff/120001/Source/web/WebSettingsI...
Source/web/WebSettingsImpl.cpp:390:
m_settings->setTextTrackBackgroundColor((String)color);
Is the (String) case really needed? setDefaultVideoPosterURL doesn't have it.
https://codereview.chromium.org/921833003/diff/120001/Source/core/frame/Setti... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/921833003/diff/120001/Source/core/frame/Setti... Source/core/frame/Settings.in:336: textTrackTextSize type=String On 2015/03/17 02:22:31, philipj_UTC7 wrote: > What is the expected format of this string? In the Chromium CL it's a > percentage, if that's the only intended usage maybe this should be a type=double > initial=1.0 with some asserts? Font size css property can have multiple formats like percentage, small/medium/large, length(px) etc. On Android, we get font-size from FontScale where 1 corresponds to 100% and medium. The Chromium-side CL converts the FontScale into a percentage but it could also be represented as small/medium/large. Leaving this as String gives the flexibility to set it in different formats. https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:141: static void setInlineStylePropertyIfNotEmpty(PassRefPtrWillBeRawPtr<HTMLDivElement> element, On 2015/03/17 02:22:31, philipj_UTC7 wrote: > Just Element& as the argument type is OK. Done. https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:1145: Settings* settings = m_cueBackgroundBox->document().settings(); On 2015/03/17 02:22:31, philipj_UTC7 wrote: > You can use just document(), see VTTCue::document(), Done. https://codereview.chromium.org/921833003/diff/120001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:1146: if (settings) { On 2015/03/17 02:22:31, philipj_UTC7 wrote: > if (!settings) return; Done. https://codereview.chromium.org/921833003/diff/120001/Source/web/WebSettingsI... File Source/web/WebSettingsImpl.cpp (right): https://codereview.chromium.org/921833003/diff/120001/Source/web/WebSettingsI... Source/web/WebSettingsImpl.cpp:390: m_settings->setTextTrackBackgroundColor((String)color); On 2015/03/17 02:22:31, philipj_UTC7 wrote: > Is the (String) case really needed? setDefaultVideoPosterURL doesn't have it. It turns out the (String) case wasn't required. I followed the example of setDefaultTextEncodingName initially.
On 2015/03/17 02:22:32, philipj_UTC7 wrote:
> Great, let's figure this out!
>
> My biggest concern is how this will interact with author style. If we have
::cue
> { background-color white; color: black; } in the page's style and only the
color
> is overridden using this API, the results will not be good.
>
> Is it expected that only one of the setting can be set, or must all of them be
> set? Documenting expectations in Settings.in and asserting them somewhere
would
> be helpful.
>
The user gets a preview of the caption styles they select on Android and Mac.
Here's what the preview looks like on Android:
https://developer.android.com/about/versions/kitkat.html#44-accessibility
The only way the user can pick background color and color separately is when
they select Custom Style in caption settings.
To handle the case you mentioned, I want to add a check to
VTTCue::applyUserOverrideCSSProperties to determine if author-specified
background-color and user-specified text color are the same and set the
user-style only if they're different(this might confuse the user though). To
read author-specified cue css properties, I tried using element->getAttribute
and element->style()->getPropertyValue with no luck. Do you have a
recommendation on how I can achieve this?
Also, I checked to see if iOS handles this case and turns out they don't. I used
a test page with cue color set to red and specified platform settings
background-color to be red and text color to use "default". This ended up
showing red on red on Chrome and Safari on an iPhone.
> The size setting seems a bit odd to me. Does font scale 1 correspond to a
> concrete size on Android? Only scaling the font size won't get the user a
> consistent look, if that's the intention.
I didn't find any Android specific documentation for font scale mapping to a
concrete size in pixels or points. Font scale of 1 corresponds to 100%, medium
and 1em in other font-size units.
https://codereview.chromium.org/921833003/diff/100001/Source/core/html/track/... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/100001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:1152: if (settings) { "if (!settings) return" to reduce the indentation is a common style in Blink. https://codereview.chromium.org/921833003/diff/120001/Source/core/frame/Setti... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/921833003/diff/120001/Source/core/frame/Setti... Source/core/frame/Settings.in:336: textTrackTextSize type=String On 2015/03/18 01:58:13, srivats wrote: > On 2015/03/17 02:22:31, philipj_UTC7 wrote: > > What is the expected format of this string? In the Chromium CL it's a > > percentage, if that's the only intended usage maybe this should be a > type=double > > initial=1.0 with some asserts? > > Font size css property can have multiple formats like percentage, > small/medium/large, length(px) etc. On Android, we get font-size from FontScale > where 1 corresponds to 100% and medium. The Chromium-side CL converts the > FontScale into a percentage but it could also be represented as > small/medium/large. Leaving this as String gives the flexibility to set it in > different formats. OK, so the actual data type of the setting is secondary. The important thing is what this actually should do. On Android, is the system-level style the only involved, or how does the size setting interact with formats that have internal size settings? It seems to me that if a user sets font scale 1.5 and that always results in captions being rendered at (say) 30px, then that should map to forcing the font size to be 30px for WebVTT. However, this would require distinguishing font scale 1 from a "untouched default" font scale, but does the Android API differentiate between these?
On 2015/03/18 02:02:35, srivats wrote:
> On 2015/03/17 02:22:32, philipj_UTC7 wrote:
> > Great, let's figure this out!
> >
> > My biggest concern is how this will interact with author style. If we have
> ::cue
> > { background-color white; color: black; } in the page's style and only the
> color
> > is overridden using this API, the results will not be good.
> >
> > Is it expected that only one of the setting can be set, or must all of them
be
> > set? Documenting expectations in Settings.in and asserting them somewhere
> would
> > be helpful.
> >
>
> The user gets a preview of the caption styles they select on Android and Mac.
> Here's what the preview looks like on Android:
> https://developer.android.com/about/versions/kitkat.html#44-accessibility
> The only way the user can pick background color and color separately is when
> they select Custom Style in caption settings.
Ah, this is great, I found this menu on my phone and played with it. It looks to
me like the user expectation would be that if they turn captions on, the size
they pick will be the size that they see, nothing else.
Do you know if it's possible to detect if the top-level captions state was set
to on or off? Maybe the Chromium-side CL already avoid setting anything if it's
off?
> To handle the case you mentioned, I want to add a check to
> VTTCue::applyUserOverrideCSSProperties to determine if author-specified
> background-color and user-specified text color are the same and set the
> user-style only if they're different(this might confuse the user though). To
> read author-specified cue css properties, I tried using element->getAttribute
> and element->style()->getPropertyValue with no luck. Do you have a
> recommendation on how I can achieve this?
You'd probably want to look at the computed style of the cue, but the cue isn't
added to the DOM before it needs to be rendered, so I think doing any logic with
the computed style would imply a two-step layout, which would be unfortunate.
> Also, I checked to see if iOS handles this case and turns out they don't. I
used
> a test page with cue color set to red and specified platform settings
> background-color to be red and text color to use "default". This ended up
> showing red on red on Chrome and Safari on an iPhone.
Thanks for testing! The behavior sounds unfortunate, but if it turns out very
hard to avoid perhaps we shouldn't be too distressed.
> > The size setting seems a bit odd to me. Does font scale 1 correspond to a
> > concrete size on Android? Only scaling the font size won't get the user a
> > consistent look, if that's the intention.
>
> I didn't find any Android specific documentation for font scale mapping to a
> concrete size in pixels or points. Font scale of 1 corresponds to 100%, medium
> and 1em in other font-size units.
I feel I don't have the necessary knowledge of how the Android settings are
intended to work. I'm going to ask Laura Palmer on Google's accessibility team
if she can find someone with knowledge of these things.
If these settings do nothing when the captions are off in Android's
accessibility menu I think it would be OK to land this and iterate from there,
though.
On 2015/03/18 06:42:24, philipj_UTC7 wrote:
> On 2015/03/18 02:02:35, srivats wrote:
> > On 2015/03/17 02:22:32, philipj_UTC7 wrote:
> > > Great, let's figure this out!
> > >
> > > My biggest concern is how this will interact with author style. If we have
> > ::cue
> > > { background-color white; color: black; } in the page's style and only the
> > color
> > > is overridden using this API, the results will not be good.
> > >
> > > Is it expected that only one of the setting can be set, or must all of
them
> be
> > > set? Documenting expectations in Settings.in and asserting them somewhere
> > would
> > > be helpful.
> > >
> >
> > The user gets a preview of the caption styles they select on Android and
Mac.
> > Here's what the preview looks like on Android:
> > https://developer.android.com/about/versions/kitkat.html#44-accessibility
> > The only way the user can pick background color and color separately is when
> > they select Custom Style in caption settings.
>
> Ah, this is great, I found this menu on my phone and played with it. It looks
to
> me like the user expectation would be that if they turn captions on, the size
> they pick will be the size that they see, nothing else.
>
> Do you know if it's possible to detect if the top-level captions state was set
> to on or off? Maybe the Chromium-side CL already avoid setting anything if
it's
> off?
>
The platform caption settings don't get set in Chromium unless the top-level
captions
state is ON.
> > To handle the case you mentioned, I want to add a check to
> > VTTCue::applyUserOverrideCSSProperties to determine if author-specified
> > background-color and user-specified text color are the same and set the
> > user-style only if they're different(this might confuse the user though). To
> > read author-specified cue css properties, I tried using
element->getAttribute
> > and element->style()->getPropertyValue with no luck. Do you have a
> > recommendation on how I can achieve this?
>
> You'd probably want to look at the computed style of the cue, but the cue
isn't
> added to the DOM before it needs to be rendered, so I think doing any logic
with
> the computed style would imply a two-step layout, which would be unfortunate.
>
> > Also, I checked to see if iOS handles this case and turns out they don't. I
> used
> > a test page with cue color set to red and specified platform settings
> > background-color to be red and text color to use "default". This ended up
> > showing red on red on Chrome and Safari on an iPhone.
>
> Thanks for testing! The behavior sounds unfortunate, but if it turns out very
> hard to avoid perhaps we shouldn't be too distressed.
>
> > > The size setting seems a bit odd to me. Does font scale 1 correspond to a
> > > concrete size on Android? Only scaling the font size won't get the user a
> > > consistent look, if that's the intention.
> >
> > I didn't find any Android specific documentation for font scale mapping to a
> > concrete size in pixels or points. Font scale of 1 corresponds to 100%,
medium
> > and 1em in other font-size units.
>
> I feel I don't have the necessary knowledge of how the Android settings are
> intended to work. I'm going to ask Laura Palmer on Google's accessibility team
> if she can find someone with knowledge of these things.
>
> If these settings do nothing when the captions are off in Android's
> accessibility menu I think it would be OK to land this and iterate from there,
> though.
These settings are not plumbed down to Chromium from platform when the top-level
captions are off.
On 2015/03/18 17:42:12, srivats wrote:
> On 2015/03/18 06:42:24, philipj_UTC7 wrote:
> > On 2015/03/18 02:02:35, srivats wrote:
> > > On 2015/03/17 02:22:32, philipj_UTC7 wrote:
> > > > Great, let's figure this out!
> > > >
> > > > My biggest concern is how this will interact with author style. If we
have
> > > ::cue
> > > > { background-color white; color: black; } in the page's style and only
the
> > > color
> > > > is overridden using this API, the results will not be good.
> > > >
> > > > Is it expected that only one of the setting can be set, or must all of
> them
> > be
> > > > set? Documenting expectations in Settings.in and asserting them
somewhere
> > > would
> > > > be helpful.
> > > >
> > >
> > > The user gets a preview of the caption styles they select on Android and
> Mac.
> > > Here's what the preview looks like on Android:
> > > https://developer.android.com/about/versions/kitkat.html#44-accessibility
> > > The only way the user can pick background color and color separately is
when
> > > they select Custom Style in caption settings.
> >
> > Ah, this is great, I found this menu on my phone and played with it. It
looks
> to
> > me like the user expectation would be that if they turn captions on, the
size
> > they pick will be the size that they see, nothing else.
> >
> > Do you know if it's possible to detect if the top-level captions state was
set
> > to on or off? Maybe the Chromium-side CL already avoid setting anything if
> it's
> > off?
> >
>
> The platform caption settings don't get set in Chromium unless the top-level
> captions
> state is ON.
>
> > > To handle the case you mentioned, I want to add a check to
> > > VTTCue::applyUserOverrideCSSProperties to determine if author-specified
> > > background-color and user-specified text color are the same and set the
> > > user-style only if they're different(this might confuse the user though).
To
> > > read author-specified cue css properties, I tried using
> element->getAttribute
> > > and element->style()->getPropertyValue with no luck. Do you have a
> > > recommendation on how I can achieve this?
> >
> > You'd probably want to look at the computed style of the cue, but the cue
> isn't
> > added to the DOM before it needs to be rendered, so I think doing any logic
> with
> > the computed style would imply a two-step layout, which would be
unfortunate.
> >
> > > Also, I checked to see if iOS handles this case and turns out they don't.
I
> > used
> > > a test page with cue color set to red and specified platform settings
> > > background-color to be red and text color to use "default". This ended up
> > > showing red on red on Chrome and Safari on an iPhone.
> >
> > Thanks for testing! The behavior sounds unfortunate, but if it turns out
very
> > hard to avoid perhaps we shouldn't be too distressed.
> >
> > > > The size setting seems a bit odd to me. Does font scale 1 correspond to
a
> > > > concrete size on Android? Only scaling the font size won't get the user
a
> > > > consistent look, if that's the intention.
> > >
> > > I didn't find any Android specific documentation for font scale mapping to
a
> > > concrete size in pixels or points. Font scale of 1 corresponds to 100%,
> medium
> > > and 1em in other font-size units.
> >
> > I feel I don't have the necessary knowledge of how the Android settings are
> > intended to work. I'm going to ask Laura Palmer on Google's accessibility
team
> > if she can find someone with knowledge of these things.
> >
> > If these settings do nothing when the captions are off in Android's
> > accessibility menu I think it would be OK to land this and iterate from
there,
> > though.
>
> These settings are not plumbed down to Chromium from platform when the
top-level
> captions are off.
I tested this behavior on Nexus and it looks like the settings persist even when
the top-level captions state is off. The recent diff on the Chromium-side CL
handles this issue now.
OK, so the conclusion of the email discussion was that the font scale should be relative to the default size, which is 5vh. It should also be impossible for author style to override this font size. Lacking a way to detect "no preference" for the font scale factor, I think we'll have to assume that if the captions setting (which I'd like to forward to Blink, see the other review) is enabled then the font size will be fixed. At least I have no better idea.
On 2015/03/19 17:48:05, philipj_UTC7 wrote: > OK, so the conclusion of the email discussion was that the font scale should be > relative to the default size, which is 5vh. It should also be impossible for > author style to override this font size. Lacking a way to detect "no preference" > for the font scale factor, I think we'll have to assume that if the captions > setting (which I'd like to forward to Blink, see the other review) is enabled > then the font size will be fixed. At least I have no better idea. When platform captions are disabled, the font scale factor and the other caption settings are not applied to the text tracks and hence we don't have to worry about the off case for any of the settings. This is handled in the Chromium-side CL. When platform captions are enabled, the font scale default value is 1 (converted to 100% to depict font-size) which doesn't change the configured track size (5vh). I verified this using Chrome's dev tools. The case you are concerned about is when a font size of say 150%(font-scale 1.5) is passed. The track size should then scale to 7.5vh. Can you point me to where the 5vh is set to text tracks? The spec says font property must be set to '5vh sans-serif' and looking at mediaControls.css, font for -webkit-media-text-track-container is set to '22px sans-serif'.
On 2015/03/19 19:08:56, srivats wrote: > On 2015/03/19 17:48:05, philipj_UTC7 wrote: > > OK, so the conclusion of the email discussion was that the font scale should > be > > relative to the default size, which is 5vh. It should also be impossible for > > author style to override this font size. Lacking a way to detect "no > preference" > > for the font scale factor, I think we'll have to assume that if the captions > > setting (which I'd like to forward to Blink, see the other review) is enabled > > then the font size will be fixed. At least I have no better idea. > > When platform captions are disabled, the font scale factor and the other caption > settings are not applied to the text tracks and hence we don't have to worry > about the off case for any of the settings. This is handled in the Chromium-side > CL. > When platform captions are enabled, the font scale default value is 1 (converted > to 100% to depict font-size) which doesn't change the configured track size > (5vh). I verified this using Chrome's dev tools. > The case you are concerned about is when a font size of say 150%(font-scale 1.5) > is passed. The track size should then scale to 7.5vh. Can you point me to where > the 5vh is set to text tracks? > The spec says font property must be set to '5vh sans-serif' and looking at > mediaControls.css, font for -webkit-media-text-track-container is set to '22px > sans-serif'. I verified with Dev Tools that font-size scaled the default text track size of 5vh accurately.
On 2015/03/19 19:08:56, srivats wrote:
> On 2015/03/19 17:48:05, philipj_UTC7 wrote:
> > OK, so the conclusion of the email discussion was that the font scale should
> be
> > relative to the default size, which is 5vh. It should also be impossible for
> > author style to override this font size. Lacking a way to detect "no
> preference"
> > for the font scale factor, I think we'll have to assume that if the captions
> > setting (which I'd like to forward to Blink, see the other review) is
enabled
> > then the font size will be fixed. At least I have no better idea.
>
> When platform captions are disabled, the font scale factor and the other
caption
> settings are not applied to the text tracks and hence we don't have to worry
> about the off case for any of the settings. This is handled in the
Chromium-side
> CL.
> When platform captions are enabled, the font scale default value is 1
(converted
> to 100% to depict font-size) which doesn't change the configured track size
> (5vh). I verified this using Chrome's dev tools.
> The case you are concerned about is when a font size of say 150%(font-scale
1.5)
> is passed. The track size should then scale to 7.5vh. Can you point me to
where
> the 5vh is set to text tracks?
Since the vh unit isn't actually supported correctly, there's a workaround in
LayoutTextTrackContainer where a size in pixels is calculated. That seems like
the ideal place to multiply by a scale factor.
In addition to scaling the default 5vh, it will also be necessary to override
author-provided style like ::cue { font-size: 30px }. Unfortunately there's no
such thing as min-font-size and max-font-size, so I think the options are
something in FontSize::getComputedSizeFromSpecifiedSize or ignoring
author-provided style. I think the second is probably better, search for
isValidCueStyleProperty to see where the WebVTT property whitelist is
implemented. Perhaps CSSPropertyOpacity, CSSPropertyVisibility and
CSSPropertyWhiteSpace should still be allowed, but the rest all seem to be
color- or font-related settings. Rather than doing a granular filtering and
worrying about all combinations of settings, I think it'd be fine to just use
the top-level captions setting.
> The spec says font property must be set to '5vh sans-serif' and looking at
> mediaControls.css, font for -webkit-media-text-track-container is set to '22px
> sans-serif'.
Yeah, that size should have no effect, if you would like to remove it to clarify
that would be nice.
Patchset #8 (id:160001) has been deleted
On 2015/03/20 07:04:49, philipj_UTC7 wrote:
> On 2015/03/19 19:08:56, srivats wrote:
> > On 2015/03/19 17:48:05, philipj_UTC7 wrote:
> > > OK, so the conclusion of the email discussion was that the font scale
should
> > be
> > > relative to the default size, which is 5vh. It should also be impossible
for
> > > author style to override this font size. Lacking a way to detect "no
> > preference"
> > > for the font scale factor, I think we'll have to assume that if the
captions
> > > setting (which I'd like to forward to Blink, see the other review) is
> enabled
> > > then the font size will be fixed. At least I have no better idea.
> >
> > When platform captions are disabled, the font scale factor and the other
> caption
> > settings are not applied to the text tracks and hence we don't have to worry
> > about the off case for any of the settings. This is handled in the
> Chromium-side
> > CL.
> > When platform captions are enabled, the font scale default value is 1
> (converted
> > to 100% to depict font-size) which doesn't change the configured track size
> > (5vh). I verified this using Chrome's dev tools.
> > The case you are concerned about is when a font size of say 150%(font-scale
> 1.5)
> > is passed. The track size should then scale to 7.5vh. Can you point me to
> where
> > the 5vh is set to text tracks?
>
> Since the vh unit isn't actually supported correctly, there's a workaround in
> LayoutTextTrackContainer where a size in pixels is calculated. That seems like
> the ideal place to multiply by a scale factor.
>
> In addition to scaling the default 5vh, it will also be necessary to override
> author-provided style like ::cue { font-size: 30px }. Unfortunately there's no
> such thing as min-font-size and max-font-size, so I think the options are
> something in FontSize::getComputedSizeFromSpecifiedSize or ignoring
> author-provided style. I think the second is probably better, search for
> isValidCueStyleProperty to see where the WebVTT property whitelist is
> implemented. Perhaps CSSPropertyOpacity, CSSPropertyVisibility and
> CSSPropertyWhiteSpace should still be allowed, but the rest all seem to be
> color- or font-related settings. Rather than doing a granular filtering and
> worrying about all combinations of settings, I think it'd be fine to just use
> the top-level captions setting.
>
> > The spec says font property must be set to '5vh sans-serif' and looking at
> > mediaControls.css, font for -webkit-media-text-track-container is set to
'22px
> > sans-serif'.
>
> Yeah, that size should have no effect, if you would like to remove it to
clarify
> that would be nice.
Hey Philip,
I added an API for platform closed captions state, which when enabled, ignores
the author-specified cue settings. I'm waiting on your review for this change
before requesting an update on the Chromium-side CL. Please let me know if this
looks reasonable.
Thanks,
Sunitha
https://codereview.chromium.org/921833003/diff/180001/Source/core/css/mediaCo... File Source/core/css/mediaControls.css (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/css/mediaCo... Source/core/css/mediaControls.css:351: font-family: sans-serif; Thanks! https://codereview.chromium.org/921833003/diff/180001/Source/core/css/resolve... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/css/resolve... Source/core/css/resolver/StyleResolver.cpp:1055: case CSSPropertyOpacity: On second thought it's odd to allow opacity and visibility but not color, given that either could be used with the :future pseudo-class for similar effects. I think allowing no properties at all might be the simplest thing to do until we have a reason to do something more clever. https://codereview.chromium.org/921833003/diff/180001/Source/core/frame/Setti... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/frame/Setti... Source/core/frame/Settings.in:330: platformClosedCaptionsEnabled initial=false Thank you! The bit of the spec where I hoped to use this says "If the user has expressed an interest in having a track from candidates enabled based on its text track kind, text track language, and text track label, then set its text track mode to showing." (https://html.spec.whatwg.org/#perform-automatic-text-track-selection) This is what I would need to implement the above: # The preferred kind of text track to enable automatically, either "captions", "subtitles" or the empty string if only default tracks should be enabled automatically. textTrackKind type=String # The preferred text track language. If set to the empty string, the userPreferredLanguages() (see platform/Language.h) is used instead. textTrackLanguage type=String Having spelled it out, I see that this isn't what you need in this CL, using settings.textTrackKind() != "" as a condition in StyleResolver will look mysterious. platformClosedCaptionsEnabled does too, it'll only make sense if you know what the menu structure looks like on Android... Renaming the setting to textTrackIgnoreAuthorStyle or textTrackAnyStyleOverridden would make it more accurate, but I think your original approach seems better. The simplest thing I can come up with without introducing another setting is to ignore all author style if any of these settings are set. If you think more granular filtering is worthwhile I won't object, but it will require more tests. https://codereview.chromium.org/921833003/diff/180001/Source/core/html/track/... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:141: static void setInlineStylePropertyIfNotEmpty(PassRefPtrWillBeRawPtr<Element> element, I meant that instead of "PassRefPtrWillBeRawPtr<Element> element" this can say just "Element& element". https://codereview.chromium.org/921833003/diff/180001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:145: element->setInlineStyleProperty(propertyID, value, true); The third argument here is "bool important", and shouldn't be needed if author style is ignored in StyleResolver.cpp.
https://codereview.chromium.org/921833003/diff/180001/Source/core/css/resolve... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/css/resolve... Source/core/css/resolver/StyleResolver.cpp:1055: case CSSPropertyOpacity: On 2015/03/24 07:11:58, philipj_UTC7 wrote: > On second thought it's odd to allow opacity and visibility but not color, given > that either could be used with the :future pseudo-class for similar effects. I > think allowing no properties at all might be the simplest thing to do until we > have a reason to do something more clever. Done. https://codereview.chromium.org/921833003/diff/180001/Source/core/frame/Setti... File Source/core/frame/Settings.in (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/frame/Setti... Source/core/frame/Settings.in:330: platformClosedCaptionsEnabled initial=false On 2015/03/24 07:11:58, philipj_UTC7 wrote: > Thank you! The bit of the spec where I hoped to use this says "If the user has > expressed an interest in having a track from candidates enabled based on its > text track kind, text track language, and text track label, then set its text > track mode to showing." > (https://html.spec.whatwg.org/#perform-automatic-text-track-selection) > > This is what I would need to implement the above: > > # The preferred kind of text track to enable automatically, either "captions", > "subtitles" or the empty string if only default tracks should be enabled > automatically. > textTrackKind type=String > # The preferred text track language. If set to the empty string, the > userPreferredLanguages() (see platform/Language.h) is used instead. > textTrackLanguage type=String > > Having spelled it out, I see that this isn't what you need in this CL, using > settings.textTrackKind() != "" as a condition in StyleResolver will look > mysterious. platformClosedCaptionsEnabled does too, it'll only make sense if you > know what the menu structure looks like on Android... > > Renaming the setting to textTrackIgnoreAuthorStyle or > textTrackAnyStyleOverridden would make it more accurate, but I think your > original approach seems better. The simplest thing I can come up with without > introducing another setting is to ignore all author style if any of these > settings are set. If you think more granular filtering is worthwhile I won't > object, but it will require more tests. I removed platformClosedCaptionsEnabled and I'm ignoring author settings when any of the user settings are present in StyleResolver. Please let me know if this is better. https://codereview.chromium.org/921833003/diff/180001/Source/core/html/track/... File Source/core/html/track/vtt/VTTCue.cpp (right): https://codereview.chromium.org/921833003/diff/180001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:141: static void setInlineStylePropertyIfNotEmpty(PassRefPtrWillBeRawPtr<Element> element, On 2015/03/24 07:11:58, philipj_UTC7 wrote: > I meant that instead of "PassRefPtrWillBeRawPtr<Element> element" this can say > just "Element& element". Done. https://codereview.chromium.org/921833003/diff/180001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.cpp:145: element->setInlineStyleProperty(propertyID, value, true); On 2015/03/24 07:11:58, philipj_UTC7 wrote: > The third argument here is "bool important", and shouldn't be needed if author > style is ignored in StyleResolver.cpp. Done.
Yes, I think this looks very promising, there's just the question of not filtering out any internal style. Would any of the tests fail if that happens? If not maybe a test to see what happens to the background color when one only overrides the font size would be in order. LGTM if this is in fact already covered by a test so you don't have to wait another day for me to acknowledge that. https://codereview.chromium.org/921833003/diff/200001/Source/core/css/resolve... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/921833003/diff/200001/Source/core/css/resolve... Source/core/css/resolver/StyleResolver.cpp:1252: if (propertyWhitelistType == PropertyWhitelistCue && !isValidCueStyleProperty(property, shouldIgnoreTextTrackAuthorStyle())) When structured like this there's no reason to pass the bool to isValidCueStyleProperty, instead just check it before the call. Can you also verify if this still lets through the style in mediaContros.css, like ::cue { background-color: rgba(0, 0, 0, 0.8) }? If it doesn't I presume you'll have to also test if we're currently parsing a UA style sheet or not.
On 2015/03/25 16:29:21, philipj_UTC7 wrote: > Yes, I think this looks very promising, there's just the question of not > filtering out any internal style. Would any of the tests fail if that happens? > If not maybe a test to see what happens to the background color when one only > overrides the font size would be in order. > > LGTM if this is in fact already covered by a test so you don't have to wait > another day for me to acknowledge that. > I will add a test for this. > https://codereview.chromium.org/921833003/diff/200001/Source/core/css/resolve... > File Source/core/css/resolver/StyleResolver.cpp (right): > > https://codereview.chromium.org/921833003/diff/200001/Source/core/css/resolve... > Source/core/css/resolver/StyleResolver.cpp:1252: if (propertyWhitelistType == > PropertyWhitelistCue && !isValidCueStyleProperty(property, > shouldIgnoreTextTrackAuthorStyle())) > When structured like this there's no reason to pass the bool to > isValidCueStyleProperty, instead just check it before the call. > > Can you also verify if this still lets through the style in mediaContros.css, > like ::cue { background-color: rgba(0, 0, 0, 0.8) }? If it doesn't I presume > you'll have to also test if we're currently parsing a UA style sheet or not. I'll combine this with the test I'm planning to add to verify that the internal styles are applied when user setting is not.
https://codereview.chromium.org/921833003/diff/200001/Source/core/css/mediaCo... File Source/core/css/mediaControls.css (right): https://codereview.chromium.org/921833003/diff/200001/Source/core/css/mediaCo... Source/core/css/mediaControls.css:351: font-family: sans-serif; I reverted this back because this change broke the test: media/track/track-cue-rendering-on-resize.html https://codereview.chromium.org/921833003/diff/200001/Source/core/css/resolve... File Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/921833003/diff/200001/Source/core/css/resolve... Source/core/css/resolver/StyleResolver.cpp:1252: if (propertyWhitelistType == PropertyWhitelistCue && !isValidCueStyleProperty(property, shouldIgnoreTextTrackAuthorStyle())) On 2015/03/25 16:29:20, philipj_UTC7 wrote: > When structured like this there's no reason to pass the bool to > isValidCueStyleProperty, instead just check it before the call. > > Can you also verify if this still lets through the style in mediaContros.css, > like ::cue { background-color: rgba(0, 0, 0, 0.8) }? If it doesn't I presume > you'll have to also test if we're currently parsing a UA style sheet or not. Done.
LGTM with some additional nits. fs, can you take another look too?
I think the tests would be simpler and clearer as reference tests, look of
*-expected.html to see how that works. That way your test would simply set one
or several of settings, and the reference would do the same thing using author
style. This would make it rather easy to verify that ::cue(c) { color: red } has
no effect if any of the settings are set, where you would have to find the
relevant internal node to do it with the current setup. The existing tests are
OK if you don't want to fiddle any more, though.
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
File LayoutTests/media/track/track-css-reset-user-override-settings.html
(right):
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
LayoutTests/media/track/track-css-reset-user-override-settings.html:58:
consoleWrite("Test that WebVTT objects user override styles when reset, refer to
css specified styles.");
I don't think this test is needed, because initUserSettings() and
resetUserSettings() follow each other, it merely tests that the settings do
nothing in their initial state.
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
File
LayoutTests/media/track/track-css-user-settings-override-author-settings.html
(right):
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
LayoutTests/media/track/track-css-user-settings-override-author-settings.html:6:
/* App default settings for the cues */
I'm not sure what app means, both of these style blocks are author settings for
the cues, right?
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
LayoutTests/media/track/track-css-user-settings-override-author-settings.html:17:
video::cue(c) {
Does this style block (and the change to styling.vtt) make any difference to the
test? You don't find the internal node and test its style, after all.
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
LayoutTests/media/track/track-css-user-settings-override-author-settings.html:75:
waitForEvent('canplaythrough', function() { video.currentTime = 0.1; });
Same simplification possible.
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
File
LayoutTests/media/track/track-css-user-settings-override-internal-settings.html
(right):
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track...
LayoutTests/media/track/track-css-user-settings-override-internal-settings.html:44:
waitForEvent('canplaythrough', function() { video.currentTime = 0.1; });
To simplify, you can set currentTime immediate after setting src, and it will
seek as soon as possible.
https://codereview.chromium.org/921833003/diff/220001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.h (right):
https://codereview.chromium.org/921833003/diff/220001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.h:263: bool
shouldIgnoreTextTrackAuthorStyle();
Oh wait, this doesn't need to be in the header, it can be a static in the cpp
file which takes a Document as an argument.
https://codereview.chromium.org/921833003/diff/220001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.h (right):
https://codereview.chromium.org/921833003/diff/220001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.h:145: // Applies CSS properties retrieved
from settings on text tracks elements.
"from settings on text track elements" sounds like it might be attributes on
<track> elements or something. Maybe "Applies CSS override style from user
settings." or similar?
Also put this directly following applyCSSProperties in both header and
implementation, they fit together.
Patchset #11 (id:240001) has been deleted
https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... File LayoutTests/media/track/track-css-reset-user-override-settings.html (right): https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... LayoutTests/media/track/track-css-reset-user-override-settings.html:58: consoleWrite("Test that WebVTT objects user override styles when reset, refer to css specified styles."); On 2015/03/26 05:24:36, philipj_UTC7 wrote: > I don't think this test is needed, because initUserSettings() and > resetUserSettings() follow each other, it merely tests that the settings do > nothing in their initial state. Removed. The initial reasoning to add this was to verify that passing empty strings doesn't override default settings. https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... File LayoutTests/media/track/track-css-user-settings-override-author-settings.html (right): https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... LayoutTests/media/track/track-css-user-settings-override-author-settings.html:6: /* App default settings for the cues */ On 2015/03/26 05:24:36, philipj_UTC7 wrote: > I'm not sure what app means, both of these style blocks are author settings for > the cues, right? The app default settings block was supposed to override the cue style specified in mediaControls.css. But it's not really needed in this test anymore. https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... LayoutTests/media/track/track-css-user-settings-override-author-settings.html:17: video::cue(c) { On 2015/03/26 05:24:36, philipj_UTC7 wrote: > Does this style block (and the change to styling.vtt) make any difference to the > test? You don't find the internal node and test its style, after all. This specifies the author settings for the cue nodes. I am testing the settings out on cue nodes by calling cue = textTrackDisplayElement(video, 'cue').firstElementChild; https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... LayoutTests/media/track/track-css-user-settings-override-author-settings.html:75: waitForEvent('canplaythrough', function() { video.currentTime = 0.1; }); On 2015/03/26 05:24:36, philipj_UTC7 wrote: > Same simplification possible. Done. https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... File LayoutTests/media/track/track-css-user-settings-override-internal-settings.html (right): https://codereview.chromium.org/921833003/diff/220001/LayoutTests/media/track... LayoutTests/media/track/track-css-user-settings-override-internal-settings.html:44: waitForEvent('canplaythrough', function() { video.currentTime = 0.1; }); On 2015/03/26 05:24:36, philipj_UTC7 wrote: > To simplify, you can set currentTime immediate after setting src, and it will > seek as soon as possible. Done. https://codereview.chromium.org/921833003/diff/220001/Source/core/css/resolve... File Source/core/css/resolver/StyleResolver.h (right): https://codereview.chromium.org/921833003/diff/220001/Source/core/css/resolve... Source/core/css/resolver/StyleResolver.h:263: bool shouldIgnoreTextTrackAuthorStyle(); On 2015/03/26 05:24:36, philipj_UTC7 wrote: > Oh wait, this doesn't need to be in the header, it can be a static in the cpp > file which takes a Document as an argument. Done. https://codereview.chromium.org/921833003/diff/220001/Source/core/html/track/... File Source/core/html/track/vtt/VTTCue.h (right): https://codereview.chromium.org/921833003/diff/220001/Source/core/html/track/... Source/core/html/track/vtt/VTTCue.h:145: // Applies CSS properties retrieved from settings on text tracks elements. On 2015/03/26 05:24:36, philipj_UTC7 wrote: > "from settings on text track elements" sounds like it might be attributes on > <track> elements or something. Maybe "Applies CSS override style from user > settings." or similar? > Done > Also put this directly following applyCSSProperties in both header and > implementation, they fit together. applyCSSProperties belongs to VTTCueBox and applyUserOverrideCSSProperties belongs to VTTCue.
lgtm
On 2015/03/26 06:37:48, srivats wrote: > https://codereview.chromium.org/921833003/diff/220001/Source/core/html/track/... > File Source/core/html/track/vtt/VTTCue.h (right): > > https://codereview.chromium.org/921833003/diff/220001/Source/core/html/track/... > Source/core/html/track/vtt/VTTCue.h:145: // Applies CSS properties retrieved > from settings on text tracks elements. > On 2015/03/26 05:24:36, philipj_UTC7 wrote: > > "from settings on text track elements" sounds like it might be attributes on > > <track> elements or something. Maybe "Applies CSS override style from user > > settings." or similar? > > > Done > > Also put this directly following applyCSSProperties in both header and > > implementation, they fit together. > applyCSSProperties belongs to VTTCueBox and applyUserOverrideCSSProperties > belongs to VTTCue. Oh, right. I see that you've moved it up a bit and that's all I really meant, so that the boilerplate (DECLARE_VIRTUAL_TRACE) isn't in the middle of the "real" stuff. Thanks for your patience, nothing further from me now.
The CQ bit was checked by srivats@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/921833003/#ps260001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/921833003/260001
Message was sent while issue was closed.
Committed patchset #11 (id:260001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192627 |
