Ash: Extend tabs to top of window for maximized windows

Ash: Extend tabs to top of window for maximized windows

This makes them clickable when the cursor is at the edge of the screen.

For maximized windows we suppress the resize area that extends 1 pixel inside the window bounds, as maximized windows can't be resized (at least for now).  This allows double-clicking on the top edge of the screen to restore a maximized window and suppresses misleading resize cursors.

BUG=117952
TEST=manual, maximize a window and try single and double-clicking along the top screen edge.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126943

[James Cook, 2012-03-15 00:10:38]

Dave, PTAL.  This is a reimplementation of some code I think you wrote initially
that makes clicking at the top edge of the screen select tabs in a maximized
window.  (The code eventually ended up in compact_frame_view.cc, which I deleted
with the removal of compact window mode, see
http://codereview.chromium.org/9630002/diff/2002/chrome/browser/ui/views/fram...
)

Ben, I need OWNERS approval.

[Ben Goodger (Google), 2012-03-15 14:53:54]

So, for desktop Chrome we achieve this simply by laying out the tab at the
top pixel when maximized. Is there a reason not to do that here? This seems
more convoluted.

-Ben

On Wed, Mar 14, 2012 at 5:10 PM, <jamescook@chromium.org> wrote:

> Reviewers: DaveMoore, Ben Goodger (Google),
>
> Message:
> Dave, PTAL.  This is a reimplementation of some code I think you wrote
> initially
> that makes clicking at the top edge of the screen select tabs in a
> maximized
> window.  (The code eventually ended up in compact_frame_view.cc, which I
> deleted
> with the removal of compact window mode, see
> http://codereview.chromium.**org/9630002/diff/2002/chrome/**
>
browser/ui/views/frame/**compact_browser_frame_view.cc<http://codereview.chromium.org/9630002/diff/2002/chrome/browser/ui/views/frame/compact_browser_frame_view.cc>
> )
>
> Ben, I need OWNERS approval.
>
>
> Description:
> Ash: Increase clickable area of tabs for maximized windows
>
> When a browser window is maximized we use the area to the left of the
> first tab
> and the area above each tab to extend the clickable area of the tabs.  This
> makes them easier to hit.  We had a similar optimization for the non-Ash
> Chrome
> OS window frames.
>
> For maximized windows we suppress the resize area that extends 1 pixel
> inside
> the window bounds, as maximized windows can't be resized (at least for
> now).
> This allows double-clicking on the top edge of the screen to restore a
> maximized
> window and suppresses misleading resize cursors.
>
> BUG=117952
> TEST=manual, maximize a window and try single and double-clicking along
> the top
> screen edge.
>
>
> Please review this at
http://codereview.chromium.**org/9706044/<http://codereview.chromium.org/9706...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M ash/wm/frame_painter.cc
>  M chrome/browser/ui/views/frame/**browser_non_client_frame_view_**aura.h
>  M chrome/browser/ui/views/frame/**browser_non_client_frame_view_**aura.cc
>
>
> Index: ash/wm/frame_painter.cc
> diff --git a/ash/wm/frame_painter.cc b/ash/wm/frame_painter.cc
> index f61baad1ada3c10ee0fa60a62eea9e**64aef88d7b..**
> 47f72f4ea66f8e6afbdf548fd1fdf7**046f015e6e 100644
> --- a/ash/wm/frame_painter.cc
> +++ b/ash/wm/frame_painter.cc
> @@ -175,15 +175,18 @@ int
FramePainter::**NonClientHitTest(views::**NonClientFrameView*
> view,
>
>   // Check the frame first, as we allow a small area overlapping the
> contents
>   // to be used for resize handles.
> -  bool can_resize = frame_->widget_delegate() ?
> +  bool can_ever_resize = frame_->widget_delegate() ?
>       frame_->widget_delegate()->**CanResize() :
>       false;
> +  // Don't allow overlapping resize handles when the window is maximized,
> as it
> +  // can't be resized in that state.
> +  int resize_border = frame_->IsMaximized() ? 0 : kResizeInsideBoundsSize;
>   int frame_component = view->GetHTComponentForFrame(**point,
> -
> kResizeInsideBoundsSize,
> -
> kResizeInsideBoundsSize,
> +                                                     resize_border,
> +                                                     resize_border,
>                                                      kResizeAreaCornerSize,
>                                                      kResizeAreaCornerSize,
> -                                                     can_resize);
> +                                                     can_ever_resize);
>   if (frame_component != HTNOWHERE)
>     return frame_component;
>
> Index: chrome/browser/ui/views/frame/**browser_non_client_frame_view_**
> aura.cc
> diff --git
a/chrome/browser/ui/views/**frame/browser_non_client_**frame_view_aura.cc
> b/chrome/browser/ui/views/**frame/browser_non_client_**frame_view_aura.cc
> index 3bcca0c45d4f8014ff611326bba195**61dbe6f4d0..**
> a0a488c6dfc310b82cf79fd36905a2**dae7a94ad4 100644
> --- a/chrome/browser/ui/views/**frame/browser_non_client_**
> frame_view_aura.cc
> +++ b/chrome/browser/ui/views/**frame/browser_non_client_**
> frame_view_aura.cc
> @@ -14,6 +14,7 @@
>  #include "grit/theme_resources_**standard.h"
>  #include "grit/ui_resources.h"
>  #include "ui/base/accessibility/**accessible_view_state.h"
> +#include "ui/base/hit_test.h"
>  #include "ui/base/l10n/l10n_util.h"
>  #include "ui/base/resource/resource_**bundle.h"
>  #include "ui/base/theme_provider.h"
> @@ -31,8 +32,13 @@ const int kTabstripRightSpacing = 10;
>  // Space between top of window and top of tabstrip for restored windows.
>  const int kTabstripTopSpacingRestored = 10;
>  // Space between top of window and top of tabstrip for maximized windows.
> -const int kTabstripTopSpacingMaximized = 1;
> -
> +const int kTabstripTopSpacingMaximized = 2;
> +// Width of area to the left of first tab for which mouse events should be
> +// forwarded to the first tab for maximized windows.
> +const int kLeftEdgeForwardingSize = 15;
> +// Height of the area at the top of the window for which mouse events
> should be
> +// forwarded to the tab strip for maximized windows.
> +const int kTopEdgeForwardingSize = 4;
>  }  // namespace
>
>  //////////////////////////////**//////////////////////////////**
> ///////////////////
> @@ -80,9 +86,8 @@ gfx::Rect BrowserNonClientFrameViewAura:**
> :GetBoundsForTabStrip(
>     views::View* tabstrip) const {
>   if (!tabstrip)
>     return gfx::Rect();
> -  bool restored = !frame()->IsMaximized();
>   return gfx::Rect(**kTabstripLeftSpacing,
> -                   GetHorizontalTabStripVerticalO**ffset(restored),
> +                   GetHorizontalTabStripVerticalO**ffset(false),
>                    std::max(0, maximize_button_->x() -
> kTabstripRightSpacing),
>                    tabstrip->GetPreferredSize().**height());
>  }
> @@ -113,6 +118,11 @@ gfx::Rect BrowserNonClientFrameViewAura:**:**
> GetWindowBoundsForClientBounds**(
>  }
>
>  int BrowserNonClientFrameViewAura:**:NonClientHitTest(const gfx::Point&
> point) {
> +  // Some locations need to be forwarded to the tab strip via the browser
> view
> +  // and hence shouldn't be reported as non-client hits.
> +  gfx::Point browser_view_point;
> +  if (ForwardPointToBrowserView(**point, &browser_view_point))
> +    return HTNOWHERE;
>   return frame_painter_->**NonClientHitTest(this, point);
>  }
>
> @@ -157,6 +167,17 @@ void BrowserNonClientFrameViewAura:**:Layout() {
>   BrowserNonClientFrameView::**Layout();
>  }
>
> +views::View* BrowserNonClientFrameViewAura:**:GetEventHandlerForPoint(
> +    const gfx::Point& point) {
> +  // Some events need to be forwarded to the tab strip via the browser
> view
> +  // so we can increase the clickable area of the tabs when the window is
> +  // maximized (see Fitt's Law).
> +  gfx::Point browser_view_point;
> +  if (ForwardPointToBrowserView(**point, &browser_view_point))
> +    return browser_view()->**GetEventHandlerForPoint(**
> browser_view_point);
> +  return BrowserNonClientFrameView::**GetEventHandlerForPoint(point)**;
> +}
> +
>  bool BrowserNonClientFrameViewAura:**:HitTest(const gfx::Point& l) const
> {
>   // If the point is outside the bounds of the client area, claim it.
>   if (NonClientFrameView::HitTest(**l))
> @@ -351,3 +372,22 @@ SkBitmap* BrowserNonClientFrameViewAura:**
> :GetCustomBitmap(
>     return tp->GetBitmapNamed(bitmap_id);
>   return tp->GetBitmapNamed(fallback_**bitmap_id);
>  }
> +
> +bool BrowserNonClientFrameViewAura:**:ForwardPointToBrowserView(
> +    const gfx::Point& point,
> +    gfx::Point* browser_view_point) const {
> +  // Only forward when window is maximized.
> +  if (!frame()->IsMaximized())
> +    return false;
> +  // Ignore events outside our special forwarding area.
> +  if (!(point.x() < kLeftEdgeForwardingSize ||
> +        point.y() < kTopEdgeForwardingSize))
> +    return false;
> +  // Offset the point and convert to browser view coordinates.
> +  browser_view_point->SetPoint(**std::max(point.x(),
> kLeftEdgeForwardingSize),
> +                               std::max(point.y(),
> kTopEdgeForwardingSize));
> +  View::ConvertPointToView(this, browser_view(), browser_view_point);
> +  // We should forward it if it's going to hit something interesting and
> not
> +  // just hit the exposed caption area.
> +  return !browser_view()->**IsPositionInWindowCaption(***
> browser_view_point);
> +}
> Index: chrome/browser/ui/views/frame/**browser_non_client_frame_view_**
> aura.h
> diff --git
a/chrome/browser/ui/views/**frame/browser_non_client_**frame_view_aura.h
> b/chrome/browser/ui/views/**frame/browser_non_client_**frame_view_aura.h
> index a4347542d6197be872414fb1e2ced4**373e82fe88..**
> aaad38c6a5ea6cd0016e16f84fdba8**e19106b27b 100644
> --- a/chrome/browser/ui/views/**frame/browser_non_client_**
> frame_view_aura.h
> +++ b/chrome/browser/ui/views/**frame/browser_non_client_**
> frame_view_aura.h
> @@ -46,6 +46,8 @@ class BrowserNonClientFrameViewAura : public
> BrowserNonClientFrameView,
>   // views::View overrides:
>   virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE;
>   virtual void Layout() OVERRIDE;
> +  virtual views::View* GetEventHandlerForPoint(
> +      const gfx::Point& point) OVERRIDE;
>   virtual bool HitTest(const gfx::Point& l) const OVERRIDE;
>   virtual void GetAccessibleState(ui::**AccessibleViewState* state)
> OVERRIDE;
>   virtual gfx::Size GetMinimumSize() OVERRIDE;
> @@ -74,6 +76,13 @@ class BrowserNonClientFrameViewAura : public
> BrowserNonClientFrameView,
>   // theme image, or the bitmap for |fallback_bitmap_id| if not.
>   SkBitmap* GetCustomBitmap(int bitmap_id, int fallback_bitmap_id) const;
>
> +  // Returns true if we should forward events at a local |point| to the
> browser
> +  // view (and hence the tab strip) in order to increase the clickable
> area of
> +  // the tabs. Sets *|browser_view_point| to the forwarded point in
> browser view
> +  // coordinates.
> +  bool ForwardPointToBrowserView(**const gfx::Point& point,
> +                                 gfx::Point* browser_view_point) const;
> +
>   // Window controls.
>   views::ImageButton* maximize_button_;
>   views::ImageButton* close_button_;
>
>
>

[James Cook, 2012-03-15 15:49:00]

On 2012/03/15 14:53:54, Ben Goodger (Google) wrote:
> So, for desktop Chrome we achieve this simply by laying out the tab at the
> top pixel when maximized. Is there a reason not to do that here? This seems
> more convoluted.

The current visual design for maximized windows calls for some space between the
top of the window and the top of the tabs (as it did for Chrome OS in the past).
 Likewise, extending the first tab's clickable area all the way to the left edge
seems worthwhile to me as well.  Should I go forward with this or push back on
the designers to eliminate the space above the tabs?

[Ben Goodger (Google), 2012-03-15 15:50:38]

The left edge is something of a lost cause, especially once we have to
consider avatar icons.

I think we should match Windows here. Maximize means I want more screen
real estate. I don't want it taken up by dead pixels :-)

-Ben

On Thu, Mar 15, 2012 at 8:49 AM, <jamescook@chromium.org> wrote:

> On 2012/03/15 14:53:54, Ben Goodger (Google) wrote:
>
>> So, for desktop Chrome we achieve this simply by laying out the tab at the
>> top pixel when maximized. Is there a reason not to do that here? This
>> seems
>> more convoluted.
>>
>
> The current visual design for maximized windows calls for some space
> between the
> top of the window and the top of the tabs (as it did for Chrome OS in the
> past).
>  Likewise, extending the first tab's clickable area all the way to the
> left edge
> seems worthwhile to me as well.  Should I go forward with this or push
> back on
> the designers to eliminate the space above the tabs?
>
>
>
http://codereview.chromium.**org/9706044/<http://codereview.chromium.org/9706...
>

[DaveMoore, 2012-03-15 16:42:56]

The original reason for the padding was that maximize was our *only* mode. We
originally went all the way to the top and people felt it was too
claustrophobic. Now that it's an optional (not even the default) mode I don't
see why we don't go back to using all the pixels.

On 2012/03/15 15:50:38, Ben Goodger (Google) wrote:
> The left edge is something of a lost cause, especially once we have to
> consider avatar icons.
> 
> I think we should match Windows here. Maximize means I want more screen
> real estate. I don't want it taken up by dead pixels :-)
> 
> -Ben
> 
> On Thu, Mar 15, 2012 at 8:49 AM, <mailto:jamescook@chromium.org> wrote:
> 
> > On 2012/03/15 14:53:54, Ben Goodger (Google) wrote:
> >
> >> So, for desktop Chrome we achieve this simply by laying out the tab at the
> >> top pixel when maximized. Is there a reason not to do that here? This
> >> seems
> >> more convoluted.
> >>
> >
> > The current visual design for maximized windows calls for some space
> > between the
> > top of the window and the top of the tabs (as it did for Chrome OS in the
> > past).
> >  Likewise, extending the first tab's clickable area all the way to the
> > left edge
> > seems worthwhile to me as well.  Should I go forward with this or push
> > back on
> > the designers to eliminate the space above the tabs?
> >
> >
> >
>
http://codereview.chromium.**org/9706044/%3Chttp://codereview.chromium.org/97...>
> >

[James Cook, 2012-03-15 16:45:48]

OK, I'll make the tabs go to the top of the screen.  Update coming shortly. 
Thanks guys.

[James Cook, 2012-03-15 17:31:11]

Ben, PTAL.

-davemoore, since this patch is now simpler and doesn't depend on a
re-implementation of his code.

http://codereview.chromium.org/9706044/diff/5002/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_aura.cc
(right):

http://codereview.chromium.org/9706044/diff/5002/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_non_client_frame_view_aura.cc:87:
GetHorizontalTabStripVerticalOffset(false),
This function doesn't take the *current* restored state, it takes a
*force_restored* option that overrides the current state.  I fixed the parameter
name in an earlier CL, this is just cleanup.

[Ben Goodger (Google), 2012-03-15 18:00:03]

lgtm