[tabbed options] more work on content settings exceptions lists

[tabbed options] more work on content settings exceptions lists

- tie "edit" mode to "selected"
-- selecting enters into edit mode
-- blurring exits edit mode and deselects
- fix up layout within rows (faux columns)
- add a special "add new exception" row (this should be at the bottom of the table, but it's currently at the top)
- change behavior when the user attempts to commit an invalid exception. Instead of trying to regrab focus, just revert the row to its previous state.

TODO: list should be sized dynamically, i.e. no scrollbar.
TODO: list should be in sub-sub dialog
TODO: rows should have delete X

BUG=64153
TEST=manual

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

[stuartmorgan, 2010-12-13 17:49:51]

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.html:33: <list mode="normal"
id="evan"></list>
I'm guessing this isn't supposed to be here.

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_exceptions_area.js:214: if
(this.isAddNewRow)
It seems like there's a bunch of this kind of logic; what about doing a bit of
refactoring of methods and then making the new row a subclass that can override
a few places?

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:326: display: none !important;
What is the !important for? There's almost certainly a better way to do this
(for example, using :not(.hidden) when you are setting the display property on
one of your elements)

[arv (Not doing code reviews), 2010-12-13 19:58:34]

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:326: display: none !important;
On 2010/12/13 17:49:51, stuartmorgan wrote:
> What is the !important for? There's almost certainly a better way to do this
> (for example, using :not(.hidden) when you are setting the display property on
> one of your elements)

I'm fine with this !important.

I'm also OK with special casing the troublesome selector by adding a more
specific one

#foo:not(.hidden) {
  display: block;
}

[Evan Stade, 2010-12-13 20:00:19]

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.html:33: <list mode="normal"
id="evan"></list>
On 2010/12/13 17:49:51, stuartmorgan wrote:
> I'm guessing this isn't supposed to be here.

that is accurate

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_exceptions_area.js:214: if
(this.isAddNewRow)
On 2010/12/13 17:49:51, stuartmorgan wrote:
> It seems like there's a bunch of this kind of logic; what about doing a bit of
> refactoring of methods and then making the new row a subclass that can
override
> a few places?

ok, i'll do this

[Evan Stade, 2010-12-14 21:56:57]

updated.

[arv (Not doing code reviews), 2010-12-14 22:42:37]

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.css
(right):

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.css:15:
margin-right: 20px;
-webkit-margin-end?

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:463: for
(var i = this.firstIndex_; i < this.lastIndex_; i++) {
firstIndex_ and lastIndex_ are private. Please do not use

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:478:
this.dataModel.push(null);
this.dataModel = new ArrayDataModel([null]);

[Evan Stade, 2010-12-14 23:16:02]

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:463: for
(var i = this.firstIndex_; i < this.lastIndex_; i++) {
On 2010/12/14 22:42:37, arv wrote:
> firstIndex_ and lastIndex_ are private. Please do not use

do you want me to add accessors, or somehow mark them as protected instead of
private? Or is there a better way to iterate over existing ListItems?

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:478:
this.dataModel.push(null);
On 2010/12/14 22:42:37, arv wrote:
> this.dataModel = new ArrayDataModel([null]);

ok

[Evan Stade, 2010-12-14 23:23:03]

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:463: for
(var i = this.firstIndex_; i < this.lastIndex_; i++) {
On 2010/12/14 23:16:02, Evan Stade wrote:
> On 2010/12/14 22:42:37, arv wrote:
> > firstIndex_ and lastIndex_ are private. Please do not use
> 
> do you want me to add accessors, or somehow mark them as protected instead of
> private? Or is there a better way to iterate over existing ListItems?

I guess I can use getListItemByIndex and just check for null.

[arv (Not doing code reviews), 2010-12-14 23:50:59]

LGTM

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/11002/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:463: for
(var i = this.firstIndex_; i < this.lastIndex_; i++) {
On 2010/12/14 23:23:03, Evan Stade wrote:
> On 2010/12/14 23:16:02, Evan Stade wrote:
> > On 2010/12/14 22:42:37, arv wrote:
> > > firstIndex_ and lastIndex_ are private. Please do not use
> > 
> > do you want me to add accessors, or somehow mark them as protected instead
of
> > private? Or is there a better way to iterate over existing ListItems?
> 
> I guess I can use getListItemByIndex and just check for null.

Yeah, I think the old code was cleaner here.

http://codereview.chromium.org/5699004/diff/19001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:466: //
recipient, or if the response is stale (i.e. the input value has
Useless parens

[Evan Stade, 2010-12-15 01:45:09]

my old code actually didn't always work, because the list item doesn't exist if
it's scrolled out of view.

I changed list.js. I don't think that list.items should return beforeFiller_ or
afterFiller_; also I don't think the code is used anywhere else except one place
in the bookmark manager where my change doesn't seem to have an effect.

[Evan Stade, 2010-12-15 01:49:48]

oh wait, it checked for null. OK, I can't remember why I changed it, but this
seems more correct in any case.

[arv (Not doing code reviews), 2010-12-15 02:47:27]

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.js:409: //
delay this a bit until we know if the new focus node is outside the list.
long line

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:226: * The HTML elements
representing the items. This is just all the <li>
I don't think <li> is correct here. Is there any other place that this is hard
coded? I think instances can use any tag name they want

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:230: get items() {
Yeah, this cannot have been used before since it was clearly broken.

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:231: return
this.querySelectorAll('li');
This also gives nested elements. I think we need to use children and filter out
the spacers.

[Evan Stade, 2010-12-15 04:12:38]

updated. Let me also take this opportunity to register my disappointment that
there's apparently no way to use css selectors to do this gracefully.

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5699004/diff/28001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:226: * The HTML elements
representing the items. This is just all the <li>
On 2010/12/15 02:47:27, arv wrote:
> I don't think <li> is correct here. Is there any other place that this is hard
> coded? I think instances can use any tag name they want

good point

[arv (Not doing code reviews), 2010-12-15 04:15:28]

http://codereview.chromium.org/5699004/diff/33001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5699004/diff/33001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:235: if (classList &&
!classList.contains('spacer'))
classList should never be falsey

http://codereview.chromium.org/5699004/diff/33001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:238: return listItems;
How about:

return Array.prototype.filter.call(this.children, function(child) {
  return child.classList.contains('spacer');
});

[Evan Stade, 2010-12-15 04:45:43]

k, thanks for the pointer to Array's filter. updated once again.

[arv (Not doing code reviews), 2010-12-15 06:11:27]

LGTM

[stuartmorgan, 2010-12-15 17:27:15]

LGTM