New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 756625 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 684642



Sign in to add a comment

[Media Router WebUI] Pause/mute/volume controls are in a wrong order in RTL in WebUI route controller

Project Member Reported by taku...@chromium.org, Aug 17 2017

Issue description

As 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
 
Description: Show this description
Here's a screenshot of what it would look like after the fix at
https://chromium-review.googlesource.com/c/613842.
rtl_fixed.png
11.7 KB View Download
Summary: [Media Router WebUI] Pause/mute/volume controls are in a wrong order in RTL in WebUI route controller (was: [Media Router WebUI] Pause and mute buttons are reversed in RTL in WebUI route controller)
Blocking: 684642
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Labels: Merge-Request-61
Requesting merge to M61.

Comment 7 by gov...@chromium.org, 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.
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.
Project Member

Comment 9 by sheriffbot@chromium.org, Aug 19 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
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
Cc: abdulsyed@chromium.org
Thank you takumif@. 
+abdulsyed, to take his opinion on the merge per comment #8.
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. 
Thank you abdulsyed@.

takumif@, change should be in canary by now. How is it looking in Canary?

Thanks govind@, abdulsyed@. I've verified on Windows and Mac Canary today that the fix is working as intended.
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #11 and #13. Please merge ASAP. Thank you.
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 21 2017

Labels: -merge-approved-61 merge-merged-3163
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

Status: Fixed (was: Started)

Sign in to add a comment