[MAC] Disable 'Import' button when visible checkboxes are unchecked.

[MAC] Disable 'Import' button when visible checkboxes are unchecked.

In "Import bookmarks and settings" overlay, 'Import' button is disabled when
no checkboxes are checked. But for 'Safari' only 2 checkboxes are shown;
when other checkboxes are checked in background(not shown), it causes the button
to always be enabled regardless of visible checkboxes checked or unchecked.
This CL checks if a checkbox is both shown and checked, to enable the button.

BUG=275265
R=dbeam@chromium.org

TEST=
1.Launch chrome ,go to settings and click on 'Import bookmarks and settings' button under 'Users' section.
2.Select 'Safari' and Uncheck 'Browsing history' and 'Favorites/Bookmarks' option .
3.Observe the 'Import' button. The 'Import' button should be disabled.

Committed: https://crrev.com/ca595a12817b4855b2eb58a038b771d0bc76f664
Cr-Commit-Position: refs/heads/master@{#299840}

[Gaja, 2014-10-07 13:39:49]

@dbeam
Please take a look. Thanks.

[Dan Beam, 2014-10-07 17:05:15]

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:78:
($('import-history-with-label').style.display != 'none' &&
i think asking the checkbox itself if it's > 0 width is better, imo

http://jsfiddle.net/b8j7ad82/

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:87:
$('import-autofill-form-data').checked);
helper function?

  function importable(type) {
    var id = 'import-' + type;
    return $(id).offsetWidth > 0 && $(id).checked;
  }

  var somethingToImport = importable('history') && importable('favorites') &&
  ...

[Gaja, 2014-10-08 04:50:07]

Dan,

Please review. Comments addressed in patch set #2.

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:78:
($('import-history-with-label').style.display != 'none' &&
On 2014/10/07 17:05:15, Dan Beam wrote:
> i think asking the checkbox itself if it's > 0 width is better, imo
> 
> http://jsfiddle.net/b8j7ad82/

I tried using '$(id).offsetWidth > 0', but during the launch of overlay,
$(id).offsetWidth > 0' value will be 'false' for all checkboxes regardless they
will be shown or not, and therefore the 'Import' button will be disabled even
though some checkboxes are checked. After any changes to drop-down options or
checkboxes, the button is being updated and works as intended.

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:87:
$('import-autofill-form-data').checked);
On 2014/10/07 17:05:15, Dan Beam wrote:
> helper function?
> 
>   function importable(type) {
>     var id = 'import-' + type;
>     return $(id).offsetWidth > 0 && $(id).checked;
>   }
> 
>   var somethingToImport = importable('history') && importable('favorites') &&
>   ...
>                           

Done. Thanks for the snippet.

[Gaja, 2014-10-08 05:02:48]

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:78:
($('import-history-with-label').style.display != 'none' &&
On 2014/10/07 17:05:15, Dan Beam wrote:
> i think asking the checkbox itself if it's > 0 width is better, imo
> 
> http://jsfiddle.net/b8j7ad82/

So, falling back to previous check, i.e $(id).style.display != 'none'. Does it
cause any other problem? Please let me know your opinion.

[Dan Beam, 2014-10-08 21:36:23]

lookin' pretty good

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:78:
($('import-history-with-label').style.display != 'none' &&
On 2014/10/08 05:02:48, gaja wrote:
> On 2014/10/07 17:05:15, Dan Beam wrote:
> > i think asking the checkbox itself if it's > 0 width is better, imo
> > 
> > http://jsfiddle.net/b8j7ad82/
> 
> So, falling back to previous check, i.e $(id).style.display != 'none'. Does it
> cause any other problem? Please let me know your opinion.

well, if the markup ever changes or these checkboxes are hidden in any other way
it'd break this code.  what about:

  var el = $('import-' + type);
  return el.checked && getComputedStyle(el).display != 'none';

(note: the .checked condition comes first as it's faster)

https://codereview.chromium.org/636933002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.js:24: * Returns true if
import checkbox for a type is visible and checked.
@param {string} type The type of data to import. Used in the element's ID.

[Gaja, 2014-10-10 05:19:30]

@dbeam,

Comments addressed in Patch Set #3. Please take a look. Thanks.

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:78:
($('import-history-with-label').style.display != 'none' &&
On 2014/10/08 21:36:23, Dan Beam wrote:
> On 2014/10/08 05:02:48, gaja wrote:
> > On 2014/10/07 17:05:15, Dan Beam wrote:
> > > i think asking the checkbox itself if it's > 0 width is better, imo
> > > 
> > > http://jsfiddle.net/b8j7ad82/
> > 
> > So, falling back to previous check, i.e $(id).style.display != 'none'. Does
it
> > cause any other problem? Please let me know your opinion.
> 
> well, if the markup ever changes or these checkboxes are hidden in any other
way
> it'd break this code.  what about:
> 
>   var el = $('import-' + type);
>   return el.checked && getComputedStyle(el).display != 'none';
> 
> (note: the .checked condition comes first as it's faster)

Done. Adding '-with-label' to 'el' and then passing to getComputedStyle, since
'display' property is being updated to its parent div element
<import-***-with-label>. (in updateCheckboxes_: function)

https://codereview.chromium.org/636933002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.js:24: * Returns true if
import checkbox for a type is visible and checked.
On 2014/10/08 21:36:23, Dan Beam wrote:
> @param {string} type The type of data to import. Used in the element's ID.

Done.

[Dan Beam, 2014-10-10 19:28:31]

why wouldn't we just do this?
https://codereview.chromium.org/648833002/diff/30001/chrome/browser/resources...

[Gaja, 2014-10-11 04:27:45]

On 2014/10/10 19:28:31, Dan Beam wrote:
> why wouldn't we just do this?
>
https://codereview.chromium.org/648833002/diff/30001/chrome/browser/resources...

Dan,

Please correct me if I am wrong.
I think it may cause regression issue ending up with BUG=271309. As per the BUG
we do not want to reset checkboxes state on changing options in drop down; i.e
if we uncheck a checkbox under "Mozilla Firefox" and change to "Safari" and
again come back to "Mozilla Firefox, the checkbox which was unchecked
previously, should remain unchecked. Please verify the same and let me know your
opinion. Thanks.

[Gaja, 2014-10-14 17:22:59]

Dan,
Please take a look.

[Dan Beam, 2014-10-15 03:41:36]

https://codereview.chromium.org/636933002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.js:29:
getComputedStyle($(el + '-with-label')).display != 'none';
fine, but use hidden instead

  var id = 'import-' + type;
  return $(id).checked && !$(id + '-with-label').hidden;

we should arguably do this on didShowPage as well...

https://codereview.chromium.org/636933002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.js:144:
checkboxWithLabel.style.display = enable ? '' : 'none';
use .hidden instead of display = 'none'.  i think this whole loop could be:

  for (var i = 0; i < importOptions.length; ++i) {
    var id = 'import-' + importOptions[i];
    var enable = browserProfile && browserProfile[importOptions[i]];
    this.setUpCheckboxState_($(id), enable);
    $(id + '-with-label').hidden = !enable;
  }

[Gaja, 2014-10-15 05:10:51]

Dan,

Comments addressed in Patch Set #4. Please take a look.

https://codereview.chromium.org/636933002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/import_data_overlay.js (right):

https://codereview.chromium.org/636933002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.js:29:
getComputedStyle($(el + '-with-label')).display != 'none';
On 2014/10/15 03:41:35, Dan Beam wrote:
> fine, but use hidden instead
> 
>   var id = 'import-' + type;
>   return $(id).checked && !$(id + '-with-label').hidden;
> 
> we should arguably do this on didShowPage as well...

Done.

> we should arguably do this on didShowPage as well...

Is it in supervised_user_import.js ?

https://codereview.chromium.org/636933002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/import_data_overlay.js:144:
checkboxWithLabel.style.display = enable ? '' : 'none';
On 2014/10/15 03:41:35, Dan Beam wrote:
> use .hidden instead of display = 'none'.  i think this whole loop could be:
> 
>   for (var i = 0; i < importOptions.length; ++i) {
>     var id = 'import-' + importOptions[i];
>     var enable = browserProfile && browserProfile[importOptions[i]];
>     this.setUpCheckboxState_($(id), enable);
>     $(id + '-with-label').hidden = !enable;
>   }

Done. This is much better. Thanks.

[Dan Beam, 2014-10-15 19:28:37]

lgtm

i'll address a related issue here:
https://codereview.chromium.org/660623002/

[Gaja, 2014-10-16 02:34:32]

On 2014/10/15 19:28:37, Dan Beam wrote:
> lgtm
> 
> i'll address a related issue here:
> https://codereview.chromium.org/660623002/

Thanks for your time and review :)

[Gaja, 2014-10-16 02:34:43]

The CQ bit was checked by gajendra.n@samsung.com

[commit-bot: I haz the power, 2014-10-16 02:35:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636933002/100001

[commit-bot: I haz the power, 2014-10-16 03:57:33]

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

[commit-bot: I haz the power, 2014-10-16 03:57:34]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gaja, 2014-10-16 03:59:47]

The CQ bit was checked by gajendra.n@samsung.com

[commit-bot: I haz the power, 2014-10-16 04:00:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636933002/100001

[commit-bot: I haz the power, 2014-10-16 04:34:06]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-10-16 04:35:03]

Patchset 4 (id:??) landed as
https://crrev.com/ca595a12817b4855b2eb58a038b771d0bc76f664
Cr-Commit-Position: refs/heads/master@{#299840}