Capitalize button text only on appropriate platforms |
|||||||||||||
Issue descriptionChrome's native VR UI currently follows Daydream style. For most buttons, Daydream style is similar to Material Design. Like Material Design, Daydream pill and square buttons have uppercase text [1]. However, Daydream disk-style buttons do not. This is currently hard-coded in the code using base::i18n::ToUpper() as in https://crrev.com/c/777861. If we need to use different styles on different platforms (issue 787888), application of ToUpper() needs to be conditional. This should be done as part of a more general solution (issue 787912). This issue tracks specific instances related to button text case in the code. Note: All strings in the GRD files should always be normal case. This allows for use on multiple platforms and reuse of translations in the event that the string is used elsewhere. [1] Material Design specifies that "Button text should be capitalized in languages that have capitalization." -- https://material.io/guidelines/components/buttons.html#buttons-style
,
Nov 22 2017
,
Nov 22 2017
Why aren't you just putting all your strings in the GRD file twice, once for use_titlecase and once for "not use_titlecase" and then letting the translation folks figure out the correct strings for each case? You shouldn't be trying to use ToUpper() as it may not always work with different locales.
,
Nov 22 2017
This is for all upper case, not title case. ToUpper() is intended to support this: https://cs.chromium.org/chromium/src/base/i18n/case_conversion.h?type=cs&q=ToUpper&sq=package:chromium&l=38. This should in theory be equivalent to `android:textAllCaps` in Android styles.
,
Nov 22 2017
Ah ok. Well, I'm no i18n expert. My suggestion is to ask them about your use case.
,
Nov 22 2017
amyroberts, gordonbrander, meganlindsay: Please confirm that ToUpper() is appropriate here.
,
Nov 22 2017
,
Nov 22 2017
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f2848d15b9bd2d1c97333568ccb3730c59cf43c3 commit f2848d15b9bd2d1c97333568ccb3730c59cf43c3 Author: David Dorwin <ddorwin@chromium.org> Date: Wed Nov 22 23:10:13 2017 Make Exit VR prompt button text to be uppercase The uppercase styling should be applied in code and not depend on the actual strings. Leave the "EXIT VR" string unchanged for now. Bug: 787654 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: If265f4f6a6584f95bd0f6cdb39f26632875967a4 Reviewed-on: https://chromium-review.googlesource.com/780692 Reviewed-by: Ian Vollick <vollick@chromium.org> Commit-Queue: David Dorwin <ddorwin@chromium.org> Cr-Commit-Position: refs/heads/master@{#518788} [modify] https://crrev.com/f2848d15b9bd2d1c97333568ccb3730c59cf43c3/chrome/app/generated_resources.grd [modify] https://crrev.com/f2848d15b9bd2d1c97333568ccb3730c59cf43c3/chrome/browser/vr/elements/audio_permission_prompt_texture.cc [modify] https://crrev.com/f2848d15b9bd2d1c97333568ccb3730c59cf43c3/chrome/browser/vr/elements/exit_prompt_texture.cc [modify] https://crrev.com/f2848d15b9bd2d1c97333568ccb3730c59cf43c3/chrome/browser/vr/ui_scene_manager.cc
,
Feb 28 2018
This is still marked as 65, is there still work to do here? Which milestone should we target?
,
Mar 2 2018
ddorwin : this is a p3 on m65. per c#10, is there more work to do here? and if so what is that priority? doesn't seem P1, maybe P2 - MVP-Next?
,
Mar 2 2018
The remaining work is to only uppercase these strings when appropriate, which depends on issue 787912. Specifically, this would address these TODOs: https://cs.chromium.org/search/?q=TODO.*787654&sq=package:chromium&type=cs This is really blocking use of this code on platforms other than Daydream.
,
Mar 7 2018
,
Mar 9 2018
,
Jul 12
,
Jul 16
,
Dec 7
,
Jan 10
Among other things, UiSceneCreator::CreatePrompts() references this, and I think we're using that on Windows. Thus, this may block issue 910748.
,
Jan 10
We don't show buttons or handle input in VR-UI on Windows, so this won't block our current set of planned in-headset UI on Windows.
,
Jan 10
We should maybe ifdef more of the code or refactor so it's easier to understand what we do use on each platform. (That's separate from this issue.)
,
Jan 10
crbug/920700 for the "refactor"/cleanup to make it clear what we use on each platform. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by ddorwin@chromium.org
, Nov 22 2017