[Media Router WebUI] Pause/mute/volume controls are in a wrong order in RTL in WebUI route controller |
|||||||||
Issue descriptionAs per UI review discussion, the controls should look like: LTR order: pause button, LTR-facing mute button, LTR-facing volume slider RTL order: RTL-facing volume slider, RTL-facing mute button, pause button
,
Aug 17 2017
Here's a screenshot of what it would look like after the fix at https://chromium-review.googlesource.com/c/613842.
,
Aug 17 2017
,
Aug 17 2017
,
Aug 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a2c238d44612e7da16927963eb7c72a4ed655812 commit a2c238d44612e7da16927963eb7c72a4ed655812 Author: Takumi Fujimoto <takumif@chromium.org> Date: Fri Aug 18 04:48:45 2017 [Media Router WebUI] Switch pause/mute buttons' locations in RTL languages Order before: mute button (LTR), pause button, volume slider (LTR) Order after: volume slider (RTL), mute button (RTL), pause button Bug: 756625 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I56903c78923e2bc38051a3ff77fb5e1a27c90b21 Reviewed-on: https://chromium-review.googlesource.com/613842 Reviewed-by: Derek Cheng <imcheng@chromium.org> Reviewed-by: mark a. foltz <mfoltz@chromium.org> Commit-Queue: Takumi Fujimoto <takumif@chromium.org> Cr-Commit-Position: refs/heads/master@{#495464} [modify] https://crrev.com/a2c238d44612e7da16927963eb7c72a4ed655812/chrome/browser/resources/media_router/elements/route_controls/route_controls.css [modify] https://crrev.com/a2c238d44612e7da16927963eb7c72a4ed655812/chrome/browser/resources/media_router/elements/route_controls/route_controls.html
,
Aug 18 2017
Requesting merge to M61.
,
Aug 18 2017
Before we approve merge to M61, please answer followings: * Is this M61 regression? Is it critical? * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61? * Any other important details to justify the merge. Please note M61 is already in Beta, so merge bar is very high. Thank you.
,
Aug 18 2017
This change is necessary to unblock the launch review for a Finch experiment that is currently enabled on M61/62 Canary/Dev, and we are trying to enable it on M61 Beta/Stable. This fix addresses a regression caused by the Finch experiment, but the regression currently does not affect M61 Beta/Stable, since the experiment is not enabled there. The change has yet to land in Canary, but it is working on TOT and once it goes to Canary it'll go through weekly manual testing by our test team (it is a visual change). Given it is a CSS/visual change, it is unlikely for the change to introduce unsafe behavior. Thanks for taking a look.
,
Aug 19 2017
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 19 2017
Thank you takumif@. +abdulsyed, to take his opinion on the merge per comment #8.
,
Aug 21 2017
This seems to be a bad user experience for RTL language users. My recommendation is to wait and update the bug with Canary results. Is there a reason why it's not in Canary yet? If canary looks ok, then I'm fine with approving merge to M61.
,
Aug 21 2017
Thank you abdulsyed@. takumif@, change should be in canary by now. How is it looking in Canary?
,
Aug 21 2017
Thanks govind@, abdulsyed@. I've verified on Windows and Mac Canary today that the fix is working as intended.
,
Aug 21 2017
Approving merge to M61 branch 3163 based on comment #11 and #13. Please merge ASAP. Thank you.
,
Aug 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d6199a255a151895d843800dccb0c9eb1e6056d1 commit d6199a255a151895d843800dccb0c9eb1e6056d1 Author: Takumi Fujimoto <takumif@chromium.org> Date: Mon Aug 21 20:33:57 2017 [Media Router WebUI] Switch pause/mute buttons' locations in RTL languages Order before: mute button (LTR), pause button, volume slider (LTR) Order after: volume slider (RTL), mute button (RTL), pause button TBR=takumif@chromium.org (cherry picked from commit a2c238d44612e7da16927963eb7c72a4ed655812) Bug: 756625 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I56903c78923e2bc38051a3ff77fb5e1a27c90b21 Reviewed-on: https://chromium-review.googlesource.com/613842 Reviewed-by: Derek Cheng <imcheng@chromium.org> Reviewed-by: mark a. foltz <mfoltz@chromium.org> Commit-Queue: Takumi Fujimoto <takumif@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#495464} Reviewed-on: https://chromium-review.googlesource.com/624754 Reviewed-by: Takumi Fujimoto <takumif@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#711} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/d6199a255a151895d843800dccb0c9eb1e6056d1/chrome/browser/resources/media_router/elements/route_controls/route_controls.css [modify] https://crrev.com/d6199a255a151895d843800dccb0c9eb1e6056d1/chrome/browser/resources/media_router/elements/route_controls/route_controls.html
,
Aug 21 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by taku...@chromium.org
, Aug 17 2017