Commit inline editable list changes when window loses focus.

Commit inline editable list changes when window loses focus.

BUG=79251
TEST=manual

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

[Evan Stade, 2011-05-21 02:39:31]

rbyers: use of EventTracker
stuartmorgan: everything

http://codereview.chromium.org/7004058/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(left):

http://codereview.chromium.org/7004058/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/content_settings_exceptions_area.js:348:
function handleBlur(e) {
this isn't really related, it's just code that's no longer needed

[stuartmorgan, 2011-05-23 16:54:49]

LGTM

http://codereview.chromium.org/7004058/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/7004058/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:375:
focusElement.blur();
I'm afraid this will be confusing/unexpected for users, but I guess we can try
it...

[Evan Stade, 2011-05-24 22:00:36]

ping rick (use of EventTracker)

[Rick Byers, 2011-05-25 03:04:15]

Sorry for the delay - just got back from vacation / Canadian holiday and caught
up on e-mail.

http://codereview.chromium.org/7004058/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/7004058/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:345:
this.addEventListener('DOMNodeRemovedFromDocument',
The use of EventTracker here looks right and necessary to me.  One problem
however is if the element is removed from a document and later re-added to a
(same or different) document it will appear to work except without the
handleWindowBlur functionality.  Perhaps you want to subscribe to
DOMNodeInsertedIntoDocument and move your subscribe to 'blur' there?

[Evan Stade, 2011-05-25 17:44:52]

you are right. I doubt we ever remove and then re-add a list but we should
support that use case. Although now the code is even more ridiculously verbose
for what should be a simple action (adding event listener to window).

[Rick Byers, 2011-05-25 18:17:23]

Yeah, it sucks that this much complexity is necessary to handle all the cases. 

The only other option I see is to handle this at a higher level of abstraction -
eg. something higher-up on the page that registers once for window.blur and
decides whether the individual focus element should somehow be blurred.  It
could, for example, fire a 'window blur' event at whatever the current focus
element is and the focus element could then decide (without having to register
for events on the window) if it wants to do anything about it.

In fact, you could do that all here - register just once during initialization
for window.blur, and in the handler fire a new event at the focus element.  Then
each list element just needs to listen for that new event - no event
unsubscribing is ever necessary (and it's probably also more efficient in the
case of lots of EditableItems on a page - only invoke two event handlers on each
window blur instead of one per item).

But regardless, I think the approach you've got here is OK.  There's just a new
bug now that looks bad:

http://codereview.chromium.org/7004058/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

http://codereview.chromium.org/7004058/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:389: if (e && e.target
== this) {
You call above with u undefined so that will be a no-op.  Did you mean this to
be "if(!e || e.target == this)" perhaps?

[Evan Stade, 2011-05-25 18:45:03]

how about this? There's some inefficiency in that you have to walk up the node
stack every time there's a blur (regardless of whether the active item is within
an inline editable list item) but that's probably still cheaper than event
dispatch.

[Rick Byers, 2011-05-25 18:53:32]

Yep - I like that.  A lot simpler, and I agree that it's better than the
overhead of firing a separate event at the active element (presumably walking
the parent chain should be at least as efficient as event bubbling).

[Rick Byers, 2011-05-25 19:42:40]

LGTM