New issue
Advanced search Search tips

Issue 875470 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 875443



Sign in to add a comment

CrElements: Fix Polymer 2 interactive_ui_tests failures

Project Member Reported by rbpotter@chromium.org, Aug 17

Issue description

The following tests fail when run with --enable-features=WebUIPolymer2:

Only with optimize_webui=true:

CrElementsActionMenuTest.All
CrElementsInputTest.All

Flaky on debug builds (will likely need to repeat tests in order to observe failures):

CrElementsCheckboxTest.All
CrElementsProfileAvatarSelectorFocusTest.All
CrElementsToggleTest.All
 
Blocking: 875443
Candidate CL for CrElementsActionMenuTest.All is at https://chromium-review.googlesource.com/c/chromium/src/+/1208454, although it is not very clear yet what the difference between Shadow DOM v0 and v1 is (which is causing the failure).
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fccfd62a8f0fd29eb9142d6984b97ac952d42067

commit fccfd62a8f0fd29eb9142d6984b97ac952d42067
Author: dpapad <dpapad@chromium.org>
Date: Wed Sep 05 23:09:30 2018

WebUI: Make CrElementsActionMenuTest.All pass in Polymer 2.

The semantics of document.activeElement and someElement.shadowRoot.activeElement
seem to have changed between Shadow DOM v0 and v1.

In V1 the following is possible:
someElement.shadowRoot.activeElement is null AND document.activeElement points to a child
belonging to |someElement|.

which does not seem to be the case for Shadow DOM v0.

Always recursing from document.activeElement seems more robust and works for both cases.

Bug:  875470 
Change-Id: I53044c23bcfcfa85264b5154e635e44c05ce0f6f
Reviewed-on: https://chromium-review.googlesource.com/1208454
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589049}
[modify] https://crrev.com/fccfd62a8f0fd29eb9142d6984b97ac952d42067/chrome/test/data/webui/cr_elements/cr_action_menu_test.js
[modify] https://crrev.com/fccfd62a8f0fd29eb9142d6984b97ac952d42067/testing/buildbot/filters/webui_polymer2_interactive_ui_tests.filter
[modify] https://crrev.com/fccfd62a8f0fd29eb9142d6984b97ac952d42067/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
[modify] https://crrev.com/fccfd62a8f0fd29eb9142d6984b97ac952d42067/ui/webui/resources/js/util.js

FYI started looking at CrElementsInputTest.All. The failure indicates an actual problem with the prod code (notice that focusing a cr-input with the mouse does not show the blue underline in Polymer 2). Somehow the listener at [1] never executes.

[1] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_input/cr_input.js?l=153
Owner: dpa...@chromium.org
Status: Started (was: Untriaged)
The ability to add lisetners used |elementId.eventName| syntax as done in [1] has been removed according to [2]. This explains the failure, and we should probably audit all other elements for similar pattern.

[1] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_input/cr_input.js?l=153-156
[2] https://www.polymer-project.org/2.0/docs/upgrade#removed-apis
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/142d451318f046a97c9df1c2459226f1f0e95afd

commit 142d451318f046a97c9df1c2459226f1f0e95afd
Author: dpapad <dpapad@chromium.org>
Date: Sat Sep 08 00:23:44 2018

WebUI: Fix cr-input to work with Polymer 2.

In Polymer 2 the following syntax for registering event listeners
is no longer supported.

listeners: {
  'id.eventName': 'onEventName_',
},

Instead the listener must be added either programmatically, or
declaratively directly on the |id| element.

Bug:875470

Change-Id: I84f1d61b9b669dda1922f6c8d5f2bbc8de211c1c
Reviewed-on: https://chromium-review.googlesource.com/1213380
Reviewed-by: Esmael El-Moslimany <aee@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589732}
[modify] https://crrev.com/142d451318f046a97c9df1c2459226f1f0e95afd/testing/buildbot/filters/webui_polymer2_interactive_ui_tests.filter
[modify] https://crrev.com/142d451318f046a97c9df1c2459226f1f0e95afd/ui/webui/resources/cr_elements/cr_input/cr_input.html
[modify] https://crrev.com/142d451318f046a97c9df1c2459226f1f0e95afd/ui/webui/resources/cr_elements/cr_input/cr_input.js

Cc: rbpotter@chromium.org
@rbpotter: The remaning test mentioned in the opening comment seem to be enabled already, see [1]. Were they originally disabled, or were they always flaky but enabled?

Trying to determine if I can mark this bug as Fixed.

[1] https://cs.chromium.org/chromium/src/testing/buildbot/filters/webui_polymer2_interactive_ui_tests.filter?l=7,9-10
Status: Fixed (was: Started)
Yes, it appears those flaky tests were actually only flaky on a Windows bot that does not run interactive_ui_tests either, so I re-enabled them after pulling all the Polymer 2 interactive_ui_tests off of that bot. See https://crrev.com/c/1187729.

I think this can now be marked as fixed since the 2 other failing tests are fixed.

Sign in to add a comment