Adding/renaming a folder in the bookmark manager should not hide the icon.

Adding/renaming a folder in the bookmark manager should not hide the icon.

BUG=649629
TEST=Select a folder in the bookmark manager and hit F2.  The folder icon should not disappear and the input field for changing the name should have a fully visible border.  Type a name and hit enter; the text should not move between before and after you hit enter.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/93a5e7c45a78646f04f1de812da3cb41d6667845
Cr-Commit-Position: refs/heads/master@{#428189}

[Peter Kasting, 2016-10-26 01:41:00]

Description was changed from

==========
Adding/renaming a folder in the bookmark manager should not hide the icon.

BUG=649629
TEST=Select a folder in the bookmark manager and hit F2.  The folder icon should
not disappear and the input field for changing the name should have a fully
visible border.  Type a name and hit enter; the text should not move between
before and after you hit enter.
==========

to

==========
Adding/renaming a folder in the bookmark manager should not hide the icon.

BUG=649629
TEST=Select a folder in the bookmark manager and hit F2.  The folder icon should
not disappear and the input field for changing the name should have a fully
visible border.  Type a name and hit enter; the text should not move between
before and after you hit enter.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Peter Kasting, 2016-10-26 01:41:03]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 01:41:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2016-10-26 01:49:57]

pkasting@chromium.org changed reviewers:
+ dbeam@chromium.org

[Peter Kasting, 2016-10-26 01:49:57]

Fallout from https://codereview.chromium.org/2350473003 .

Please see individual explanatory comments below, especially where I ask for
help on changing the overflow of .label-text.

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:59: z-index: -1;
Without this, the input field, which has a -webkit-margin-start of -4px (see
line 142), looks truncated on the left where it overlaps this div, even though
the div "appears" to be empty in this region.

I don't know exactly what draws the gradient background on selected BMM rows,
but that background is what we see in this region instead.  Maybe it's getting
separately drawn on each individual div.

We don't want to fix this by removing the -4px margin, or the text will shift
horizontally when we start/stop editing.

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:139: list [editing]
.label-text input,
This change should have no effect, nor should the one on line 166, but they were
made to be consistent with line 176.

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:176: list [editing]
.label-text,
I made this change speculatively.  Due to
https://codereview.chromium.org/2350473003 , whatever purpose the "overflow:
visible" style here served was no effective.  Before that change, .label was the
direct parent of the <input> element; afterwards, .label-text was introduced to
fill that role, and given its own "overflow: hidden" style (see line 71), which
would override this.

So this aims to return to the previous behavior.  But I don't know what effect
toggling these overflow values actually has in this specific case, so I'm not
sure how to test that this is really fixing a bug.  Help :(

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:483: var urlEl
= this.querySelector('.url');
I could have used a bunch of .firstChild.nextSibling and similar statements, but
this seemed less fragile to future changes.

[commit-bot: I haz the power, 2016-10-26 01:54:59]

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

[commit-bot: I haz the power, 2016-10-26 01:55:00]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Peter Kasting, 2016-10-26 02:56:30]

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:483: var urlEl
= this.querySelector('.url');
On 2016/10/26 01:49:57, Peter Kasting wrote:
> I could have used a bunch of .firstChild.nextSibling and similar statements,
but
> this seemed less fragile to future changes.

...however, now Closure compiler complains that labelEl is (Element|null) at
line 556, and Node is required.

Do I just need to put in some sort of explicit (pointless) null-check here?

[Dan Beam, 2016-10-26 03:06:21]

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:483: var urlEl
= this.querySelector('.url');
On 2016/10/26 02:56:30, Peter Kasting wrote:
> On 2016/10/26 01:49:57, Peter Kasting wrote:
> > I could have used a bunch of .firstChild.nextSibling and similar statements,
> but
> > this seemed less fragile to future changes.
> 
> ...however, now Closure compiler complains that labelEl is (Element|null) at
> line 556, and Node is required.
> 
> Do I just need to put in some sort of explicit (pointless) null-check here?

assert(), queryRequiredElement() or /** @type {!Element} */(...)

[Peter Kasting, 2016-10-26 03:33:38]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[Peter Kasting, 2016-10-26 03:33:59]

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:483: var urlEl
= this.querySelector('.url');
On 2016/10/26 03:06:21, Dan Beam wrote:
> On 2016/10/26 02:56:30, Peter Kasting wrote:
> > On 2016/10/26 01:49:57, Peter Kasting wrote:
> > > I could have used a bunch of .firstChild.nextSibling and similar
statements,
> > but
> > > this seemed less fragile to future changes.
> > 
> > ...however, now Closure compiler complains that labelEl is (Element|null) at
> > line 556, and Node is required.
> > 
> > Do I just need to put in some sort of explicit (pointless) null-check here?
> 
> assert(), queryRequiredElement() or /** @type {!Element} */(...)

Thanks; used queryRequiredElement() since that seemed the most elegant.

[commit-bot: I haz the power, 2016-10-26 03:34:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 04:09:21]

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

[commit-bot: I haz the power, 2016-10-26 04:09:22]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Dan Beam, 2016-10-27 03:56:17]

lgtm

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:59: z-index: -1;
On 2016/10/26 01:49:57, Peter Kasting wrote:
> Without this, the input field, which has a -webkit-margin-start of -4px (see
> line 142), looks truncated on the left where it overlaps this div, even though
> the div "appears" to be empty in this region.
> 
> I don't know exactly what draws the gradient background on selected BMM rows,
> but that background is what we see in this region instead.  Maybe it's getting
> separately drawn on each individual div.
> 
> We don't want to fix this by removing the -4px margin, or the text will shift
> horizontally when we start/stop editing.

Acknowledged.

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:139: list [editing]
.label-text input,
On 2016/10/26 01:49:57, Peter Kasting wrote:
> This change should have no effect, nor should the one on line 166, but they
were
> made to be consistent with line 176.

Acknowledged.

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:176: list [editing]
.label-text,
On 2016/10/26 01:49:57, Peter Kasting wrote:
> I made this change speculatively.  Due to
> https://codereview.chromium.org/2350473003 , whatever purpose the "overflow:
> visible" style here served was no effective.  Before that change, .label was
the
> direct parent of the <input> element; afterwards, .label-text was introduced
to
> fill that role, and given its own "overflow: hidden" style (see line 71),
which
> would override this.
> 
> So this aims to return to the previous behavior.  But I don't know what effect
> toggling these overflow values actually has in this specific case, so I'm not
> sure how to test that this is really fixing a bug.  Help :(

¯\_(ツ)_/¯

https://codereview.chromium.org/2444373003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://codereview.chromium.org/2444373003/diff/20001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:482: var
labelEl = queryRequiredElement('.label-text', this);
labelTextEl?

https://codereview.chromium.org/2444373003/diff/20001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:548: var doc =
this.ownerDocument;
can we do

var labelTextEl = queryRequiredElement('.label-text', this);
var urlEl = queryRequiredElement('.url', this);

here instead of up above where you are?  these variables aren't used until later

[Peter Kasting, 2016-10-27 06:00:26]

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:176: list [editing]
.label-text,
On 2016/10/27 03:56:17, Dan Beam wrote:
> On 2016/10/26 01:49:57, Peter Kasting wrote:
> > I made this change speculatively.  Due to
> > https://codereview.chromium.org/2350473003 , whatever purpose the "overflow:
> > visible" style here served was no effective.  Before that change, .label was
> the
> > direct parent of the <input> element; afterwards, .label-text was introduced
> to
> > fill that role, and given its own "overflow: hidden" style (see line 71),
> which
> > would override this.
> > 
> > So this aims to return to the previous behavior.  But I don't know what
effect
> > toggling these overflow values actually has in this specific case, so I'm
not
> > sure how to test that this is really fixing a bug.  Help :(
> 
> ¯\_(ツ)_/¯

:(

I'm inclined to rip out the overflow fiddling entirely and see what breaks,
except I wouldn't notice, and months later it'd end up being tasked to me to
fix. :P

This whole file needs way more "Why do we have this" comments.

https://codereview.chromium.org/2444373003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right):

https://codereview.chromium.org/2444373003/diff/20001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:482: var
labelEl = queryRequiredElement('.label-text', this);
On 2016/10/27 03:56:17, Dan Beam wrote:
> labelTextEl?

Done.

https://codereview.chromium.org/2444373003/diff/20001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:548: var doc =
this.ownerDocument;
On 2016/10/27 03:56:17, Dan Beam wrote:
> can we do
> 
> var labelTextEl = queryRequiredElement('.label-text', this);
> var urlEl = queryRequiredElement('.url', this);
> 
> here instead of up above where you are?  these variables aren't used until
later

Done.

[Peter Kasting, 2016-10-27 06:00:30]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-10-27 06:00:30]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2444373003/#ps40001
(title: "Review comments")

[commit-bot: I haz the power, 2016-10-27 06:00:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 07:31:00]

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

[commit-bot: I haz the power, 2016-10-27 07:31:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2016-10-27 17:27:55]

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
File chrome/browser/resources/bookmark_manager/css/bmm.css (right):

https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
chrome/browser/resources/bookmark_manager/css/bmm.css:176: list [editing]
.label-text,
On 2016/10/27 06:00:26, Peter Kasting wrote:
> On 2016/10/27 03:56:17, Dan Beam wrote:
> > On 2016/10/26 01:49:57, Peter Kasting wrote:
> > > I made this change speculatively.  Due to
> > > https://codereview.chromium.org/2350473003 , whatever purpose the
"overflow:
> > > visible" style here served was no effective.  Before that change, .label
was
> > the
> > > direct parent of the <input> element; afterwards, .label-text was
introduced
> > to
> > > fill that role, and given its own "overflow: hidden" style (see line 71),
> > which
> > > would override this.
> > > 
> > > So this aims to return to the previous behavior.  But I don't know what
> effect
> > > toggling these overflow values actually has in this specific case, so I'm
> not
> > > sure how to test that this is really fixing a bug.  Help :(
> > 
> > ¯\_(ツ)_/¯
> 
> :(
> 
> I'm inclined to rip out the overflow fiddling entirely and see what breaks,
> except I wouldn't notice, and months later it'd end up being tasked to me to
> fix. :P
> 
> This whole file needs way more "Why do we have this" comments.

i mean, i could find out, but this code is basicallly deprecated in my mind and
i'd just prefer to focus on its replacement rather than understanding old code
with all authors gone

[Peter Kasting, 2016-10-27 21:42:49]

On 2016/10/27 17:27:55, Dan Beam wrote:
>
https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
> File chrome/browser/resources/bookmark_manager/css/bmm.css (right):
> 
>
https://codereview.chromium.org/2444373003/diff/1/chrome/browser/resources/bo...
> chrome/browser/resources/bookmark_manager/css/bmm.css:176: list [editing]
> .label-text,
> On 2016/10/27 06:00:26, Peter Kasting wrote:
> > On 2016/10/27 03:56:17, Dan Beam wrote:
> > > On 2016/10/26 01:49:57, Peter Kasting wrote:
> > > > I made this change speculatively.  Due to
> > > > https://codereview.chromium.org/2350473003 , whatever purpose the
> "overflow:
> > > > visible" style here served was no effective.  Before that change, .label
> was
> > > the
> > > > direct parent of the <input> element; afterwards, .label-text was
> introduced
> > > to
> > > > fill that role, and given its own "overflow: hidden" style (see line
71),
> > > which
> > > > would override this.
> > > > 
> > > > So this aims to return to the previous behavior.  But I don't know what
> > effect
> > > > toggling these overflow values actually has in this specific case, so
I'm
> > not
> > > > sure how to test that this is really fixing a bug.  Help :(
> > > 
> > > ¯\_(ツ)_/¯
> > 
> > :(
> > 
> > I'm inclined to rip out the overflow fiddling entirely and see what breaks,
> > except I wouldn't notice, and months later it'd end up being tasked to me to
> > fix. :P
> > 
> > This whole file needs way more "Why do we have this" comments.
> 
> i mean, i could find out, but this code is basicallly deprecated in my mind
and
> i'd just prefer to focus on its replacement rather than understanding old code
> with all authors gone

Oh, is there a snazzy MD BMM on the way then?  I hadn't heard anything.

I would feel better knowing that this stuff is going to be replaced at some
point :P

[Peter Kasting, 2016-10-27 21:56:49]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-10-27 21:57:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 23:08:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 23:10:38]

Description was changed from

==========
Adding/renaming a folder in the bookmark manager should not hide the icon.

BUG=649629
TEST=Select a folder in the bookmark manager and hit F2.  The folder icon should
not disappear and the input field for changing the name should have a fully
visible border.  Type a name and hit enter; the text should not move between
before and after you hit enter.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Adding/renaming a folder in the bookmark manager should not hide the icon.

BUG=649629
TEST=Select a folder in the bookmark manager and hit F2.  The folder icon should
not disappear and the input field for changing the name should have a fully
visible border.  Type a name and hit enter; the text should not move between
before and after you hit enter.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/93a5e7c45a78646f04f1de812da3cb41d6667845
Cr-Commit-Position: refs/heads/master@{#428189}
==========

[commit-bot: I haz the power, 2016-10-27 23:10:39]

Patchset 3 (id:??) landed as
https://crrev.com/93a5e7c45a78646f04f1de812da3cb41d6667845
Cr-Commit-Position: refs/heads/master@{#428189}