Polish when tab separators appear

Fixed two issues noticed while working on Tab Groups:
1) The first tab does not have a leading separator even when a group header is to its left. It should have a separator in this case.
2) The last tab in a selection has a trailing separator if it's A) the last visible tab and B) not the active tab. It shouldn't have a separator in this case.

This fix produces the correct results for #1 and #2 while still respecting the fade in/out of the separators on hover.

I was not able to find any existing unit tests for the separator. I looked into writing one, but the separators are just painted on using canvas->DrawRect(). Let me know if a screencast would help, or I can demonstrate the change locally.

Bug: 1022100
Change-Id: Id5ad6c7e03ee00292a63b8f2350ceb84e448c05d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899887
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714571}

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

@@ -75,6 +75,24 @@ class GM2TabStyle : public TabStyleViews {
   // effects and consider only the current tab's state.
   SeparatorOpacities GetSeparatorOpacities(bool for_layout) const;
 
+  // Returns a single separator's opacity based on whether it is the
+  // logically |leading| separator. |for_layout| has the same meaning as in
+  // GetSeparatorOpacities().
+  float GetSeparatorOpacity(bool for_layout, bool leading) const;
+
+  // Helper that returns an interpolated opacity if the tab or its neighbor
+  // |other_tab| is mid-hover-animation. Used in almost all cases when a
+  // separator is shown, since hovering is independent of tab state.
+  // |for_layout| has the same meaning as in GetSeparatorOpacities().
+  float GetHoverInterpolatedSeparatorOpacity(bool for_layout,
+                                             const Tab* other_tab) const;
+
+  // Helper that returns an interpolated opacity if the tab is
+  // mid-bounds-animation. Used only for the first and last tabs, since those
+  // are the primary cases where separator opacity is likely to change during
+  // a bounds animation.
+  float GetBoundsInterpolatedSeparatorOpacity() const;
+
   // Returns whether we shoould extend the hit test region for Fitts' Law.
   bool ShouldExtendHitTest() const;
 
@@ -536,78 +554,136 @@ TabStyle::SeparatorBounds GM2TabStyle::GetSeparatorBounds(float scale) const {
 
 TabStyle::SeparatorOpacities GM2TabStyle::GetSeparatorOpacities(
     bool for_layout) const {
-  // Something should visually separate tabs from each other and any adjacent
-  // new tab button.  Normally, active and hovered tabs draw distinct shapes
-  // (via different background colors) and thus need no separators, while
-  // background tabs need separators between them.
-  float leading_opacity, trailing_opacity;
-  if (tab_->IsActive()) {
-    leading_opacity = trailing_opacity = 0;
-  } else {
-    const Tab* subsequent_tab = tab_->controller()->GetAdjacentTab(tab_, 1);
-    const Tab* previous_tab = tab_->controller()->GetAdjacentTab(tab_, -1);
+  // Adjacent slots should be visually separated from each other. This can be
+  // achieved in multiple ways:
+  //   - Contrasting background colors for tabs, due to:
+  //       - Active state
+  //       - Selected state
+  //       - Hovered state
+  //       - Theming (affected by all the above, plus the neutral state)
+  //   - Manually painting a separator.
+  // The separator should be the last resort, if none of the above states
+  // apply. It's also needed if multiple adjacent views are selected, in which
+  // case the uniform selected color does not provide enough contrast.
+  // In addition, separators should smoothly fade in and out between states,
+  // particularly during the hover animation.
+
+  float leading_opacity = GetSeparatorOpacity(for_layout, true);
+  float trailing_opacity = GetSeparatorOpacity(for_layout, false);
+
+  // Return the opacities in physical order, rather than logical.
+  if (base::i18n::IsRTL())
+    std::swap(leading_opacity, trailing_opacity);
+  return {leading_opacity, trailing_opacity};
+}
+
+float GM2TabStyle::GetSeparatorOpacity(bool for_layout, bool leading) const {
+  // If the current tab is active, never show the separator.
+  if (tab_->IsActive())
+    return 0.0f;
+
+  const Tab* adjacent_tab =
+      tab_->controller()->GetAdjacentTab(tab_, leading ? -1 : 1);
+
+  const Tab* left_tab = leading ? adjacent_tab : tab_;
+  const Tab* right_tab = leading ? tab_ : adjacent_tab;
+  const bool adjacent_to_header =
+      right_tab && right_tab->group().has_value() &&
+      (!left_tab || left_tab->group() != right_tab->group());
+
+  // If the current tab is selected, default to hiding the separator. Only show
+  // the separator if it's adjacent to other selected tabs.
+  if (tab_->IsSelected()) {
+    // If the adjacent view is actually a group header, hide the separator since
+    // group headers currently cannot be selected.
+    // TODO(crbug.com/1017822): Update this if headers become selectable.
+    if (adjacent_to_header)
+      return 0.0f;
+
+    if (adjacent_tab && adjacent_tab->IsSelected())
+      return GetHoverInterpolatedSeparatorOpacity(for_layout, adjacent_tab);
+
+    return 0.0f;
+  }
+
+  // Otherwise, default to showing the separator, respecting the hover
+  // animation. Only hide the separator if it's in the first slot, or in
+  // certain cases if the tab has a visible background (see below).
+
+  // If the adjacent view is actually a group header, show the separator since
+  // the group header takes up a slot.
+  // TODO(crbug.com/1017822): Update this if headers become selectable.
+  if (adjacent_to_header)
+    return GetHoverInterpolatedSeparatorOpacity(for_layout, nullptr);
+
+  // If the tab has a visible background even when not selected or active, there
+  // are additional cases where the separators can be hidden.
+  if (tab_->controller()->HasVisibleBackgroundTabShapes()) {
+    // If the tab with a visible background is in an end slot, hide the
+    // separator because it doesn't need additional contrast with the tab strip
+    // or the new tab button. This value isn't interpolated like the others
+    // because the separator was likely already hidden: if it's animating into
+    // an end slot, then the tab was probably next to a selected dragging tab
+    // (see the condition below).
+    if (!adjacent_tab)
+      return 0.0f;
+
+    // If the adjacent tab is selected, any separator on the current tab will be
+    // "hidden" beneath the adjacent tab's background. Normally tabs will still
+    // have a separator, in case the adjacent tab is dragged away and it reveals
+    // an empty gap. However, tabs with visible backgrounds already have
+    // sufficient contrast against the empty gap, so this contingency isn't
+    // needed. Therefore, the separator is hidden only for tabs with visible
+    // backgrounds.
+    // TODO(crbug.com/876599): This value should be interpolated because the
+    // separator may be going from shown (the default) to hidden (when animating
+    // past an empty gap like this). This should behave similarly to
+    // GetBoundsInterpolatedSeparatorOpacity(), but not just for the end slots.
+    if (adjacent_tab->IsSelected())
+      return 0.0f;
+  }
+
+  // If the tab does not have a visible background and is in the first slot,
+  // make sure the opacity is interpolated correctly when it animates into
+  // position, since the separator is likely going from shown (the default) to
+  // hidden (in the first slot). See GetBoundsInterpolatedSeparatorOpacity().
+  if (!adjacent_tab && leading)
+    return GetBoundsInterpolatedSeparatorOpacity();
 
+  return GetHoverInterpolatedSeparatorOpacity(for_layout, adjacent_tab);
+}
+
+float GM2TabStyle::GetHoverInterpolatedSeparatorOpacity(
+    bool for_layout,
+    const Tab* other_tab) const {
   // Fade out the intervening separator while this tab or an adjacent tab is
   // hovered, which prevents sudden opacity changes when scrubbing the mouse
   // across the tabstrip. If that adjacent tab is active, don't consider its
   // hover animation value, otherwise the separator on this tab will disappear
   // while that tab is being dragged.
-    auto adjacent_hover_value = [for_layout](const Tab* tab) {
-      if (for_layout || !tab || tab->IsActive())
-        return 0.f;
-      auto* tab_style = static_cast<const GM2TabStyle*>(tab->tab_style());
+  auto adjacent_hover_value = [for_layout](const Tab* other_tab) {
+    if (for_layout || !other_tab || other_tab->IsActive())
+      return 0.0f;
+    auto* tab_style = static_cast<const GM2TabStyle*>(other_tab->tab_style());
     return float{tab_style->GetHoverAnimationValue()};
   };
   const float hover_value = GetHoverAnimationValue();
-    trailing_opacity =
-        1.f - std::max(hover_value, adjacent_hover_value(subsequent_tab));
-    leading_opacity =
-        1.f - std::max(hover_value, adjacent_hover_value(previous_tab));
-
-    if (tab_->IsSelected()) {
-      // Since this tab is selected, its shape will be visible against adjacent
-      // unselected tabs, so remove the separator in those cases.
-      if (previous_tab && !previous_tab->IsSelected())
-        leading_opacity = 0;
-      if (subsequent_tab && !subsequent_tab->IsSelected())
-        trailing_opacity = 0;
-    } else if (tab_->controller()->HasVisibleBackgroundTabShapes()) {
-      // Since this tab is unselected, adjacent selected tabs will normally
-      // paint atop it, covering the separator.  But if the user drags those
-      // selected tabs away, the exposed region looks like the window frame; and
-      // since background tab shapes are visible, there should be no separator.
-      // TODO(pkasting): https://crbug.com/876599  When a tab is animating
-      // into this gap, we should adjust its separator opacities as well.
-      if (previous_tab && previous_tab->IsSelected())
-        leading_opacity = 0;
-      if (subsequent_tab && subsequent_tab->IsSelected())
-        trailing_opacity = 0;
-    }
-  }
+  return 1.0f - std::max(hover_value, adjacent_hover_value(other_tab));
+}
 
-  // For the first or (when tab shapes are visible) last tab in the strip, fade
-  // the leading or trailing separator based on how close to the target bounds
-  // this tab is.  In the steady state, this hides the leading separator; it
-  // fades out the separators as tabs animate into these positions, after they
-  // pass by the other tabs; and it snaps the separators to full visibility
-  // immediately when animating away from these positions, which seems
-  // desirable.
+float GM2TabStyle::GetBoundsInterpolatedSeparatorOpacity() const {
+  // When the bounds of a tab are animating, fade the separator based on how
+  // close to the target bounds this tab is. This function is only called
+  // when the target bounds are an end slot. That means this function will fade
+  // the separators in or out as a tab animtes into the end slot, but it will
+  // not be called if the tab is animating out of the end slot. In that case,
+  // the separator will snap to full opacity immediately, which is visually
+  // consistent with other bounds animations.
   const gfx::Rect target_bounds =
       tab_->controller()->GetTabAnimationTargetBounds(tab_);
   const int tab_width = std::max(tab_->width(), target_bounds.width());
-  const float target_opacity =
-      float{std::min(std::abs(tab_->x() - target_bounds.x()), tab_width)} /
+  return float{std::min(std::abs(tab_->x() - target_bounds.x()), tab_width)} /
          tab_width;
-  if (tab_->controller()->IsFirstVisibleTab(tab_))
-    leading_opacity = target_opacity;
-  if (tab_->controller()->IsLastVisibleTab(tab_) &&
-      tab_->controller()->HasVisibleBackgroundTabShapes())
-    trailing_opacity = target_opacity;
-
-  // Return the opacities in physical order, rather than logical.
-  if (base::i18n::IsRTL())
-    std::swap(leading_opacity, trailing_opacity);
-  return {leading_opacity, trailing_opacity};
 }
 
 bool GM2TabStyle::ShouldExtendHitTest() const {