Fix RTL with keyboard shortcuts to move tabs.

CTRL+RIGHT with keyboard focus on a tab will now always move the current
tab right, and CTRL+LEFT will move it left. Likewise, CTRL+SHIFT+RIGHT
will always move the current tab to the right end of the tabstrip, and
CTRL+SHIFT+LEFT will move to the left end.

Prelude to fixing the attached bug.

Bug: 1110426
Change-Id: Ib29d539db982b9b970ea192ca9edb13b18ca7d0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2528179Reviewed-by: Charlene Yan <cyan@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826112}

chrome/browser/ui/views/tabs/tab.cc

@@ -424,24 +424,22 @@ bool Tab::OnKeyPressed(const ui::KeyEvent& event) {
 #endif
 
   if (event.type() == ui::ET_KEY_PRESSED && (event.flags() & kModifiedFlag)) {
-    if (event.flags() & ui::EF_SHIFT_DOWN) {
-      if (event.key_code() == ui::VKEY_RIGHT) {
-        controller()->MoveTabLast(this);
-        return true;
-      }
-      if (event.key_code() == ui::VKEY_LEFT) {
-        controller()->MoveTabFirst(this);
-        return true;
-      }
-    } else {
-      if (event.key_code() == ui::VKEY_RIGHT) {
-        controller()->ShiftTabRight(this);
-        return true;
-      }
-      if (event.key_code() == ui::VKEY_LEFT) {
-        controller()->ShiftTabLeft(this);
-        return true;
+    const bool is_right = event.key_code() == ui::VKEY_RIGHT;
+    const bool is_left = event.key_code() == ui::VKEY_LEFT;
+    if (is_right || is_left) {
+      const bool is_rtl = base::i18n::IsRTL();
+      const bool is_next = (is_right && !is_rtl) || (is_left && is_rtl);
+      if (event.flags() & ui::EF_SHIFT_DOWN) {
+        if (is_next)
+          controller()->MoveTabLast(this);
+        else
+          controller()->MoveTabFirst(this);
+      } else if (is_next) {
+        controller()->ShiftTabNext(this);
+      } else {
+        controller()->ShiftTabPrevious(this);
       }
+      return true;
     }
   }
 

chrome/browser/ui/views/tabs/tab_controller.h

@@ -58,11 +58,13 @@ class TabController {
   // Closes the tab.
   virtual void CloseTab(Tab* tab, CloseTabSource source) = 0;
 
-  // Attempts to shift the specified tab to the right by one index.
-  virtual void ShiftTabRight(Tab* tab) = 0;
+  // Attempts to shift the specified tab towards the end of the tabstrip by one
+  // index.
+  virtual void ShiftTabNext(Tab* tab) = 0;
 
-  // Attempts to shift the specified tab to the left by one index.
-  virtual void ShiftTabLeft(Tab* tab) = 0;
+  // Attempts to shift the specified tab towards the start of the tabstrip by
+  // one index.
+  virtual void ShiftTabPrevious(Tab* tab) = 0;
 
   // Attempts to move the specified tab to the beginning of the tabstrip (or the
   // beginning of the unpinned tab region if the tab is not pinned).

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -18,6 +18,7 @@
 #include "base/containers/adapters.h"
 #include "base/containers/flat_map.h"
 #include "base/feature_list.h"
+#include "base/i18n/rtl.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_functions.h"
@@ -127,9 +128,6 @@ constexpr int kStackedPadding = 6;
 int g_drop_indicator_width = 0;
 int g_drop_indicator_height = 0;
 
-// The offset in indices when shifting a tab or group in the given direction.
-enum ShiftOffset { kLeft = -1, kRight = 1 };
-
 // Listens in on the browser event stream (as a pre target event handler) and
 // hides an associated hover card on any keypress.
 class TabHoverCardEventSniffer : public ui::EventHandler {
@@ -1754,12 +1752,12 @@ void TabStrip::CloseTab(Tab* tab, CloseTabSource source) {
   CloseTabInternal(GetModelIndexOf(tab), source);
 }
 
-void TabStrip::ShiftTabLeft(Tab* tab) {
-  ShiftTabRelative(tab, ShiftOffset::kLeft);
+void TabStrip::ShiftTabNext(Tab* tab) {
+  ShiftTabRelative(tab, 1);
 }
 
-void TabStrip::ShiftTabRight(Tab* tab) {
-  ShiftTabRelative(tab, ShiftOffset::kRight);
+void TabStrip::ShiftTabPrevious(Tab* tab) {
+  ShiftTabRelative(tab, -1);
 }
 
 void TabStrip::MoveTabFirst(Tab* tab) {
@@ -2997,7 +2995,7 @@ void TabStrip::UpdateContrastRatioValues() {
 }
 
 void TabStrip::ShiftTabRelative(Tab* tab, int offset) {
-  DCHECK(offset == ShiftOffset::kLeft || offset == ShiftOffset::kRight);
+  DCHECK_EQ(1, std::abs(offset));
   const int start_index = GetModelIndexOf(tab);
   int target_index = start_index + offset;
 
@@ -3038,7 +3036,7 @@ void TabStrip::ShiftTabRelative(Tab* tab, int offset) {
         if (IsValidModelIndex(candidate_index)) {
           target_index = candidate_index - offset;
         } else {
-          target_index = offset == ShiftOffset::kLeft ? 0 : GetModelCount() - 1;
+          target_index = offset < 0 ? 0 : GetModelCount() - 1;
         }
       } else {
         controller_->AddTabToGroup(start_index, target_group.value());
@@ -3051,7 +3049,7 @@ void TabStrip::ShiftTabRelative(Tab* tab, int offset) {
 
 void TabStrip::ShiftGroupRelative(const tab_groups::TabGroupId& group,
                                   int offset) {
-  DCHECK(offset == ShiftOffset::kLeft || offset == ShiftOffset::kRight);
+  DCHECK_EQ(1, std::abs(offset));
   std::vector<int> tabs_in_group = controller_->ListTabsInGroup(group);
 
   const int start_index = tabs_in_group.front();

chrome/browser/ui/views/tabs/tab_strip.h

@@ -281,8 +281,8 @@ class TabStrip : public views::View,
   void ToggleSelected(Tab* tab) override;
   void AddSelectionFromAnchorTo(Tab* tab) override;
   void CloseTab(Tab* tab, CloseTabSource source) override;
-  void ShiftTabLeft(Tab* tab) override;
-  void ShiftTabRight(Tab* tab) override;
+  void ShiftTabNext(Tab* tab) override;
+  void ShiftTabPrevious(Tab* tab) override;
   void MoveTabFirst(Tab* tab) override;
   void MoveTabLast(Tab* tab) override;
   void ShowContextMenuForTab(Tab* tab,

chrome/browser/ui/views/tabs/tab_strip_browsertest.cc

@@ -87,16 +87,16 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, MoveTabAndDeleteGroup) {
   EXPECT_EQ(groups[0], group);
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_Success) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabPrevious_Success) {
   AppendTab();
   AppendTab();
 
   const auto expected = GetWebContentsesInOrder({1, 0, 2});
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(1));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(1));
   EXPECT_EQ(expected, GetWebContentses());
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_AddsToGroup) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabPrevious_AddsToGroup) {
   AppendTab();
   AppendTab();
 
@@ -104,13 +104,13 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_AddsToGroup) {
 
   // Instead of moving, the tab should be added to the group.
   const auto expected = GetWebContentsesInOrder({0, 1, 2});
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(2));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(2));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(2)->group().value(), group);
 }
 
 IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
-                       ShiftTabLeft_PastCollapsedGroup_Success) {
+                       ShiftTabPrevious_PastCollapsedGroup_Success) {
   AppendTab();
   AppendTab();
 
@@ -121,7 +121,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   ASSERT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group));
 
   const auto expected = GetWebContentsesInOrder({2, 0, 1});
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(2));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(2));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group));
   EXPECT_EQ(tab_strip()->tab_at(0)->group(), base::nullopt);
@@ -130,7 +130,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
 }
 
 IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
-                       ShiftTabLeft_BetweenTwoCollapsedGroups_Success) {
+                       ShiftTabPrevious_BetweenTwoCollapsedGroups_Success) {
   AppendTab();
   AppendTab();
   AppendTab();
@@ -149,7 +149,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   ASSERT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group2));
 
   const auto expected = GetWebContentsesInOrder({0, 1, 4, 2, 3});
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(4));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(4));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group1));
   EXPECT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group2));
@@ -160,7 +160,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   EXPECT_EQ(tab_strip()->tab_at(4)->group().value(), group2);
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_RemovesFromGroup) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabPrevious_RemovesFromGroup) {
   AppendTab();
   AppendTab();
 
@@ -168,12 +168,13 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_RemovesFromGroup) {
 
   // Instead of moving, the tab should be removed from the group.
   const auto expected = GetWebContentsesInOrder({0, 1, 2});
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(1));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(1));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(1)->group(), base::nullopt);
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_ShiftsBetweenGroups) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
+                       ShiftTabPrevious_ShiftsBetweenGroups) {
   AppendTab();
   AppendTab();
 
@@ -183,46 +184,46 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_ShiftsBetweenGroups) {
   // Instead of moving, the tab should be removed from its old group, then added
   // to the new group.
   const auto expected = GetWebContentsesInOrder({0, 1, 2});
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(1));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(1));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(1)->group(), base::nullopt);
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(1));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(1));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(1)->group().value(), group);
 }
 
 IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
-                       ShiftTabLeft_Failure_EdgeOfTabstrip) {
+                       ShiftTabPrevious_Failure_EdgeOfTabstrip) {
   AppendTab();
   AppendTab();
 
   const auto contentses = GetWebContentses();
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(0));
   // No change expected.
   EXPECT_EQ(contentses, GetWebContentses());
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabLeft_Failure_Pinned) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabPrevious_Failure_Pinned) {
   AppendTab();
   AppendTab();
   tab_strip_model()->SetTabPinned(0, true);
 
   const auto contentses = GetWebContentses();
-  tab_strip()->ShiftTabLeft(tab_strip()->tab_at(1));
+  tab_strip()->ShiftTabPrevious(tab_strip()->tab_at(1));
   // No change expected.
   EXPECT_EQ(contentses, GetWebContentses());
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_Success) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabNext_Success) {
   AppendTab();
   AppendTab();
 
   const auto expected = GetWebContentsesInOrder({1, 0, 2});
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   EXPECT_EQ(expected, GetWebContentses());
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_AddsToGroup) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabNext_AddsToGroup) {
   AppendTab();
   AppendTab();
 
@@ -230,13 +231,13 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_AddsToGroup) {
 
   // Instead of moving, the tab should be added to the group.
   const auto expected = GetWebContentsesInOrder({0, 1, 2});
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(0)->group().value(), group);
 }
 
 IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
-                       ShiftTabRight_PastCollapsedGroup_Success) {
+                       ShiftTabNext_PastCollapsedGroup_Success) {
   AppendTab();
   AppendTab();
 
@@ -247,7 +248,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   ASSERT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group));
 
   const auto expected = GetWebContentsesInOrder({1, 2, 0});
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group));
   EXPECT_EQ(tab_strip()->tab_at(0)->group().value(), group);
@@ -256,7 +257,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
 }
 
 IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
-                       ShiftTabRight_BetweenTwoCollapsedGroups_Success) {
+                       ShiftTabNext_BetweenTwoCollapsedGroups_Success) {
   AppendTab();
   AppendTab();
   AppendTab();
@@ -275,7 +276,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   ASSERT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group2));
 
   const auto expected = GetWebContentsesInOrder({1, 2, 0, 3, 4});
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group1));
   EXPECT_TRUE(tab_strip()->controller()->IsGroupCollapsed(group2));
@@ -286,7 +287,7 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
   EXPECT_EQ(tab_strip()->tab_at(4)->group().value(), group2);
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_RemovesFromGroup) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabNext_RemovesFromGroup) {
   AppendTab();
   AppendTab();
 
@@ -294,12 +295,12 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_RemovesFromGroup) {
 
   // Instead of moving, the tab should be removed from the group.
   const auto expected = GetWebContentsesInOrder({0, 1, 2});
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(1));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(1));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(1)->group(), base::nullopt);
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_ShiftsBetweenGroups) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabNext_ShiftsBetweenGroups) {
   AppendTab();
   AppendTab();
 
@@ -309,32 +310,32 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_ShiftsBetweenGroups) {
   // Instead of moving, the tab should be removed from its old group, then added
   // to the new group.
   const auto expected = GetWebContentsesInOrder({0, 1, 2});
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(0)->group(), base::nullopt);
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   EXPECT_EQ(expected, GetWebContentses());
   EXPECT_EQ(tab_strip()->tab_at(0)->group().value(), group);
 }
 
 IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
-                       ShiftTabRight_Failure_EdgeOfTabstrip) {
+                       ShiftTabNext_Failure_EdgeOfTabstrip) {
   AppendTab();
   AppendTab();
 
   const auto contentses = GetWebContentses();
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(2));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(2));
   // No change expected.
   EXPECT_EQ(contentses, GetWebContentses());
 }
 
-IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabRight_Failure_Pinned) {
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftTabNext_Failure_Pinned) {
   AppendTab();
   AppendTab();
   tab_strip_model()->SetTabPinned(0, true);
 
   const auto contentses = GetWebContentses();
-  tab_strip()->ShiftTabRight(tab_strip()->tab_at(0));
+  tab_strip()->ShiftTabNext(tab_strip()->tab_at(0));
   // No change expected.
   EXPECT_EQ(contentses, GetWebContentses());
 }

chrome/browser/ui/views/tabs/tab_unittest.cc

@@ -61,8 +61,8 @@ class FakeTabController : public TabController {
   void ToggleSelected(Tab* tab) override {}
   void AddSelectionFromAnchorTo(Tab* tab) override {}
   void CloseTab(Tab* tab, CloseTabSource source) override {}
-  void ShiftTabRight(Tab* tab) override {}
-  void ShiftTabLeft(Tab* tab) override {}
+  void ShiftTabNext(Tab* tab) override {}
+  void ShiftTabPrevious(Tab* tab) override {}
   void MoveTabFirst(Tab* tab) override {}
   void MoveTabLast(Tab* tab) override {}
   void ShowContextMenuForTab(Tab* tab,