MD Settings: Change "view reported settings" string.

MD Settings: Change "view reported settings" string.

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

Review-Url: https://codereview.chromium.org/2722083003
Cr-Commit-Position: refs/heads/master@{#455188}
Committed: https://chromium.googlesource.com/chromium/src/+/4f4441915d7f7dee6dd36fcf3e06e743f5779e2c

[dpapad, 2017-03-01 19:01:50]

Description was changed from

==========
MD Settings: Change "view reported settings" string.

BUG=695630
==========

to

==========
MD Settings: Change "view reported settings" string.

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

[Dan Beam, 2017-03-01 19:10:02]

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

[Dan Beam, 2017-03-01 19:10:03]

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:130: if
(Polymer.dom(e).localTarget.tagName == 'A')
did you try

else
  e.stopPropagation();

or preventDefault()?

[dpapad, 2017-03-01 19:30:11]

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:130: if
(Polymer.dom(e).localTarget.tagName == 'A')
On 2017/03/01 at 19:10:03, Dan Beam wrote:
> did you try
> 
> else
>   e.stopPropagation();
> 
> or preventDefault()?

Yes. Tried both and did not fix the issue. The checkbox is still getting
toggled.

Note that the listener is on the checkbox itself, since I don't have a
programmatic way of adding the listener on the action link (it resides within
the translation), which is why stopPropagation() is not fixing the issue I
think.

[dpapad, 2017-03-01 19:31:10]

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:130: if
(Polymer.dom(e).localTarget.tagName == 'A')
On 2017/03/01 at 19:30:11, dpapad wrote:
> On 2017/03/01 at 19:10:03, Dan Beam wrote:
> > did you try
> > 
> > else
> >   e.stopPropagation();
> > 
> > or preventDefault()?
> 
> Yes. Tried both and did not fix the issue. The checkbox is still getting
toggled.
> 
> Note that the listener is on the checkbox itself, since I don't have a
programmatic way of adding the listener on the action link (it resides within
the translation), which is why stopPropagation() is not fixing the issue I
think.

Correction: I don't have an *declarative* way of adding the listener on the
action link itself.

[Dan Beam, 2017-03-01 19:44:45]

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:130: if
(Polymer.dom(e).localTarget.tagName == 'A')
On 2017/03/01 19:31:10, dpapad wrote:
> On 2017/03/01 at 19:30:11, dpapad wrote:
> > On 2017/03/01 at 19:10:03, Dan Beam wrote:
> > > did you try
> > > 
> > > else
> > >   e.stopPropagation();
> > > 
> > > or preventDefault()?
> > 
> > Yes. Tried both and did not fix the issue. The checkbox is still getting
> toggled.
> > 
> > Note that the listener is on the checkbox itself, since I don't have a
> programmatic way of adding the listener on the action link (it resides within
> the translation), which is why stopPropagation() is not fixing the issue I
> think.
> 
> Correction: I don't have an *declarative* way of adding the listener on the
> action link itself.

did you try stopImmediatePropagation()?

[dpapad, 2017-03-01 20:00:30]

Patchset #2 (id:20001) has been deleted

[dpapad, 2017-03-01 20:00:59]

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

[commit-bot: I haz the power, 2017-03-01 20:01:29]

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

[dpapad, 2017-03-01 20:03:54]

Patchset #2 (id:40001) has been deleted

[dpapad, 2017-03-01 20:04:01]

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

[commit-bot: I haz the power, 2017-03-01 20:04:31]

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

[dpapad, 2017-03-01 20:05:03]

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:130: if
(Polymer.dom(e).localTarget.tagName == 'A')
> did you try stopImmediatePropagation()?

I figured out a way to fix (see latest patch).

The key is to add the event listener on the <a> itself, otherwise any calls to
stopPropagation/stopImmediatePropagation are occurring too late. As revealed by
DevTools debugging the paper-checkbox was toggled before even my listener
executed (not surprising).

[Dan Beam, 2017-03-01 20:29:37]

lgtm

https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:80: 'tap',
this.onShowReportedSettingsTap_.bind(this));
is addEventListener('tap') the same as listen('tap')?

basically, is this still triggered for clicks and Enter from the keyboard?

if not, maybe try

this.listen(this.$$('paper-checkbox a'), 'tap', 'onShowReportedSettingsTap_');

[dpapad, 2017-03-01 20:37:49]

https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:80: 'tap',
this.onShowReportedSettingsTap_.bind(this));
On 2017/03/01 at 20:29:37, Dan Beam wrote:
> is addEventListener('tap') the same as listen('tap')?
> 
> basically, is this still triggered for clicks and Enter from the keyboard?
> 
> if not, maybe try
> 
> this.listen(this.$$('paper-checkbox a'), 'tap', 'onShowReportedSettingsTap_');

Yes, it is triggered either way. The problem I see though is that
stopPropagation() only seems to work when you use the mouse. If you hit "Enter"
the checkbox is still toggled. Ugh

[dpapad, 2017-03-01 20:42:59]

https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):

https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:80: 'tap',
this.onShowReportedSettingsTap_.bind(this));
On 2017/03/01 at 20:37:49, dpapad wrote:
> On 2017/03/01 at 20:29:37, Dan Beam wrote:
> > is addEventListener('tap') the same as listen('tap')?
> > 
> > basically, is this still triggered for clicks and Enter from the keyboard?
> > 
> > if not, maybe try
> > 
> > this.listen(this.$$('paper-checkbox a'), 'tap',
'onShowReportedSettingsTap_');
> 
> Yes, it is triggered either way. The problem I see though is that
stopPropagation() only seems to work when you use the mouse. If you hit "Enter"
the checkbox is still toggled. Ugh

Fixed by adding a listener for keydown, see latest patch.

[dpapad, 2017-03-01 21:58:44]

On 2017/03/01 at 20:42:59, dpapad wrote:
>
https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
(right):
> 
>
https://codereview.chromium.org/2722083003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/reset_page/reset_profile_dialog.js:80:
'tap', this.onShowReportedSettingsTap_.bind(this));
> On 2017/03/01 at 20:37:49, dpapad wrote:
> > On 2017/03/01 at 20:29:37, Dan Beam wrote:
> > > is addEventListener('tap') the same as listen('tap')?
> > > 
> > > basically, is this still triggered for clicks and Enter from the keyboard?
> > > 
> > > if not, maybe try
> > > 
> > > this.listen(this.$$('paper-checkbox a'), 'tap',
'onShowReportedSettingsTap_');
> > 
> > Yes, it is triggered either way. The problem I see though is that
stopPropagation() only seems to work when you use the mouse. If you hit "Enter"
the checkbox is still toggled. Ugh
> 
> Fixed by adding a listener for keydown, see latest patch.

FWIW, I also tried using
this.listen(this.$$('paper-checkbox a'), 'tap', 'onShowReportedSettingsTap_');
and calling e.detail.sourceEvent.stopPropagation(), but that caused clicking on
the link to toggle the checkbox. So, I think the combination of 'tap' and
'keydown' handler is our best option.

[Dan Beam, 2017-03-06 19:28:01]

still lgtm but as we talked about offline: this potentially means there are
issues other places in settings

[dpapad, 2017-03-06 20:12:26]

On 2017/03/06 at 19:28:01, dbeam wrote:
> still lgtm but as we talked about offline: this potentially means there are
issues other places in settings

Added an assertion to ensure that tapping on the link does not toggle the
checkbox. Will follow up with filing a bug for existing cases that don't work as
expected.

[dpapad, 2017-03-06 20:13:44]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-03-06 20:13:45]

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/2722083003/#ps100001
(title: "Add test.")

[commit-bot: I haz the power, 2017-03-06 20:14:28]

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, 2017-03-06 21:24:42]

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

[commit-bot: I haz the power, 2017-03-06 21:24:43]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dpapad, 2017-03-06 22:07:50]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-06 22:08:27]

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, 2017-03-06 23:44:02]

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

[commit-bot: I haz the power, 2017-03-06 23:44:04]

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_...)

[dpapad, 2017-03-07 00:11:29]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:12:36]

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, 2017-03-07 03:25:01]

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

[commit-bot: I haz the power, 2017-03-07 03:25:02]

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_...)

[dpapad, 2017-03-07 04:05:01]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-07 04:05:35]

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, 2017-03-07 06:56:43]

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

[commit-bot: I haz the power, 2017-03-07 06:56:45]

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_...)

[dpapad, 2017-03-07 18:06:06]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:08:32]

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, 2017-03-07 20:10:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488909966832140,
"parent_rev": "60588ea13e075f43e47cd97d6911b8c50ce5d579", "commit_rev":
"4f4441915d7f7dee6dd36fcf3e06e743f5779e2c"}

[commit-bot: I haz the power, 2017-03-07 20:10:47]

Description was changed from

==========
MD Settings: Change "view reported settings" string.

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

to

==========
MD Settings: Change "view reported settings" string.

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

Review-Url: https://codereview.chromium.org/2722083003
Cr-Commit-Position: refs/heads/master@{#455188}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f4441915d7f7dee6dd36fcf3e06...
==========

[commit-bot: I haz the power, 2017-03-07 20:10:50]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4f4441915d7f7dee6dd36fcf3e06...