Fullscreen bubble: Draw a white box around "ESC" in "Press ESC to exit".

chrome/browser/ui/views/exclusive_access_bubble_views.cc

Index: chrome/browser/ui/views/exclusive_access_bubble_views.cc
diff --git a/chrome/browser/ui/views/exclusive_access_bubble_views.cc b/chrome/browser/ui/views/exclusive_access_bubble_views.cc
index 3bebd07b47585b5083de16085b434a59d3935273..4623dead17773bbc85adc6cf47faddd71b3a5bfb 100644
--- a/chrome/browser/ui/views/exclusive_access_bubble_views.cc
+++ b/chrome/browser/ui/views/exclusive_access_bubble_views.cc
@@ -4,7 +4,9 @@
 
 #include "chrome/browser/ui/views/exclusive_access_bubble_views.h"
 
+#include "base/i18n/case_conversion.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -44,6 +46,11 @@ namespace {
 // Space between the site info label and the buttons / link.
 const int kMiddlePaddingPx = 30;
 
+// Spacing around the escape key name.
+const int kKeyNameMarginHorizPx = 7;
+const int kKeyNameMarginVertPx = 0;
+const int kKeyNamePaddingPx = 7;
+
 // Opacity of the background (out of 255). Only used with
 // IsSimplifiedFullscreenUIEnabled.
 const unsigned char kBackgroundOpacity = 180;
@@ -85,6 +92,73 @@ ButtonView::ButtonView(views::ButtonListener* listener,
 ButtonView::~ButtonView() {
 }
 
+// Class containing the exit instruction text. Contains fancy styling on the
+// keyboard key (not just a simple label).
+class InstructionView : public views::View {
+ public:
+  // Creates an InstructionView with specific text. |text| may contain a single
+  // segment delimited by a pair of pipes ('|'); this segment will be displayed
+  // as a keyboard key. e.g., "Press |Esc| to exit" will have "Esc" rendered as
+  // a key.
+  InstructionView(const base::string16& text,
+                  const gfx::FontList& font_list,
+                  SkColor foreground_color,
+                  SkColor background_color);

[Evan Stade, 2015/10/29 00:49:53]

I don't think you need to pass background_color, just turn off auto color
readability on all the labels. It would be an error to have a situation where fg
and bg (whether based on the theme or hardcoded) were not readable relative to
each other.

[Matt Giuca, 2015/11/09 06:35:12]

Not sure what the problem is here or what that change will solve (are you just
trying to simplify the code?). Won't your suggestion *cause* potential contrast
issues?

Passing background color has two desirable effects:
1. Getting the subpixel rendering to look right for the particular background
color, or disabling subpixel if the background is partially or fully
transparent.
2. Inverting the text color if the contrast ratio is too low.

For both of those, we should be setting auto color readability (which is on by
default), AND setting the correct background color.

Your suggestion would a) mean that the label is subpixel-rendered for the
default theme background (that will "just happen" to be correct for when the
flag is off, but not when the background is transparent), and b) will not
correct for bad contrast in the system theme. So I'm not sure why you want this.
It would not be an error; that is valid input to this function (if the user's
system theme has a bad contrast ratio).

(Note: this behaviour isn't new --- it's just been refactored from the code in
ExclusiveAccessView's ctor --- so any change to the way this is done also needs
to be reflected there.)

+
+ private:
+  views::Label* before_key_;

[Evan Stade, 2015/10/29 00:49:53]

are these members necessary? you only use them in the ctor and never reference
them again

[Matt Giuca, 2015/11/09 06:35:12]

Done.

+  views::View* key_name_;
+  views::Label* after_key_;

[Evan Stade, 2015/10/29 00:49:53]

DISALLOW_COPY_AND_ASSIGN

[Matt Giuca, 2015/11/09 06:35:12]

Done.

+};
+
+InstructionView::InstructionView(const base::string16& text,
+                                 const gfx::FontList& font_list,
+                                 SkColor foreground_color,
+                                 SkColor background_color)
+    : before_key_(nullptr), key_name_(nullptr), after_key_(nullptr) {
+  // Parse |text|, looking for pipe-delimited segment.
+  std::vector<base::string16> segments =

[Evan Stade, 2015/11/04 00:37:20]

technically the style guide says not to call virtual fns in a constructor
("Avoid virtual method calls in constructors"). We break this rule a lot in
existing code but I think it's not too difficult to follow in this case (just
move everything here to an Init function).

[Matt Giuca, 2015/11/09 06:35:12]

Which virtual method am I calling? I can't find any calls to virtual methods of
views::View anywhere in this function.

Instead of an Init method, I'd rather just remove the InstructionView class and
make a function that returns a new View (since it no longer has any fields). But
I'd only do that if there were virtual method calls here...

[Evan Stade, 2015/11/09 18:51:24]

ah, very true, I guess I was thinking AddChildView and SetLayoutManager were
virtual.

[Matt Giuca, 2015/11/12 01:03:46]

Acknowledged.

+      base::SplitString(text, base::ASCIIToUTF16("|"), base::TRIM_WHITESPACE,
+                        base::SPLIT_WANT_ALL);
+  // Expect 1 or 3 pieces (either no pipe-delimited segments, or one).
+  DCHECK(segments.size() == 1 || segments.size() == 3);
+
+  bool has_key = segments.size() == 3;
+  int margin_vert = has_key ? kKeyNameMarginVertPx : 0;

[msw, 2015/10/29 17:50:02]

kKeyNameMarginVertPx is also zero, perhaps |margin_vert| isn't needed?

[Matt Giuca, 2015/11/09 06:35:12]

I think it makes sense to keep this (otherwise it is equivalent to hard-coding a
constant of 0; if anyone wants to change it to nonzero in the future it is extra
work). I just set it to 0 for the time being. This UX is in flux and I may need
to change the margins. I don't like the idea of having a configurable horizontal
margin but not vertical.

[msw, 2015/11/09 18:28:47]

The 'configurable horizontal margin' you mention is the |between_child_spacing|,
where it makes sense to use a constant for the non-zero value; although it's not
aptly named as |kKeyNameMarginHorizPx| and I'd actually scope all those
constants to this function, since they're only used here.

You pass a constant of 0 for the |inside_border_horizontal_spacing|, so why not
the same for |inside_border_vertical_spacing|? Instead, you have a constant of
zero and then a ternary statement switching between constant 0 and literal 0 on
a |has_key| conditional, which doesn't have any tangible value, please simplify.

[Matt Giuca, 2015/11/12 01:03:46]

The naming is correct, from thinking about it as CSS. I am defining the
horizontal and vertical margins around the KeyName (which is the middle of 3
elements in the BoxLayout).

Again, because Views doesn't have a concept of padding or margin, I define the
constants in a semantically correct way (kKeyNameMarginHorizPx and
kKeyNameMarginVertPx) and then implement as best I can. In this case, I
implement horizontal margin around the middle element of BoxLayout by setting
between_child_spacing to the key name's horizontal margin.
Done.
I implemented the vertical margin by setting the
|inside_border_vertical_spacing| of the BoxLayout to the key name's vertical
margin. Except that, because there is also a padding around the entire bubble,
the key name's vertical margin is 0. So I think the way I had it made sense (and
allows you to customize both the horiz and vert margins), but as you say, it
isn't strictly necessary, so I'm taking out the vertical margin.

+  views::BoxLayout* layout = new views::BoxLayout(
+      views::BoxLayout::kHorizontal, 0, margin_vert, kKeyNameMarginHorizPx);
+  SetLayoutManager(layout);
+  layout->set_collapse_when_hidden(true);
+
+  before_key_ = new views::Label(segments[0], font_list);
+  before_key_->SetEnabledColor(foreground_color);
+  before_key_->SetBackgroundColor(background_color);
+  AddChildView(before_key_);
+
+  if (has_key) {
+    base::string16 key = base::i18n::ToUpper(segments[1]);

[Evan Stade, 2015/10/29 00:49:53]

why is this ToUpper instead of making the source string all upper case? It's
probably also worth pinging sebastien as to why his mock for the tab key is
"Tab" and the mock for the escape key is "ESC". Seems inconsistent.

[Evan Stade, 2015/10/29 00:58:36]

Talked to Sebastien, he says to use "Esc" (he doesn't think it matters much that
on OSX "esc" might be more natural).

[Matt Giuca, 2015/10/29 22:04:24]

Separation of presentation from content (as you would do in CSS). If we decide
on a different policy decision, we don't want to have to update and re-translate
all the strings.

(I still haven't made up my mind about whether to re-use the existing key names
or hard-code "Esc" into the string, but as the code currently does the former,
that would require having separate uppercase versions of key name strings.)
And confirmed on private thread that we'll go with "Esc".

+    views::Label* key_name_label = new views::Label(key, font_list);
+    key_name_label->SetEnabledColor(foreground_color);
+    key_name_label->SetBackgroundColor(background_color);
+
+    key_name_ = new views::View;
+    views::BoxLayout* key_name_layout = new views::BoxLayout(
+        views::BoxLayout::kHorizontal, kKeyNamePaddingPx, kKeyNamePaddingPx, 0);
+    key_name_->SetLayoutManager(key_name_layout);

[msw, 2015/10/29 17:50:02]

Why do you need a separate layout manager for the single |key_name_label| view?
It seems like you should be able to adjust |layout|'s ctor spacing args to
achieve what you want. (or tweak |key_name_label|'s insets/min-size/border?)

[Matt Giuca, 2015/11/09 06:35:12]

Essentially what I want (in HTML/CSS terms) is for key_name_layout to have a 2px
border and 7px padding.

Since Views doesn't have a concept of padding, this is the only way I could
think of (without creating a custom view class) of getting the 7px padding
*inside* the border.

If I change the parent layout spacing, it will change the *margins* of this
label, not the *padding*.
Another alternative (which I considered) is that we change the Border class (in
this case, RoundedRectBorder) to have a concept of padding (so it produces
bigger insets than what it actually draws). But it doesn't seem right to add
that to RoundedRectBorder without adding it to all the other Border classes. And
in general, it seems like a concept that should be added to View, not to Border.

Given that Views doesn't currently have a concept of padding, this seemed like
the cleanest way to do it.

[msw, 2015/11/09 18:28:47]

Views have insets, which amount to padding, but they're not easily configured
for instances. There may be a better way, but I guess this is fine for now.

[Matt Giuca, 2015/11/12 01:03:45]

Acknowledged.

+    key_name_->AddChildView(key_name_label);
+    // The key name has a border around it.
+    scoped_ptr<views::Border> border(
+        views::Border::CreateRoundedRectBorder(2, 2, foreground_color));

[Evan Stade, 2015/10/29 00:49:53]

Interesting how different this is to a very similar UI I just implemented in
https://codereview.chromium.org/1416683003/

Perhaps the difference is warranted, because there is less of a vertical space
constraint here. In the omnibox, we really need the key to stay a particular
height, but here there may be more room to grow based on the string.

[Matt Giuca, 2015/10/29 06:41:01]

Do you think you could use CreateRoundedRectBorder? Or should I not be creating
this?

(Note: CreateRoundedRectBorder is something I am creating specifically for this,
but putting into Border because it seems generic. Hasn't landed yet:
https://codereview.chromium.org/1428623005/ -- comment there if you have an
opinion.)

[Evan Stade, 2015/10/29 18:36:41]

I think I could use it, but it doesn't buy much (CreateRoundedRectBorder is
hardlier easier than canvas->DrawRoundRect). I would just as soon put it at the
top of this file for now until we were sure someone else wanted it (it's not
particularly likely that if this code changed someone would realize there are no
other uses of CreateRoundedRectBorder and delete it). Jon Ross is in the midst
of creating something similar (but different) which he's putting in
c/b/u/v/location_bar, not u/v/border.cc

[Evan Stade, 2015/10/29 21:11:30]

Actually I take it back, I couldn't use this border class because the exact
details of the border will be different for these two keys (I'll be reusing
jonross's border).

[Evan Stade, 2015/11/04 00:37:20]

are you sure this thickness matches the spec? I think it's supposed to be 1dp,
i.e. 1px and 1x and 2px at 2x. Mock was probably provided in 2x.

[Matt Giuca, 2015/11/09 06:35:12]

Maybe... I tried with 1px and 2px and 1px just looks weird (it draws 2px wide
but at half opacity; I think it's some anti-aliasing thing).

I'll look into that but for now, I'm not trying to match the spec, but just bang
this into a basic shape resembling what people are asking for. (The text string
and font size is also wrong.)

Also I'm still confused about which spec I should be following --- Sebastien's
spec includes the word "fullscreen" whereas the previously agreed upon text does
not. Let's just get this in and then decide on specifics.

I did move this to a constant.

[Evan Stade, 2015/11/09 18:51:25]

see my comment in the other CL (which imo should be folded into this CL since
it's so closely related)

[Matt Giuca, 2015/11/12 01:03:46]

OK I've fixed the underlying issue in the other CL (with anti-aliasing). But I
tried again with 1px and now it just looks too thin compared to the text size.
So I'll stick with 2px here; it most closely matches the mocks visually (despite
not being the same numbers expressed in the mocks).

Again, we can iterate on the specifics later.

[Evan Stade, 2015/11/12 01:27:37]

I don't think it does match the mocks visually. The mock you're looking at is
probably the 2x mock and you're looking at it in 1x.

I vote for matching the mocks and then getting design feedback from there.
Either way, lgtm

+    key_name_->SetBorder(border.Pass());
+    AddChildView(key_name_);
+
+    after_key_ = new views::Label(segments[2], font_list);
+    after_key_->SetEnabledColor(foreground_color);
+    after_key_->SetBackgroundColor(background_color);
+    AddChildView(after_key_);
+  }
+}
+
 gfx::Size ButtonView::GetPreferredSize() const {
   return visible() ? views::View::GetPreferredSize() : gfx::Size();
 }
@@ -123,7 +197,7 @@ class ExclusiveAccessBubbleViews::ExclusiveAccessView
   ButtonView* button_view_;
   // Instruction for exiting fullscreen / mouse lock. Only present if there is
   // no link or button (always present in simplified mode).
-  views::Label* exit_instruction_;
+  InstructionView* exit_instruction_;
   const base::string16 browser_fullscreen_exit_accelerator_;
 
   DISALLOW_COPY_AND_ASSIGN(ExclusiveAccessView);
@@ -177,11 +251,8 @@ ExclusiveAccessBubbleViews::ExclusiveAccessView::ExclusiveAccessView(
   }
 
   exit_instruction_ =
-      new views::Label(bubble_->GetInstructionText(), medium_font_list);
-  exit_instruction_->set_collapse_when_hidden(true);
-
-  exit_instruction_->SetEnabledColor(foreground_color);
-  exit_instruction_->SetBackgroundColor(background_color);
+      new InstructionView(bubble_->GetInstructionText(), medium_font_list,
+                          foreground_color, background_color);
 
   link_ = new views::Link();
   link_->set_collapse_when_hidden(true);