Hide the DIV containing password import/export buttons

Hide the DIV containing password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that about:settings/passwords looks the same as without the patch. With chrome://flags/#password-import-export disabled, observe that during reloading of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/33bd96dd8b9c462fe5a83d43a6ce535321907ca5
Cr-Commit-Position: refs/heads/master@{#406616}

[vabr (Chromium), 2016-07-18 10:23:37]

Description was changed from

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls with the "hidden" attribute instead of
display:none, and keeps the containing DIV visible. As a result, on loading the
settings, the space for the controls is briefly taken into consideration and a
vertical scrollbar shows up.

This CL changes two things:
(1) Moves from making the buttons invisible to making the containing DIV
    invisible.
(2) Uses display:none instead of the "hidden" attribute, because only the
    former implies that the element takes no space on the page.

BUG=626634
==========

to

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls with the "hidden" attribute instead of
display:none, and keeps the containing DIV visible. As a result, on loading the
settings, the space for the controls is briefly taken into consideration and a
vertical scrollbar shows up.

This CL changes two things:
(1) Moves from making the buttons invisible to making the containing DIV
    invisible.
(2) Uses display:none instead of the "hidden" attribute, because only the
    former implies that the element takes no space on the page.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[vabr (Chromium), 2016-07-18 10:27:25]

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

[commit-bot: I haz the power, 2016-07-18 10:27:41]

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

[vabr (Chromium), 2016-07-18 10:29:57]

Description was changed from

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls with the "hidden" attribute instead of
display:none, and keeps the containing DIV visible. As a result, on loading the
settings, the space for the controls is briefly taken into consideration and a
vertical scrollbar shows up.

This CL changes two things:
(1) Moves from making the buttons invisible to making the containing DIV
    invisible.
(2) Uses display:none instead of the "hidden" attribute, because only the
    former implies that the element takes no space on the page.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls with the "hidden" attribute instead of
display:none, and keeps the containing DIV visible. As a result, on loading the
settings, the space for the controls is briefly taken into consideration and a
vertical scrollbar shows up.

This CL changes two things:
(1) Moves from making the buttons invisible to making the containing DIV
    invisible.
(2) Uses display:none instead of the "hidden" attribute, because only the
    former implies that the element takes no space on the page.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-18 11:28:36]

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

[commit-bot: I haz the power, 2016-07-18 11:28:36]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-18 14:07:16]

vabr@chromium.org changed reviewers:
+ stevenjb@chromium.org

[vabr (Chromium), 2016-07-18 14:07:17]

Hi Steven,

Could you please take a look?

Thanks!
Vaclav

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:292:
$('password-manager-import-export').style.display = '-webkit-box';
-webkit-box is what the action-area class in ui/webui/resources/css/overlay.css
uses. The DIV has this class, so I'm emulating dropping the override from the
password_manager.css class here. It seems fragile because if someone updates
action-area, this line will likely be missed. But I'm not sure how to do this
better.

[stevenjb, 2016-07-18 15:21:37]

stevenjb@chromium.org changed reviewers:
+ hcarmona@chromium.org

[stevenjb, 2016-07-18 15:21:37]

+hcarmona@

[stevenjb, 2016-07-18 15:27:18]

Have we made sure that this feature is included in the new md-settings UI (and
that enabling/disabling the feature works correctly there)?

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:292:
$('password-manager-import-export').style.display = '-webkit-box';
On 2016/07/18 14:07:17, vabr (Chromium) wrote:
> -webkit-box is what the action-area class in
ui/webui/resources/css/overlay.css
> uses. The DIV has this class, so I'm emulating dropping the override from the
> password_manager.css class here. It seems fragile because if someone updates
> action-area, this line will likely be missed. But I'm not sure how to do this
> better.

Instead can you toggle a class, e.g. 'noDisplay' and change the CSS to something
like #password-manager-import-export.noDisplay { display: none } ?

[vabr (Chromium), 2016-07-18 15:47:59]

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

[commit-bot: I haz the power, 2016-07-18 15:48:55]

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

[vabr (Chromium), 2016-07-18 15:53:10]

Thanks for the review, please take a look again.

As for adding this to the Material design:
The feature is behind a flag currently and will need more UI guidance for the MD
settings page. If it is not there when the MD settings launch, that's no problem
at all. And once it is getting ready for launch (launch bug is
http://crbug.com/504138) my team will make sure that we have a MD implementation
approved by the UI team. The UI team actually gated launching this on the MD
settings.

Cheers,
Vaclav

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:292:
$('password-manager-import-export').style.display = '-webkit-box';
On 2016/07/18 15:27:18, stevenjb wrote:
> On 2016/07/18 14:07:17, vabr (Chromium) wrote:
> > -webkit-box is what the action-area class in
> ui/webui/resources/css/overlay.css
> > uses. The DIV has this class, so I'm emulating dropping the override from
the
> > password_manager.css class here. It seems fragile because if someone updates
> > action-area, this line will likely be missed. But I'm not sure how to do
this
> > better.
> 
> Instead can you toggle a class, e.g. 'noDisplay' and change the CSS to
something
> like #password-manager-import-export.noDisplay { display: none } ?
> 

Good idea, done.

If you prefer "noDisplay" to the "hidden" which I used, just let me know and
I'll rename it. In that case, please confirm that it should be "noDisplay" and
not "no-display" (the latter style seems to be used for the class names in the
code around).

[stevenjb, 2016-07-18 16:34:33]

Now that I am thinking about this more, dud you try just using 'hidden' on the
parent div? It seems like that should do the same thing as setting display=none
on the div.

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:292:
$('password-manager-import-export').style.display = '-webkit-box';
On 2016/07/18 15:53:10, vabr (Chromium) wrote:
> On 2016/07/18 15:27:18, stevenjb wrote:
> > On 2016/07/18 14:07:17, vabr (Chromium) wrote:
> > > -webkit-box is what the action-area class in
> > ui/webui/resources/css/overlay.css
> > > uses. The DIV has this class, so I'm emulating dropping the override from
> the
> > > password_manager.css class here. It seems fragile because if someone
updates
> > > action-area, this line will likely be missed. But I'm not sure how to do
> this
> > > better.
> > 
> > Instead can you toggle a class, e.g. 'noDisplay' and change the CSS to
> something
> > like #password-manager-import-export.noDisplay { display: none } ?
> > 
> 
> Good idea, done.
> 
> If you prefer "noDisplay" to the "hidden" which I used, just let me know and
> I'll rename it. In that case, please confirm that it should be "noDisplay" and
> not "no-display" (the latter style seems to be used for the class names in the
> code around).

Since there is already a global 'hidden' attribute, with subtly different
behavior (as demonstrated in the bug this is fixing), using a class with the
same name seems confusing. You are right though, classes should be named, e.g.
no-display.

https://codereview.chromium.org/2154223002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/password_manager.html (right):

https://codereview.chromium.org/2154223002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/password_manager.html:31: <div
id="password-manager-import-export" class="hidden">
class="action-area no-display"

Then, in the JS:
$('password-manager-import-export').classLisst.remove('no-display')

[commit-bot: I haz the power, 2016-07-18 17:07:11]

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

[commit-bot: I haz the power, 2016-07-18 17:07:13]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-19 07:28:57]

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

[commit-bot: I haz the power, 2016-07-19 07:29:17]

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

[vabr (Chromium), 2016-07-19 08:13:49]

Description was changed from

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls with the "hidden" attribute instead of
display:none, and keeps the containing DIV visible. As a result, on loading the
settings, the space for the controls is briefly taken into consideration and a
vertical scrollbar shows up.

This CL changes two things:
(1) Moves from making the buttons invisible to making the containing DIV
    invisible.
(2) Uses display:none instead of the "hidden" attribute, because only the
    former implies that the element takes no space on the page.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[vabr (Chromium), 2016-07-19 08:14:14]

Description was changed from

==========
Set display:none for password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Hide the DIV containing password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[vabr (Chromium), 2016-07-19 08:16:57]

Thanks, Steven!

You were right, even the "hidden" attribute worked, the important thing was just
to work on the div, not on the contained controls. I am apparently still not
understanding properly the difference between hidden and display:none, but at
least the CL fixes the problem and is simpler thanks to your advice.

PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/2154223002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:292:
$('password-manager-import-export').style.display = '-webkit-box';
On 2016/07/18 16:34:33, stevenjb wrote:
> On 2016/07/18 15:53:10, vabr (Chromium) wrote:
> > On 2016/07/18 15:27:18, stevenjb wrote:
> > > On 2016/07/18 14:07:17, vabr (Chromium) wrote:
> > > > -webkit-box is what the action-area class in
> > > ui/webui/resources/css/overlay.css
> > > > uses. The DIV has this class, so I'm emulating dropping the override
from
> > the
> > > > password_manager.css class here. It seems fragile because if someone
> updates
> > > > action-area, this line will likely be missed. But I'm not sure how to do
> > this
> > > > better.
> > > 
> > > Instead can you toggle a class, e.g. 'noDisplay' and change the CSS to
> > something
> > > like #password-manager-import-export.noDisplay { display: none } ?
> > > 
> > 
> > Good idea, done.
> > 
> > If you prefer "noDisplay" to the "hidden" which I used, just let me know and
> > I'll rename it. In that case, please confirm that it should be "noDisplay"
and
> > not "no-display" (the latter style seems to be used for the class names in
the
> > code around).
> 
> Since there is already a global 'hidden' attribute, with subtly different
> behavior (as demonstrated in the bug this is fixing), using a class with the
> same name seems confusing. You are right though, classes should be named, e.g.
> no-display.
> 

Acknowledged.
(Would implement that, but your other suggestion with the hidden attribute
worked even better.)

https://codereview.chromium.org/2154223002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/password_manager.html (right):

https://codereview.chromium.org/2154223002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/password_manager.html:31: <div
id="password-manager-import-export" class="hidden">
On 2016/07/18 16:34:33, stevenjb wrote:
> class="action-area no-display"
> 
> Then, in the JS:
> $('password-manager-import-export').classLisst.remove('no-display')

Acknowledged, thanks for teaching me that!

[commit-bot: I haz the power, 2016-07-19 09:41:07]

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

[commit-bot: I haz the power, 2016-07-19 09:41:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[stevenjb, 2016-07-20 16:25:55]

On 2016/07/19 08:16:57, vabr (Chromium) wrote:
> Thanks, Steven!
> 
> You were right, even the "hidden" attribute worked, the important thing was
just
> to work on the div, not on the contained controls. I am apparently still not
> understanding properly the difference between hidden and display:none, but at
> least the CL fixes the problem and is simpler thanks to your advice.
> 

'hidden' is a global attribute. 'display' is a CSS property. They are somewhat
independent, however when 'display' is changed to anything other than 'none',
any 'hidden' attribute is removed:
 https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden
 https://developer.mozilla.org/en-US/docs/Web/CSS/display

[stevenjb, 2016-07-20 16:26:38]

lgtm

[vabr (Chromium), 2016-07-20 16:33:12]

On 2016/07/20 16:25:55, stevenjb wrote:
> On 2016/07/19 08:16:57, vabr (Chromium) wrote:
> > Thanks, Steven!
> > 
> > You were right, even the "hidden" attribute worked, the important thing was
> just
> > to work on the div, not on the contained controls. I am apparently still not
> > understanding properly the difference between hidden and display:none, but
at
> > least the CL fixes the problem and is simpler thanks to your advice.
> > 
> 
> 'hidden' is a global attribute. 'display' is a CSS property. They are somewhat
> independent, however when 'display' is changed to anything other than 'none',
> any 'hidden' attribute is removed:
>  https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden
>  https://developer.mozilla.org/en-US/docs/Web/CSS/display

Thanks for all the useful information and the review!
Vaclav

[vabr (Chromium), 2016-07-20 16:34:45]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-07-20 16:35:03]

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-07-20 18:04:43]

Description was changed from

==========
Hide the DIV containing password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Hide the DIV containing password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-20 18:04:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-20 18:07:35]

Description was changed from

==========
Hide the DIV containing password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Hide the DIV containing password import/export buttons

The import/export feature for passwords is currently behind a flag. However,
the settings WebUI hides the controls instead of the containing DIV. As a
result, on loading the settings, the space for the controls is briefly
taken into consideration and a vertical scrollbar shows up.

This CL hides the containing DIV instead of the individual controls.

Test steps:
With chrome://flags/#password-import-export enabled, observe that
about:settings/passwords looks the same as without the patch. With
chrome://flags/#password-import-export disabled, observe that during reloading
of about:settings/passwords, no vertical scroll bar is flashing/seen.

BUG=626634
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/33bd96dd8b9c462fe5a83d43a6ce535321907ca5
Cr-Commit-Position: refs/heads/master@{#406616}
==========

[commit-bot: I haz the power, 2016-07-20 18:07:37]

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