New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 896624 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression

Blocking:
issue 896748



Sign in to add a comment

Regression: Shadow DOM v1 tabindex -1 behavior different than v0, causing WebUI regressions.

Reported by sanyam.g...@etouch.net, Oct 18

Issue description

Chrome version: 72.0.3584.0 (Official Build)Revision 945ab041ec6ac3fb389f94dcbfebb1839a12a69b-refs/branch-heads/3584@{#1}(32/64-bit)
OS: Windows (7, 8, 8.1, 10), Mac(10.13.1, 10.13.6, 10.14) & Linux(14.04 LTS)

What steps will reproduce the problem?
1. Launch chrome, navigate to chrome://settings/content/notifications
2. Now click anywhere on page and press tab key to traverse focus and observe.

Actual Result  : Focus does not traverse to ‘Iron’ icon after add button.
Expected Result: Focus should traverse to ‘Iron’ icon.

This is regression issue broken in ‘M-72’ and will soon update other info:
Good build: 72.0.3583.0 (Revision: 600164)
Bad build : 72.0.3584.0 (Revision: 600616)
 
Actual_Result.mov
1.2 MB View Download
Expected_Result.mov
2.7 MB View Download
Labels: hasbisect
Owner: dpa...@chromium.org
Status: Assigned (was: Unconfirmed)
Update :

Chromium bisect info:

You are probably looking for a change made after 600223 (known good), but no later than 600228 (first known bad).

CHANGE-LOG URL:
  
https://chromium.googlesource.com/chromium/src/+log/62f2f8f209a82db0dd5a311eb4f844bddb75c98e..631a939b2b9a02a05cd1db367c7ea7fb17d14f50

Suspecting: https://chromium.googlesource.com/chromium/src/+/177774d57a744b75630cba5507c3bf1ed419aea3

@dpapad: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Note:
1.Tried per revision bisect on Windows ,Mac and Linux OS but unable to perform the same since getting "RuntimeError: We don't have enough builds to bisect" error 
2.Hence provided suspect through 'Chromium bisect'

Thank You

Blocking: 896748
Cc: rbpotter@chromium.org
Labels: Pri-2
Owner: ----
Status: Available (was: Assigned)
I am able to reproduce this. No lead on what's happening yet.
Cc: tkent@chromium.org scottchen@chromium.org
Components: Blink>DOM>ShadowDOM
Summary: Regression: Shadow DOM v1 tabindex behavior different than v0, causing WebUI regressions. (was: Regression: Focus does not traverse to Iron-icon under chrome://settings/content/notifications.)
This actually affects FocusRowBehavior and therefore regresses focus on various places of the UI, not just in chrome://settings/content/notifications.

After some investigation with the help of scottchen@, we concluded that this is a difference between Shadow DOM v0 and v1. Minimal repro below.

Shadow DOM v0: https://jsfiddle.net/2n0khjqc/2/, a child element is focusable even though it's parent has tabindex -1.

Shadow DOM v1: https://jsfiddle.net/tbyp9hqg/2/, a child element is *not* focusable when the parent has tabindex -1.

@tkent: Any thoughts on which behavior is the correct one? We would need to come up with some workaround, if Shadow DOM v1's behavior is expected.


Cc: dpa...@chromium.org
 Issue 897013  has been merged into this issue.
Summary: Regression: Shadow DOM v1 tabindex -1 behavior different than v0, causing WebUI regressions. (was: Regression: Shadow DOM v1 tabindex behavior different than v0, causing WebUI regressions.)
as a hack, I tried passing delegatesFocus inside of Polymer (by munging Element.prototype.createShadow) here:

https://jsfiddle.net/tbyp9hqg/3/

it made it such that *clicking* on the button allowed focus, but not keyboard

i may have tickled this line:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/element.cc?sq=package:chromium&g=0&l=3442

but didn't have time to follow the turtles the whole way down (but i could be convinced to add some logs inside blink to track down further if needed)
Cc: aee@chromium.org
IIUC, delegateFocus simply affects how :focus pseudo selector works. If delegateFocus is true, then :focus matches, even if a child is focused. In other words it basically makes :focus work similarly to how :focus-within works. But it does not affect "Tab" key behavior.
Cc: hayato@chromium.org
@tkent, @hayato: Could you take a look #5? We need to determine if we should come up with a work-around on our side, or if this needs to be fixed within Blink itself.
Cc: rakina@chromium.org
Components: Blink>HTML>Focus
We don't have a good owner for focus navigation after kochi@ left.
Anyway, let me take a look soon.

@rakina, could you take over an ownership of focus related things from kochi@?
Confirmed: There *is* a difference between Shadow DOM v0 and v1, which is the expected behavior, as per https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

Actually, there is a layout test for the current Shadow DOM v1' behavior; test5() in  https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/shadow-dom/focus-navigation-with-delegatesFocus.html?q=focus-navigation-with&sq=package:chromium&g=0&l=106

So this is a intentional different behavior.
We need a work-around on UI side, instead of Blink side.


@hayato: Can you explain a bit the difference between delegateFocus false/true (test6() in the link)?

Would we be able to restore the previous functionality by using delegateFocus: true?
> @hayato: Can you explain a bit the difference between delegateFocus false/true (test6() in the link)?

It looks there is no behavior change between in test5() and test6(), where tabindex=-1 is set.
The whole shadow tree is skipped whether delegatesFocus is true or not if tabindex=-1.


Cc: -rbpotter@chromium.org
Owner: rbpotter@chromium.org
Status: Started (was: Available)
Cc: phanindra.mandapaka@chromium.org rbpotter@chromium.org
 Issue 901841  has been merged into this issue.
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 15

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

commit 13dfcd6d6ab09656870c8fba537230d82f7c21de
Author: rbpotter <rbpotter@chromium.org>
Date: Thu Nov 15 22:22:35 2018

History and Settings: Remove tabIndex = -1

Setting tabIndex=-1 no longer allows child elements to be focusable in
Shadow DOM v1 (see bug for additional context).

Remove logic that does this, and in order to prevent focusing of the
parent element when traversing backwards from the first item in the
focus row, listen for shift+tab on this element.

Bug:  896624 
Change-Id: I35e871863418e7bca57f648713b8d172677a949c
Reviewed-on: https://chromium-review.googlesource.com/c/1325352
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608546}
[modify] https://crrev.com/13dfcd6d6ab09656870c8fba537230d82f7c21de/chrome/browser/resources/md_history/history_item.js
[modify] https://crrev.com/13dfcd6d6ab09656870c8fba537230d82f7c21de/chrome/browser/resources/settings/focus_row_behavior.js
[modify] https://crrev.com/13dfcd6d6ab09656870c8fba537230d82f7c21de/chrome/browser/resources/settings/site_settings/site_entry.js

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 15

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

commit 292d851b752f8a97a347ecdf8e1f412afe9bcc37
Author: rbpotter <rbpotter@chromium.org>
Date: Thu Nov 15 22:53:22 2018

Settings and History: Track when items are blurred

When the tab index for the parent element is never set to -1, if focus
leaves the list of items and then returns to it, the focus will
automatically target the most recently focused control. Instead, if
focus leaves the list, focus should return to the first control on the
most recently focused item.

Bug:  896624 
Change-Id: I0e33e099f3b15b840d942d23fdccd620810b0806
Reviewed-on: https://chromium-review.googlesource.com/c/1336657
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608562}
[modify] https://crrev.com/292d851b752f8a97a347ecdf8e1f412afe9bcc37/chrome/browser/resources/md_history/history_item.js
[modify] https://crrev.com/292d851b752f8a97a347ecdf8e1f412afe9bcc37/chrome/browser/resources/settings/focus_row_behavior.js

Labels: TE-Verified-M72 TE-Verified-72.0.3612.0
Update: 

Rechecked the above issue on Windows (7, 8, 8.1, 10), Mac(10.13.1, 10.13.6, 10.14.2) & Linux(14.04 LTS) OS using latest Canary build #72.0.3612.0 and the issue is fixed. 

Kindly refer the attached screen-cast for reference.
Thank you..!
Fixed_Behaviour.mov
1.5 MB View Download
Status: Fixed (was: Started)

Sign in to add a comment