UI adjustments for the HIDE button in the screen share notification window.

UI adjustments for the HIDE button in the screen share notification window.
The link is not underlined and there is 10px padding between the link and the right edge.

BUG=354893
NOTRY=true

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

[jiayl, 2014-03-21 18:15:11]

P1 for M34. PTAL. Thanks!

[jiayl, 2014-03-21 18:16:36]

rsesek: could you review the cocoa change?
ben: could you review the views change?

[Sergey Ulanov, 2014-03-21 18:54:10]

https://codereview.chromium.org/208643004/diff/1/chrome/browser/ui/views/scre...
File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right):

https://codereview.chromium.org/208643004/diff/1/chrome/browser/ui/views/scre...
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:33: const int
kPaddingVerticalHorizonal = 10;
kPaddingHorizontal?

[Robert Sesek, 2014-03-21 18:55:54]

lgtm

[jiayl, 2014-03-21 18:58:08]

https://codereview.chromium.org/208643004/diff/1/chrome/browser/ui/views/scre...
File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right):

https://codereview.chromium.org/208643004/diff/1/chrome/browser/ui/views/scre...
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:33: const int
kPaddingVerticalHorizonal = 10;
On 2014/03/21 18:54:10, Sergey Ulanov wrote:
> kPaddingHorizontal?

Oops. Done.

[jiayl, 2014-03-24 23:14:35]

msw: could you review ui/views?

[msw, 2014-03-25 00:01:57]

https://codereview.chromium.org/208643004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right):

https://codereview.chromium.org/208643004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:34: const
SkColor kHideLinkColor = SkColorSetRGB(0x55, 0x95, 0xFE);
It's wrong to use a hard-coded color over a themed color (in this case
ui::NativeTheme::kColorId_DialogBackground). What does this look like on a
high-contrast-black windows theme? I'll ping the bug and ask why this color
makes more sense than the conventional/themed link color.

[jily_google.com, 2014-03-27 22:10:57]

Jenn,

could you answer msw's question about the UI design?

#37 <https://code.google.com/p/webrtc/issues/detail?id=2789#c37>
msw@chromium.org <https://code.google.com/u/msw@chromium.org/>

jennschen - why must the link color use a custom color (#5595FE), and
not the conventional link color? It's bad practice to use hard-coded
colors over themed colors. The views (win/cros/linux-aura)
implementation uses ui::NativeTheme::kColorId_DialogBackground, which
may be inverted for high-contrast-black system themes*, do you know
that this color looks okay in that case? Is likely ugly either way,
but more likely to remain broken longer by introducing a
non-conventional hard coded color.
*
https://code.google.com/p/chromium/codesearch#chromium/src/ui/native_theme/na...



On Mon, Mar 24, 2014 at 5:01 PM, <msw@chromium.org> wrote:

>
> https://codereview.chromium.org/208643004/diff/20001/
> chrome/browser/ui/views/screen_capture_notification_ui_views.cc
> File chrome/browser/ui/views/screen_capture_notification_ui_views.cc
> (right):
>
> https://codereview.chromium.org/208643004/diff/20001/
> chrome/browser/ui/views/screen_capture_notification_ui_views.cc#newcode34
> chrome/browser/ui/views/screen_capture_notification_ui_views.cc:34:
>
> const SkColor kHideLinkColor = SkColorSetRGB(0x55, 0x95, 0xFE);
> It's wrong to use a hard-coded color over a themed color (in this case
> ui::NativeTheme::kColorId_DialogBackground). What does this look like on
> a high-contrast-black windows theme? I'll ping the bug and ask why this
> color makes more sense than the conventional/themed link color.
>
> https://codereview.chromium.org/208643004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jennschen_google.com, 2014-03-27 22:36:25]

Responded here: https://code.google.com/p/webrtc/issues/detail?id=2789#c39


On Thu, Mar 27, 2014 at 3:10 PM, Jiayang Liu <jily@google.com> wrote:

> Jenn,
>
> could you answer msw's question about the UI design?
>
> #37 <https://code.google.com/p/webrtc/issues/detail?id=2789#c37>
> msw@chromium.org <https://code.google.com/u/msw@chromium.org/>
>
> jennschen - why must the link color use a custom color (#5595FE), and not the
conventional link color? It's bad practice to use hard-coded colors over themed
colors. The views (win/cros/linux-aura) implementation uses
ui::NativeTheme::kColorId_DialogBackground, which may be inverted for
high-contrast-black system themes*, do you know that this color looks okay in
that case? Is likely ugly either way, but more likely to remain broken longer by
introducing a non-conventional hard coded color.
> *
https://code.google.com/p/chromium/codesearch#chromium/src/ui/native_theme/na...
>
>
>
> On Mon, Mar 24, 2014 at 5:01 PM, <msw@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/208643004/diff/20001/
>> chrome/browser/ui/views/screen_capture_notification_ui_views.cc
>> File chrome/browser/ui/views/screen_capture_notification_ui_views.cc
>> (right):
>>
>> https://codereview.chromium.org/208643004/diff/20001/
>> chrome/browser/ui/views/screen_capture_notification_ui_views.cc#newcode34
>> chrome/browser/ui/views/screen_capture_notification_ui_views.cc:34:
>>
>> const SkColor kHideLinkColor = SkColorSetRGB(0x55, 0x95, 0xFE);
>> It's wrong to use a hard-coded color over a themed color (in this case
>> ui::NativeTheme::kColorId_DialogBackground). What does this look like on
>> a high-contrast-black windows theme? I'll ping the bug and ask why this
>> color makes more sense than the conventional/themed link color.
>>
>> https://codereview.chromium.org/208643004/
>>
>
>


-- 
Jenn Chen
User Experience Designer | Google

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jiayl, 2014-03-27 23:03:03]

Reverted the color change. PTAL

[msw, 2014-03-28 02:15:22]

c/b/ui/views lgtm. It's odd to have a link without an underline, but I suppose
it's fine since that's what UX wants for this temporary solution. The unified
diff works, but the side-by-side diff is busted, you might need to re-upload.

[jiayl, 2014-03-28 16:01:01]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:02:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/208643004/100001

[commit-bot: I haz the power, 2014-03-28 19:14:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 19:14:53]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jiayl, 2014-03-28 19:18:48]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-03-28 19:20:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/208643004/100001

[commit-bot: I haz the power, 2014-03-28 19:20:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 19:20:51]

Failed to apply patch for
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/screen_capture_notification_ui_views.cc
  Hunk #1 FAILED at 33.
  Hunk #2 succeeded at 156 (offset 1 line).
  Hunk #3 succeeded at 281 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/screen_capture_notification_ui_views.cc.rej

Patch:       chrome/browser/ui/views/screen_capture_notification_ui_views.cc
Index: chrome/browser/ui/views/screen_capture_notification_ui_views.cc
diff --git a/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
b/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
index
280d4fa1c3c40696d5060112c5225c0af472cfde..8b42ab571c3b8c0258bc7b21bd23a06cf4471907
100644
--- a/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
+++ b/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
@@ -33,8 +33,8 @@ namespace {
 const int kMinimumWidth = 460;
 const int kMaximumWidth = 1000;
 const int kHorizontalMargin = 10;
-const int kPadding = 5;
-const int kPaddingLeft = 10;
+const int kPaddingVertical = 5;
+const int kPaddingHorizontal = 10;
 
 namespace {
 
@@ -155,6 +155,7 @@
ScreenCaptureNotificationUIViews::ScreenCaptureNotificationUIViews(
   hide_link_ = new views::Link(
       l10n_util::GetStringUTF16(IDS_PASSWORDS_PAGE_VIEW_HIDE_BUTTON));
   hide_link_->set_listener(this);
+  hide_link_->SetUnderline(false);
   AddChildView(hide_link_);
 }
 
@@ -279,7 +280,10 @@ views::NonClientFrameView*
 ScreenCaptureNotificationUIViews::CreateNonClientFrameView(
     views::Widget* widget) {
   views::BubbleFrameView* frame = new views::BubbleFrameView(
-      gfx::Insets(kPadding, kPaddingLeft, kPadding, kPadding));
+      gfx::Insets(kPaddingVertical,
+                  kPaddingHorizontal,
+                  kPaddingVertical,
+                  kPaddingHorizontal));
   SkColor color = widget->GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_DialogBackground);
   frame->SetBubbleBorder(scoped_ptr<views::BubbleBorder>(

[jiayl, 2014-03-28 19:30:58]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-03-28 19:33:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/208643004/120001

[commit-bot: I haz the power, 2014-03-28 21:38:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 21:38:24]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[jiayl, 2014-03-28 21:41:52]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-03-28 21:42:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/208643004/120001

[commit-bot: I haz the power, 2014-03-28 21:47:04]

Change committed as 260290