[SVG] Propagate user languages preferences to SVG images for systemLanguage support [chromium/src : main]
Divyansh Mangal (Gerrit)
Message from Divyansh Mangal![]( "Open in Gerrit")
Set Ready For Review
Related details
- Divyansh Mangal
- Ian Kilpatrick
- Philip Rogers
- Vinay Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 5 comments![]( "Open in Gerrit")
[SVG] Propagate user languages preferences to SVG images for
systemLanguage support
When an SVG is loaded as an image, it runs inside an isolated page
with an `SVGImageChromeClient` that inherits from
`EmptyChromeClient`. `EmptyChromeClient::AcceptLanguages()` returns
an empty string, which causes `SVGTests::IsValid()` to never match
any `systemLanguage` value. This means <switch> elements with
`systemLanguage` always fall through to the fallback branch in image
contexts.
In this CL, we fix this issue by overriding `AcceptLanguages()` on
`SVGImageChromeClient` and propagating the user language preferences
in `SVGImage::DataChanged()`, similar to how font settings and color
maps are already copied.TODO: This description and title would need to be updated if we decided to go with the new approach as being discussed in
https://chromium-review.googlesource.com/c/chromium/src/+/7579553/comment/5483f2b5_0cd9089a/
// attributes like systemLanguage work correctly in image contexts.Divyansh MangalThere's code for invalidating system languages (WebViewImpl::AcceptLanguagesChanged). Is there a clean way to wire up invalidation for when the accept languages change? If not, can you just add a note of that in the comment?
Just like Font settings, accept languages is fixed inside and will not update if changed (unless Page is reloaded).
There is already a note in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/svg/graphics/svg_image.cc;drc=abdfdef6a8e6dfabec1ad3cdfc3f83a849d21319;l=691
and as per the changes in the latest patchset this should cover accept language as well.
(However, in the old patchet (patchset 6) such a comment would have to be explicitly added.).
Let me know if have any more questions.
String accept_languages_;Divyansh MangalThis feels out of place, but it may be the best place for this. Is it an option to move accept languages off of ChromeClient and onto Settings or Page? This is just a question rather than a request.
That's a great question @p...@chromium.org, it's definitely possible and in fact make sense to move accept languages off of `ChromeClient` onto `Settings`. `Settings` already stores per-page configuration, and accept languages is conceptually a preference/setting sourced from `RendererPreferences`. In fact, in the latest PatchSet, I have tried to do that, changes are mainly straightforward, instead of `ChromeClient` now `Settings` are used to update/refer Accept Languages.
I actually originally thought of doing this, but I was not sure adding in settings.json5 is acceptable as per the top description in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/settings.json5;drc=b29d63222d10f4c7e620d057578d737969eb7ae3;l=2 ?
`Page` is also possible but less clean, `Page` doesn't typically own configuration data directly; it delegates to `Settings` for that.
Existing tests related to system language also passes in the latest PatchSet, but let me know your thoughts on this as well.
void SetAcceptLanguages(const String& accept_languages) {Divyansh MangalShould this work for a svg resource document? If so, can you move this to IsolatedSVGChromeClient and add a test of that?
For svg resource document, this issue doesn't occur as those documents seems to utilise the normal Page's settings itself unlike SVG Images.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/svg/graphics/svg_image.cc;drc=abdfdef6a8e6dfabec1ad3cdfc3f83a849d21319;l=691
I also checked this manually with some svgs. (I utilised <use> element which referred an external document).
<!DOCTYPE HTML>Divyansh MangalIs it possible for this to be a WPT test? I see some code for setting accept-language for some variants of WPT tests (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/wpt_tests/product.py;l=174?q=wpt%20%22Accept-Language%22&ss=chromium%2Fchromium%2Fsrc&start=11#:~:text=173-,174,-175), so I wonder if this would work as a WPT test that assumes the en system language?
If not a WPT test, can this be put in wpt_internal/? We try to add all new tests in the WPT format. wpt_internal/ has access to the internals API. If this is very complicated, a regular wept test as you have here seems fine.
Thanks for the reference @p...@chromium.org, if the headless shell has such setup of accept language already then, yes it's possible to convert this test to a WPT one.
I have converted it to a WPT in the latest Patchset but let me know if more changes are needed.
(Earlier I mainly kept this in third_party/blink/web_tests/svg/dom/ cause all other related systemLanguage test for svg where there.)
Related details
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
GetPage()->GetSettings().SetAcceptLanguages(Optional: can we even go a little further and remove WebViewImpl::AcceptLanguagesChanged? This is called from WebViewImpl::UpdateRendererPreferences, but we can just set the blink setting directly there (see other code in the area). Rather than calling GetPage()->AcceptLanguagesChanged here, we can call it from Page::SettingsChanged, and make Page::AcceptLanguagesChanged private, and have the blink setting invalidate kAcceptLanguages. WDYT?
String accept_languages_;Divyansh MangalThis feels out of place, but it may be the best place for this. Is it an option to move accept languages off of ChromeClient and onto Settings or Page? This is just a question rather than a request.
That's a great question @p...@chromium.org, it's definitely possible and in fact make sense to move accept languages off of `ChromeClient` onto `Settings`. `Settings` already stores per-page configuration, and accept languages is conceptually a preference/setting sourced from `RendererPreferences`. In fact, in the latest PatchSet, I have tried to do that, changes are mainly straightforward, instead of `ChromeClient` now `Settings` are used to update/refer Accept Languages.
I actually originally thought of doing this, but I was not sure adding in settings.json5 is acceptable as per the top description in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/settings.json5;drc=b29d63222d10f4c7e620d057578d737969eb7ae3;l=2 ?
`Page` is also possible but less clean, `Page` doesn't typically own configuration data directly; it delegates to `Settings` for that.
Existing tests related to system language also passes in the latest PatchSet, but let me know your thoughts on this as well.
Thanks for looking into this. After seeing the patch, I agree that Settings is a bit of a mess, but ChromeClient is too, so on balance I think the settings approach is the way to go.
<!DOCTYPE HTML>Divyansh MangalIs it possible for this to be a WPT test? I see some code for setting accept-language for some variants of WPT tests (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/wpt_tests/product.py;l=174?q=wpt%20%22Accept-Language%22&ss=chromium%2Fchromium%2Fsrc&start=11#:~:text=173-,174,-175), so I wonder if this would work as a WPT test that assumes the en system language?
If not a WPT test, can this be put in wpt_internal/? We try to add all new tests in the WPT format. wpt_internal/ has access to the internals API. If this is very complicated, a regular wept test as you have here seems fine.
Thanks for the reference @p...@chromium.org, if the headless shell has such setup of accept language already then, yes it's possible to convert this test to a WPT one.
I have converted it to a WPT in the latest Patchset but let me know if more changes are needed.(Earlier I mainly kept this in third_party/blink/web_tests/svg/dom/ cause all other related systemLanguage test for svg where there.)
I looked around the WPT infrastructure and I am not sure the approach of using "en" will work for other engines, or if it will cause issues for configurations not using "en". Sorry for having you move this and then move it again. Can you move this to wpt_internal/ and use `internals.settings.setAcceptLanguages("foo");`?
Related details
- Divyansh Mangal
- Ian Kilpatrick
- Vinay Singh
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
- Divyansh Mangal
- Vinay Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |