ntp4 info bubble

ntp4 info bubble

displays a bubble pointing at "Most Visited"; this is dismissed after 10 displays (try opening the ntp 11 times), or after the user clicks a navigation dot. See bug for more background.

BUG=92434
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96680

[Evan Stade, 2011-08-12 21:48:32]

+arv

[Rick Byers, 2011-08-12 21:54:00]

Looks great.  I have a couple minor suggestions and a clarification on the
intended UI design here..

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:116: * @type {!Element|undefined}
type doesn't match initialization to null before.  I suggest you just use this
type but leave it undefined to represent unset.

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/bubble.js (right):

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:13: */
Nice, I like this class.

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:62: show: function() {
add some protection against calling show twice (don't want, for example, to leak
the events).

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:103: e.preventDefault();
you really want to swallow all input on the document until the bubble is hidden?
 I thought the point of bubbles (as opposed to modal pop-ups) usually was to
overlay an otherwise functional UI?

[Evan Stade, 2011-08-12 22:35:13]

note that I plan to let Cole see it in practice, then he can tweak all the
color/font values, so there will undoubtedly be a follow up patch at some point
fixing the style.

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:116: * @type {!Element|undefined}
On 2011/08/12 21:54:00, Rick Byers wrote:
> type doesn't match initialization to null before.  I suggest you just use this
> type but leave it undefined to represent unset.

Done.

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/bubble.js (right):

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:13: */
On 2011/08/12 21:54:00, Rick Byers wrote:
> Nice, I like this class.

side note: the alternative I have heard is used by many to create a triangle
shape is to create a very thick border on one side (border width = half of div
width). The borders on the 4 sides don't overlap, so each border will end up as
a triangle. Needless to say, this is ghastly. And it also doesn't offer much
flexibility, e.g. you couldn't add a drop shadow or border. html5 transforms to
the rescue.

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:62: show: function() {
On 2011/08/12 21:54:00, Rick Byers wrote:
> add some protection against calling show twice (don't want, for example, to
leak
> the events).

done (also improved attach)

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:103: e.preventDefault();
On 2011/08/12 21:54:00, Rick Byers wrote:
>  I thought the point of bubbles (as opposed to modal pop-ups) usually was to
> overlay an otherwise functional UI?

I guess this is up to one of the UI leads; I could see them wanting to leave the
bubble up until the user explicity dismisses it. But this mirrors the
functionality of chrome bubbles, which are app modal (actually they are desktop
modal but I think app modal is preferable in the webpage bubble case). I think
it's important to match what we've already taught users, because a user should
be able to accurately predict the behavior of a UI element based on its
appearance.

[Rick Byers, 2011-08-12 23:46:08]

LGTM

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/bubble.js (right):

http://codereview.chromium.org/7461160/diff/3013/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/bubble.js:103: e.preventDefault();
On 2011/08/12 22:35:13, Evan Stade wrote:
> On 2011/08/12 21:54:00, Rick Byers wrote:
> >  I thought the point of bubbles (as opposed to modal pop-ups) usually was to
> > overlay an otherwise functional UI?
> 
> I guess this is up to one of the UI leads; I could see them wanting to leave
the
> bubble up until the user explicity dismisses it. But this mirrors the
> functionality of chrome bubbles, which are app modal (actually they are
desktop
> modal but I think app modal is preferable in the webpage bubble case). I think
> it's important to match what we've already taught users, because a user should
> be able to accurately predict the behavior of a UI element based on its
> appearance.

I agree this should match behavior of other bubbles.  I was thinking of the
infobars when I was thinking it shouldn't be modal - sorry.  I also misread the
mousedown code - was thinking it disappeared only when clicked, when in fact it
disappears only when somewhere else on the document ins clicked.  That's much
better.  Sorry for the confusion.

[arv (Not doing code reviews), 2011-08-13 00:44:01]

LGTM

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:201:
infoBubble.setText(localStrings.getString('ntp4_intro_message'));
Why not use a setter?

infoBubble.text = localStrings.getString('ntp4_intro_message');

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/bubble.js (right):

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:33: setText: function(text) {
set text(text) {
  this.querySelector('.bubble-contents').textContent = text;
},

get text() {
  return this.querySelector('.bubble-contents').textContent;
}

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:38: * Sets the anchor node,
i.e. the node that this bubble points at.
maybe change this to a property then?

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:91: * Handles keywdown and
mousedown events, dismissing the bubble if
typo

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:98: if (e.keyCode == 27 /*
Esc */)
or

if (e.keyCode == 27)  // Esc

[Evan Stade, 2011-08-13 01:25:42]

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/bubble.js (right):

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:38: * Sets the anchor node,
i.e. the node that this bubble points at.
On 2011/08/13 00:44:01, arv wrote:
> maybe change this to a property then?

Done.

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:91: * Handles keywdown and
mousedown events, dismissing the bubble if
On 2011/08/13 00:44:01, arv wrote:
> typo

Done.

http://codereview.chromium.org/7461160/diff/11001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/bubble.js:98: if (e.keyCode == 27 /*
Esc */)
On 2011/08/13 00:44:01, arv wrote:
> or
> 
> if (e.keyCode == 27)  // Esc

Done.

[commit-bot: I haz the power, 2011-08-13 04:29:48]

Change committed as 96680