Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8208014:
Update the notification bubble. (Closed)
|
Created:
9 years, 2 months ago by Finnur Modified:
9 years, 1 month ago CC:
chromium-reviews, estade+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionUpdate the notification bubble on the new tab page, as per UI mocks.
This introduces a new ExpandableBubble, to accomodate this. It is
collapsed when shown and can be set to not dismiss on blur. Clicking
on the bubble expands the bubble.
Note: Still uses the old-style arrow.
BUG=88067
TEST=Notification bubble should look different and function differently.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108449
Patch Set 1 #
Total comments: 29
Patch Set 2 : Comments addressed #
Total comments: 15
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 4
Patch Set 7 : '' #
Total comments: 6
Patch Set 8 : '' #
Messages
Total messages: 30 (0 generated)
ping
sorry, I am travelling. May take me a while to get to this but I'll do it as soon as I can.
arv, do you have time to take a look? On 2011/10/13 22:00:24, Evan Stade wrote: > sorry, I am travelling. May take me a while to get to this but I'll do it as > soon as I can.
http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/ntp4/a... File chrome/browser/resources/ntp4/apps_page.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/ntp4/a... chrome/browser/resources/ntp4/apps_page.js:337: this.infoBubbleShowing_ = infoBubble; can you document this? Also currentlyShowingInfoBubble_ might be a better name (infoBubbleShowing_ sounds like a boolean) http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/css/expandable_bubble.css (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:6: .ex-bubble { don't abbreviate: use .expandable-bubble http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:19: z-index: 3; why no -webkit-transitions anywhere? http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:74: .ex-bubble-arrow { I thought we were going for a hook arrow? Is this something you plan to change? What is your plan for implementing the hook? http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:39: this.dismissOnBlur_ = true; this is always false, right? for now I wouldn't make this a toggleable option
Sorry for not seeing this earlier. I always try do your code reviews in the morning due to the time zone difference. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/ntp4/a... File chrome/browser/resources/ntp4/apps_page.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/ntp4/a... chrome/browser/resources/ntp4/apps_page.js:287: /* Missing * /** http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/ntp4/a... chrome/browser/resources/ntp4/apps_page.js:290: * @param {boolean} full Whether we want the headline or just the content. @private http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/ntp4/a... chrome/browser/resources/ntp4/apps_page.js:315: /* Fix comment format here too http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/css/expandable_bubble.css (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:40: .ex-bubble-main { remove http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:29: '<div class=\"ex-bubble-contents\">' + Replace \" with " http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:31: '<div class=\"ex-bubble-main\" hidden=\"true\"></div>' + It is hidden, not hidden="..." '<div class="ex-bubble-main" hidden></div>' + http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:39: this.dismissOnBlur_ = true; This can be moved to the prototype. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:45: * @param {node} An HTML element. @param {Node} node An HTML element. or @type {Node} since this is a setter. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:49: bubbleTitle.innerHTML = ''; textContent = '' But it doesn't really matter http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:77: * Sets whether the bubble will dismiss on blur. By default, it does. Please fix formatting of the comment http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:80: set dismissOnBlur(value) { This does not need a setter. since it has no side effect. I would just go with /** * Whether the bubble will dismiss on blur. By default, it does. * @type {boolean} **/ dismissOnBlur: true, http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:89: set handleCloseEvent(func) { no need for setter http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:102: this.style.top = (clientRect.top - this.offsetHeight + Maybe a variable for top to reduce the code size http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:113: resizeAndReposition: function() { This should have a trailing underscore since it is private. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:128: bubbleTitle.style.width = (width - 2) + 'px'; I feel like the parentheses on expression like these does not help readability. Also, the Google JS Style has my back here and says that parens should only be used where required. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:146: title.classList.add('ex-bubble-title-bubble-expanded'); This could probably be done by adding and removing an expanded attribute on the bubble element and then use css attribute selectors for the sub elements. Maybe the elements are not in the same tree? http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:246: e.stopPropagation(); Please add comments about both of these to explain why they are needed. It is a common source of bugs to call stopPropagation and preventDefault too aggressively.
http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:128: bubbleTitle.style.width = (width - 2) + 'px'; On 2011/10/14 22:20:55, arv wrote: > I feel like the parentheses on expression like these does not help readability. > Also, the Google JS Style has my back here and says that parens should only be > used where required. I believe he got this style from copying my code. The reason I always do this is because you're mixing two operations, addition and string concatenation. Although evaluation order saves us in this particular case, if you are not careful this can lead to unexpected results, for example: 'foo' + 1 + 2 === 'foo12'
All done (except as below). PTAL. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/css/expandable_bubble.css (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:19: z-index: 3; Animations were deemed nice-to-have and since this project got pushed to M17 (and there is plenty of work left for M16) we decided to take what we have now and get it in and polish it for M17 (this is all behind a flag anyway). On 2011/10/14 18:22:59, Evan Stade wrote: > why no -webkit-transitions anywhere? http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:74: .ex-bubble-arrow { Yes, same reason as above (pushed to M17). I don't exactly know how the hook will be implemented, but I suspect it will be a bitmap once it shows up in M17. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:146: title.classList.add('ex-bubble-title-bubble-expanded'); Indeed. And I like it a lot more, actually. Done. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:246: e.stopPropagation(); I don't know if they are needed. This is from the bubble.js class, which this class is based on. Evan, do you know? On 2011/10/14 22:20:55, arv wrote: > Please add comments about both of these to explain why they are needed. It is a > common source of bugs to call stopPropagation and preventDefault too > aggressively.
http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/css/expandable_bubble.css (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/css/expandable_bubble.css:74: .ex-bubble-arrow { On 2011/10/18 12:39:10, Finnur wrote: > Yes, same reason as above (pushed to M17). I don't exactly know how the hook > will be implemented, but I suspect it will be a bitmap once it shows up in M17. right... I just think fixing that is going to significantly change the implementation. If you're telling me it's temporary and about to be deleted then I hardly see the point in reviewing it. http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/1/chrome/browser/resources/shared... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:246: e.stopPropagation(); On 2011/10/18 12:39:10, Finnur wrote: > I don't know if they are needed. This is from the bubble.js class, which this > class is based on. > > Evan, do you know? yea they are needed, this is a hacked up way of implementing a focus grab as I don't think focus grabs are officially possible in js > > On 2011/10/14 22:20:55, arv wrote: > > Please add comments about both of these to explain why they are needed. It is > a > > common source of bugs to call stopPropagation and preventDefault too > > aggressively. >
http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/apps_page.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/apps_page.js:442: <<<<<<< .mine Please resolve http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:43: * @type {node} An HTML element to set as the title. {Node} http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:83: if (this.getAttribute('expanded') != null) { or hasAttribute There is also a "macro" in cr.js that allows you to do: cr.defineProperty(ExpandableBubble, 'expanded', cr.PropertyKind.BOOL_ATTR); after that you can use it as an ordinary property. See http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/sha... for sample usage.
arv: you can ignore this email... just addressing one point from estade to get an extra 'timezone cycle' in before I go to bed. :) estade: > right... I just think fixing that is going to significantly change the > implementation. If you're telling me it's temporary and about to be > deleted then I hardly see the point in reviewing it. I didn't say it was temporary so don't let that stop you from reviewing. What I want is to get what I have now checked in so people can play with it. It might change for M17, but I don't think changing the arrow is drastic enough to warrant scrapping this class and starting over. Might be wrong, but that's my current thinking. On Tue, Oct 18, 2011 at 16:50, <arv@chromium.org> wrote: > > http://codereview.chromium.**org/8208014/diff/10001/chrome/** > browser/resources/ntp4/apps_**page.js<http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/ntp4/apps_page.js> > File chrome/browser/resources/ntp4/**apps_page.js (right): > > http://codereview.chromium.**org/8208014/diff/10001/chrome/** > browser/resources/ntp4/apps_**page.js#newcode442<http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/ntp4/apps_page.js#newcode442> > chrome/browser/resources/ntp4/**apps_page.js:442: <<<<<<< .mine > Please resolve > > http://codereview.chromium.**org/8208014/diff/10001/chrome/** > browser/resources/shared/js/**cr/ui/expandable_bubble.js<http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js> > File chrome/browser/resources/**shared/js/cr/ui/expandable_**bubble.js > (right): > > http://codereview.chromium.**org/8208014/diff/10001/chrome/** > browser/resources/shared/js/**cr/ui/expandable_bubble.js#**newcode43<http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js#newcode43> > chrome/browser/resources/**shared/js/cr/ui/expandable_**bubble.js:43: * > @type {node} An HTML element to set as the title. > {Node} > > http://codereview.chromium.**org/8208014/diff/10001/chrome/** > browser/resources/shared/js/**cr/ui/expandable_bubble.js#**newcode83<http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js#newcode83> > chrome/browser/resources/**shared/js/cr/ui/expandable_**bubble.js:83: if > (this.getAttribute('expanded') != null) { > or hasAttribute > > There is also a "macro" in cr.js that allows you to do: > > cr.defineProperty(**ExpandableBubble, 'expanded', > cr.PropertyKind.BOOL_ATTR); > > after that you can use it as an ordinary property. See > http://www.google.com/**codesearch#OAMlx_jo-ck/src/** > chrome/browser/resources/**shared/js/cr/ui/menu_item.js&**type=cs&l=140<http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/shared/js/cr/ui/menu_item.js&type=cs&l=140> > for sample usage. > > http://codereview.chromium.**org/8208014/<http://codereview.chromium.org/8208... >
http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:205: if (e.keyCode == 27) // Esc. if escape hides, then don't you want blur() to hide as well? http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:219: e.stopPropagation(); I am not sure whether these are desired in the mousedown case since you are not trying to emulate a focus grab
Updated to address review comments (and fix z-order where one expanded bubble could underlap another unexpanded bubble). http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:83: if (this.getAttribute('expanded') != null) { Ooh, nice. http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:205: if (e.keyCode == 27) // Esc. Good question. I think, since the key event is not targeted towards a specific balloon, that Esc should collapse any expanded bubble, as opposed to hiding it. Otherwise, you press Esc and all bubbles on screen disappear. On 2011/10/19 01:57:05, Evan Stade wrote: > if escape hides, then don't you want blur() to hide as well? http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:219: e.stopPropagation(); Do you mean I don't need it for the keyboard event? If I remove it for mousedown I see some wonkyness when I click the within/outside the bubble (the web store icon pulsates). This is by the way exactly what bubble.js does in this function... On 2011/10/19 01:57:05, Evan Stade wrote: > I am not sure whether these are desired in the mousedown case since you are not > trying to emulate a focus grab
http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:205: if (e.keyCode == 27) // Esc. On 2011/10/19 15:12:30, Finnur wrote: > Good question. I think, since the key event is not targeted towards a specific > balloon, that Esc should collapse any expanded bubble, as opposed to hiding it. I agree. Your keyboard handler should only be connected to this bubble, not the doc. but I was talking about the blur event, i.e. when the bubble loses focus via tabbing or other means, that the bubble should collapse. This seems to go hand in hand with escape causing collapse. > Otherwise, you press Esc and all bubbles on screen disappear. > > On 2011/10/19 01:57:05, Evan Stade wrote: > > if escape hides, then don't you want blur() to hide as well? > http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:219: e.stopPropagation(); On 2011/10/19 15:12:30, Finnur wrote: > Do you mean I don't need it for the keyboard event? no, I mean that you don't need it for mousedown > If I remove it for mousedown I see some wonkyness when I click the > within/outside the bubble (the web store icon pulsates). that is an unrelated bug fixed on tip of tree > This is by the way exactly what bubble.js does in this function... because bubble.js is emulating a focus grab. > > On 2011/10/19 01:57:05, Evan Stade wrote: > > I am not sure whether these are desired in the mousedown case since you are > not > > trying to emulate a focus grab >
http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:183: this.eventTracker_.add(doc, 'keydown', this, true); Silly question: arv & estade: How do I convert this to a keydown event for this class only. I've tried various things and looked at how the eventTracker is used elsewhere, but I've not gotten it to work right. http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:205: if (e.keyCode == 27) // Esc. See question above... On 2011/10/19 23:26:14, Evan Stade wrote: > On 2011/10/19 15:12:30, Finnur wrote: > > Good question. I think, since the key event is not targeted towards a specific > > balloon, that Esc should collapse any expanded bubble, as opposed to hiding > it. > > I agree. Your keyboard handler should only be connected to this bubble, not the > doc. > > but I was talking about the blur event, i.e. when the bubble loses focus via > tabbing or other means, that the bubble should collapse. This seems to go hand > in hand with escape causing collapse. > > > Otherwise, you press Esc and all bubbles on screen disappear. > > > > On 2011/10/19 01:57:05, Evan Stade wrote: > > > if escape hides, then don't you want blur() to hide as well? > > > http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:219: e.stopPropagation(); Ah, I see. On 2011/10/19 23:26:14, Evan Stade wrote: > On 2011/10/19 15:12:30, Finnur wrote: > > Do you mean I don't need it for the keyboard event? > > no, I mean that you don't need it for mousedown > > > If I remove it for mousedown I see some wonkyness when I click the > > within/outside the bubble (the web store icon pulsates). > > that is an unrelated bug fixed on tip of tree > > > This is by the way exactly what bubble.js does in this function... > > because bubble.js is emulating a focus grab. > > > > > On 2011/10/19 01:57:05, Evan Stade wrote: > > > I am not sure whether these are desired in the mousedown case since you are > > not > > > trying to emulate a focus grab > > >
http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:183: this.eventTracker_.add(doc, 'keydown', this, true); On 2011/10/20 11:36:08, Finnur wrote: > Silly question: arv & estade: How do I convert this to a keydown event for this > class only. I've tried various things and looked at how the eventTracker is used > elsewhere, but I've not gotten it to work right. change doc to the node you want to listen on
http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:183: this.eventTracker_.add(doc, 'keydown', this, true); On 2011/10/20 17:08:50, Evan Stade wrote: > On 2011/10/20 11:36:08, Finnur wrote: > > Silly question: arv & estade: How do I convert this to a keydown event for > this > > class only. I've tried various things and looked at how the eventTracker is > used > > elsewhere, but I've not gotten it to work right. > > change doc to the node you want to listen on Remember that key events are dispatched from the focused element.
On 2011/10/20 18:54:13, arv wrote: > http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... > File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): > > http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... > chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:183: > this.eventTracker_.add(doc, 'keydown', this, true); > On 2011/10/20 17:08:50, Evan Stade wrote: > > On 2011/10/20 11:36:08, Finnur wrote: > > > Silly question: arv & estade: How do I convert this to a keydown event for > > this > > > class only. I've tried various things and looked at how the eventTracker is > > used > > > elsewhere, but I've not gotten it to work right. > > > > change doc to the node you want to listen on > > Remember that key events are dispatched from the focused element. right right... so you need to take focus when you expand the bubble
On 2011/10/20 19:55:37, Evan Stade wrote: > On 2011/10/20 18:54:13, arv wrote: > > > http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... > > File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): > > > > > http://codereview.chromium.org/8208014/diff/10001/chrome/browser/resources/sh... > > chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:183: > > this.eventTracker_.add(doc, 'keydown', this, true); > > On 2011/10/20 17:08:50, Evan Stade wrote: > > > On 2011/10/20 11:36:08, Finnur wrote: > > > > Silly question: arv & estade: How do I convert this to a keydown event for > > > this > > > > class only. I've tried various things and looked at how the eventTracker > is > > > used > > > > elsewhere, but I've not gotten it to work right. > > > > > > change doc to the node you want to listen on > > > > Remember that key events are dispatched from the focused element. > > right right... so you need to take focus when you expand the bubble the more I think about it, the more I think this should be (fake) focus grabbing just like bubble.js
OK, if we want to fake the focus grabbing... then we are at full circle, aren't we? I'm not sure what if anything remains to be changed in this changelist...
On 2011/10/25 11:32:25, Finnur wrote: > OK, if we want to fake the focus grabbing... then we are at full circle, aren't > we? I'm not sure what if anything remains to be changed in this changelist... well I am confused. I thought you said that pressing escape would close all open notification bubbles. How do you have more than one open notification bubble?
Clicking outside the bubble or pressing Esc collapses the bubble (if it is expanded) and does nothing if not expanded. Therefore, you can have many _open_ bubbles in a collapsed state, just only one can be expanded at a time, which is what I think we want for this feature at least. I can foresee we might want to have bubbles not collapse on say clicking outside the bubble, but that's what this.dismissOnBlur_ was for, which we removed in version 2 of this changelist (it was always set to false).
So, to expand a bit... I think a fake focus grab, like we have in bubble.js, sounds good. That also means we should intercept the even propagation, like we do in bubble.js. Therefore, I believe I've addressed all the remaining comments, or do you guys disagree and think something is remaining?
On 2011/10/27 00:01:18, Finnur wrote: > So, to expand a bit... I think a fake focus grab, like we have in bubble.js, > sounds good. That also means we should intercept the even propagation, like we > do in bubble.js. Therefore, I believe I've addressed all the remaining comments, > or do you guys disagree and think something is remaining? _event_ propagation.
lgtm http://codereview.chromium.org/8208014/diff/31002/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/31002/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:221: e.stopPropagation(); I still like comments in the code for these http://codereview.chromium.org/8208014/diff/31002/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:223: return; useless return
Thanks. I'll address those. Evan, let me know if you have remaining concerns. Otherwise I'll check this in in a couple of days or so.
http://codereview.chromium.org/8208014/diff/31002/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/31002/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:179: this.eventTracker_.add(window, don't connect on |window| http://codereview.chromium.org/8208014/diff/31002/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:221: e.stopPropagation(); you only want a grab when expanded. This is blocking keydown and mousedown even when the bubble is collapsed, no?
All good points (addressed). Please take a look and see if you like these last changes...
lgtm, just nits http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:83: if (this.expanded) { prefer ternary http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:176: 'load', this.resizeAndReposition_.bind(this)); why is this one necessary? I should have thought load would have to have been called by the time we show() http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:178: 'resize', this.collapseBubble_.bind(this)); Why have you made this collapse instead of just reposition? I'd rather it just resize/reposition but I don't feel particularly strongly. http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:180: 'click', this.onNotificationClick_.bind(this)); the bind(this) part is no longer necessary
http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... File chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js (right): http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:176: 'load', this.resizeAndReposition_.bind(this)); Nope. The show call can come before load is called. On 2011/11/01 20:47:24, Evan Stade wrote: > why is this one necessary? I should have thought load would have to have been > called by the time we show() http://codereview.chromium.org/8208014/diff/30006/chrome/browser/resources/sh... chrome/browser/resources/shared/js/cr/ui/expandable_bubble.js:178: 'resize', this.collapseBubble_.bind(this)); We can start with re-position and see how it goes. On 2011/11/01 20:47:24, Evan Stade wrote: > Why have you made this collapse instead of just reposition? I'd rather it just > resize/reposition but I don't feel particularly strongly. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
