History WebUI: Focus gets lost on pressing tab key once from search box. |
|||||||||
Issue descriptionThis is similar as issue 905999, but filing separately, because the fix will possibly be slightly different. Repro: 1) Go to chrome://history. The search box should be automatically focused. 2) Hit tab once. Focus goes on the scrolling container (it should not). 3) Hit tab again. Focus is now seen within the page contents (that's expected. Expected. 2 should not be happening. See more context at [1] Copying from "@hugoh: The tabindex -1 works for settings, because the scrolling container is a <div>, but it does not work for History, which has also regressed as a result of this change. In History's case, the scrolling container is a Custom Element, and because of issue 896624 (which per Blink team is an intentional spec change), adding tabindex -1 at [1] prevents anything within that Custom Element to be in the tab order." Also adding hayato@, since per his response at the way tabindex -1 works for Custom Elements is per spec. FWIW, to me it seems that there is a spec bug now. tabindex -1 behaves differently on a non-Custom Element (like a <div>) than it does on a Custom Element. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/md_history/app.html?l=92 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=896624#c13
,
Nov 20
I found some background in kochi@'s design doc] where he envisioned Custom Elements' tabIndex sematics being potentially confusing: https://docs.google.com/document/d/1k93Ez6yNSyWQDtGjdJJqTBPmljk9l2WS3JTe5OHHB50/ . I don't know much about Shadow DOM and Custom Elements, but I guess you're right; it would make more sense if Custom Elements and regular elements had the same tabIndex-sematics. I created: https://github.com/w3c/webcomponents/issues/774. Let's see if the spec can give us some feedback on this. Meanwhile, I'm afraid you need to prepare yet another workaround for Web UI. :/
,
Nov 20
@hugoh: Thanks for starting a thread about the spec. > Meanwhile, I'm afraid you need to prepare yet another workaround for Web UI. :/ @hugoh: As discussed at https://bugs.chromium.org/p/chromium/issues/detail?id=905604#c2, I think it is reasonable to revert this change for now, since 1) it introduces risk for regressions that will make it to the M72 branch 2) There is very little time to fix existing WebUI regressions, since this is a short week in the US (because of thanksgiving). 3) There is no clear fix either, since just adding tabindex -1 on Custom Elements does not work. My plan is to revert this tomorrow (Wednesday), before the Thanksgiving break. Then you can reland this change after the branch cut, and hopefully the spec thread has made some progress until then as well.
,
Nov 21
,
Nov 21
,
Nov 21
Thanks. Ok. Did you try delegatesFocus=true? From https://github.com/w3c/webcomponents/issues/554#issuecomment-243043720 it sounds like delegatesFocus=true could fix this bug?
,
Nov 21
From https://docs.google.com/document/d/1k93Ez6yNSyWQDtGjdJJqTBPmljk9l2WS3JTe5OHHB50/ about delegatesFocus=true: "If non-focusable area is clicked, the shadow host gets focus." So when a user clicks an "empty"/"white" area of your component with the intention to "unfocus" (hide the focus ring), your shadow host (a scrollable <div> in your case?) gets focus. => You will still need to style your shadow host with |outline: none|.
,
Nov 21
@hugoh: Yes, tried delegatesFocus, and it still does not fix it. See https://github.com/w3c/webcomponents/issues/774#issuecomment-440751548, specifically about the delegatesFocus case. Maybe Chrome has mis-interpreted the spec here?
,
Nov 21
Bulk update: The root cause of this regression has been reverted, see [1]. Leaving this bug open to ensure it gets addressed before re-landing the original Blink change. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=907284#c6
,
Nov 21
,
Nov 22
@dapapad, did you try delegatesFocus=true with tabIndex >= 0 on the host? That should eject the host from tab order: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/shadow-dom/focus-navigation-with-delegatesFocus.html?l=170 With KeyBoardFocusableScrollers, scrollers automatically get tabIndex=0 so you should be able to omit tabIndex altogether on your scrolling host and only set delegatesFocus=true on your shadow root.
,
Nov 28
@hugoh: Have not tried that. According to [1] Polymer 2 provides a way to control how the shadow root is constructed, but it is unclear whether that is also available to the legacy (hybrid syntax between P1 and P2). It is definitely worth trying. [1] https://polymer-library.polymer-project.org/2.0/docs/devguide/dom-template#create-your-own-shadow-root
,
Dec 19
Can no longer reproduce, even with KeyboardFocusableScrollers turned on.
,
Dec 19
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0e0ddb91c4c3bc545ab36a88b06561bff3be099 commit d0e0ddb91c4c3bc545ab36a88b06561bff3be099 Author: John Lee <johntlee@chromium.org> Date: Thu Dec 20 22:35:05 2018 History WebUI: Move sidebar and content within scrolling containers with tabindex of -1 and no outlines Bug: 906729 Change-Id: I11248b67f99cf6cfcc44d765ff9778ffcf6116b0 Reviewed-on: https://chromium-review.googlesource.com/c/1385852 Reviewed-by: Scott Chen <scottchen@chromium.org> Commit-Queue: John Lee <johntlee@chromium.org> Cr-Commit-Position: refs/heads/master@{#618357} [modify] https://crrev.com/d0e0ddb91c4c3bc545ab36a88b06561bff3be099/chrome/browser/resources/md_history/app.html [modify] https://crrev.com/d0e0ddb91c4c3bc545ab36a88b06561bff3be099/chrome/browser/resources/md_history/history_list.html [modify] https://crrev.com/d0e0ddb91c4c3bc545ab36a88b06561bff3be099/chrome/browser/resources/md_history/side_bar.html
,
Dec 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d368fbe5b9e12412672e7af28519c106837744c5 commit d368fbe5b9e12412672e7af28519c106837744c5 Author: John Lee <johntlee@chromium.org> Date: Fri Dec 21 19:21:22 2018 Revert "History WebUI: Move sidebar and content within scrolling containers with tabindex of -1 and no outlines" This reverts commit d0e0ddb91c4c3bc545ab36a88b06561bff3be099. Reason for revert: I'm going to revert until there's a more reliable way to do without breaking any existing functionality. Original change's description: > History WebUI: Move sidebar and content within scrolling containers with tabindex of -1 and no outlines > > Bug: 906729 > Change-Id: I11248b67f99cf6cfcc44d765ff9778ffcf6116b0 > Reviewed-on: https://chromium-review.googlesource.com/c/1385852 > Reviewed-by: Scott Chen <scottchen@chromium.org> > Commit-Queue: John Lee <johntlee@chromium.org> > Cr-Commit-Position: refs/heads/master@{#618357} TBR=scottchen@chromium.org,johntlee@chromium.org Change-Id: I2ce18c103aa245879abf538c728c8425bfa56ac5 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 906729 Reviewed-on: https://chromium-review.googlesource.com/c/1388745 Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Scott Chen <scottchen@chromium.org> Commit-Queue: John Lee <johntlee@chromium.org> Cr-Commit-Position: refs/heads/master@{#618569} [modify] https://crrev.com/d368fbe5b9e12412672e7af28519c106837744c5/chrome/browser/resources/md_history/app.html [modify] https://crrev.com/d368fbe5b9e12412672e7af28519c106837744c5/chrome/browser/resources/md_history/history_list.html [modify] https://crrev.com/d368fbe5b9e12412672e7af28519c106837744c5/chrome/browser/resources/md_history/side_bar.html
,
Jan 9
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by dpa...@chromium.org
, Nov 19