Verified
Status Update
No update yet.
Comments
ko...@chromium.org
rm...@chromium.org
[Empty comment from Monorail migration]
[Monorail components: -Mobile>WebView]
[Monorail components: -Mobile>WebView]
wa...@chromium.org
The flash is the tap highlight to indicate what is tapped. This can be reproduced on Linux with DevTools mobile emulation.
Yes, I believe it's caused by the CL. The CL made the focus ring area (which is also used to draw tap higlight) include out-of-flow descendants. The purpose was to let the focus ring enclose all clickable areas of an element, especially for <a href> element containing out-of-flow descendants, e.g.
<a href="...">
<div style="position: absolute; ...">Clicable</div>
</a>
To fix the problem:
1. visibility:hidden out-of-flow descendants should be ignored.
2. Descendants with a different touch-action should probably be excluded, or we should probably limit the out-of-flow-inclusion behavior to <a href> only.
penglin22@huawei.com are you interested in this bug?
[Monorail components: -Blink>Layout Blink>Paint]
Yes, I believe it's caused by the CL. The CL made the focus ring area (which is also used to draw tap higlight) include out-of-flow descendants. The purpose was to let the focus ring enclose all clickable areas of an element, especially for <a href> element containing out-of-flow descendants, e.g.
<a href="...">
<div style="position: absolute; ...">Clicable</div>
</a>
To fix the problem:
1. visibility:hidden out-of-flow descendants should be ignored.
2. Descendants with a different touch-action should probably be excluded, or we should probably limit the out-of-flow-inclusion behavior to <a href> only.
penglin22@huawei.com are you interested in this bug?
[Monorail components: -Blink>Layout Blink>Paint]
pe...@gmail.com
Ok,I will look into it.
wa...@chromium.org
[Empty comment from Monorail migration]
pe...@gmail.com
Descendants with a different touch-action should probably be excluded, or we should probably limit the out-of-flow-inclusion behavior to <a href> only.
-->Why do you think this behavior should be limitted to <a href> only? This fixhttps://chromium-review.googlesource.com/c/chromium/src/+/3568765 looks like applying to all out-of-flow descendants cases.I checked firefox and safari,neither of them draw these descendants.I wander if we really need to do this.
-->Why do you think this behavior should be limitted to <a href> only? This fix
wa...@chromium.org
<a href> is special because it makes all descendants part of the link. Showing focus ring and touch highlight covering the whole link gives users better feedback on which area is clickable and clicked.
However, I just found that the case inhttps://crbug.com/chromium/1380673#c4 doesn't actually work. https://chromium-review.googlesource.com/c/chromium/src/+/3568765 doesn't actually add outline rects of out-of-flow descendants of <a href>. It only works in particular cases, and broke duckduckgo.com . I think we should make the <a href> case work, and avoid extra outline rects for out-of-flow descendants for other cases.
> I checked firefox and safari,neither of them draw these descendants.
What do you mean by "draw these descendants"? Is it about the focus ring, outline and/or touch highlight? Which test case did you use?
However, I just found that the case in
> I checked firefox and safari,neither of them draw these descendants.
What do you mean by "draw these descendants"? Is it about the focus ring, outline and/or touch highlight? Which test case did you use?
pe...@gmail.com
> I checked firefox and safari,neither of them draw these descendants.
What do you mean by "draw these descendants"? Is it about the focus ring, outline and/or touch highlight? Which test case did you use?
->Sorry to make this ambiguous.I mean neither safari and firefox draw the out-of-flow descendants for this casehttps://messy-excessive-ringer.glitch.me (safari will draw an extra outline which may be a bug.).
Let's make this issue clear.Do you mean only cases like
<a href="#" style="display:block; outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
or
<a href="#" style="outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
should draw outline rects of the out-of-flow descendants but others should be excluded?
What do you mean by "draw these descendants"? Is it about the focus ring, outline and/or touch highlight? Which test case did you use?
->Sorry to make this ambiguous.I mean neither safari and firefox draw the out-of-flow descendants for this case
Let's make this issue clear.Do you mean only cases like
<a href="#" style="display:block; outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
or
<a href="#" style="outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
should draw outline rects of the out-of-flow descendants but others should be excluded?
wa...@chromium.org
> ... should draw outline rects of the out-of-flow descendants but others should be excluded?
Yes.
We also need to fix the bug about visibility:hidden and !IsContentVisibilityVisible() descendants.
FYI, below is some background about the focus rings.
We have two kinds of outlines:
1. Normal css outlines. We need to follow the css-ui spec and keep compatibility among browsers.
2. Focus rings. This is not strictly defined in the spec. It's a browser UI feature, and the browser can implement it in any proper way to give users feedback about the interactive areas of an element. We think the focus ring should cover all interactive areas belonging to the element, e.g. out-of-flow descendants of <a href>.
For #2, firefox doesn't use any css properties, while Safari and Chrome use css outline:auto. In LayoutObject::OutlineRects(), the NGOutlineType parameter indicates whether we want a standard outline or a focus ring.
Touch highlight is also a browser UI feature, and we want it to give the same user feedback as focus rings.
Forhttps://chromium-review.googlesource.com/c/chromium/src/+/3568765 , our purpose was to support the <a href> with OOF descendant case. As the CL didn't achieve what we want, I think we should revert it first.
Yes.
We also need to fix the bug about visibility:hidden and !IsContentVisibilityVisible() descendants.
FYI, below is some background about the focus rings.
We have two kinds of outlines:
1. Normal css outlines. We need to follow the css-ui spec and keep compatibility among browsers.
2. Focus rings. This is not strictly defined in the spec. It's a browser UI feature, and the browser can implement it in any proper way to give users feedback about the interactive areas of an element. We think the focus ring should cover all interactive areas belonging to the element, e.g. out-of-flow descendants of <a href>.
For #2, firefox doesn't use any css properties, while Safari and Chrome use css outline:auto. In LayoutObject::OutlineRects(), the NGOutlineType parameter indicates whether we want a standard outline or a focus ring.
Touch highlight is also a browser UI feature, and we want it to give the same user feedback as focus rings.
For
wa...@chromium.org
[Empty comment from Monorail migration]
pe...@gmail.com
Got it.Thanks for your information.
pe...@gmail.com
wa...@chromium.org
I think it's in the right direction. About HasAnchorContainerOutlineNode(), I'd expect a new NGOutlineType for anchor instead to avoid the cost of the function.
pe...@gmail.com
The CL has been blocked for some time and until now it may be different from the original purpose.Only cases like
<a href="#" style="position: relative; outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
and
<a href="#" style="position: absolute; outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
will draw focus ring for the out-of-flow descendants.
On the other hand, It still can't pass the Mac platform web tests,could you please help to take a look? thanks.
<a href="#" style="position: relative; outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
and
<a href="#" style="position: absolute; outline: red auto thin;" >test
<div style="position: absolute; ">Clicable</div>
</a>
will draw focus ring for the out-of-flow descendants.
On the other hand, It still can't pass the Mac platform web tests,could you please help to take a look? thanks.
wa...@chromium.org
I'm on vacation now. Will look into this next week.
[Deleted User] <[Deleted User]>
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src/+/b52d6acca840fc1d8a07feabcc9b49943048c5ea
commit b52d6acca840fc1d8a07feabcc9b49943048c5ea
Author: lin <penglin22@huawei.com>
Date: Tue Jan 03 09:47:29 2023
Add focus rings for A tag oof descendants
Add focus rings for all A tag out-of-flow position descendants and
exclude others'.
Delete transform-focus-ring-repaint.html test file which has been
merged into focus-ring-for-anchor1.html
Bug: 1380673
Change-Id: Ia3edf307e2e646a03e43a531f9ef71e027bdd657
Reviewed-on:https://chromium-review.googlesource.com/c/chromium/src/+/4037172
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: lin peng <penglin22@huawei.com>
Cr-Commit-Position: refs/heads/main@{#1088147}
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/renderer/core/layout/ng/ng_outline_type.h
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/FlagExpectations/disable-layout-ng
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/paint/invalidation/outline/outline-auto-descendant-transform.html
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.cc
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/renderer/core/layout/layout_block.cc
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/renderer/core/layout/layout_block_flow.cc
[delete]https://crrev.com/ace1ab9f407fe1db0dcdb37cafe7998d015ef4da/third_party/blink/web_tests/platform/fuchsia/paint/invalidation/outline/focus-layers-expected.png
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/flag-specific/highdpi/paint/invalidation/outline/focus-layers-expected.png
[delete]https://crrev.com/ace1ab9f407fe1db0dcdb37cafe7998d015ef4da/third_party/blink/web_tests/paint/invalidation/outline/transform-focus-ring-repaint-expected.html
[add]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/paint/invalidation/outline/focus-ring-for-anchor.html
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/renderer/core/layout/ng/ng_physical_fragment.cc
[add]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/paint/invalidation/outline/focus-ring-for-anchor-expected.html
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/platform/mac/paint/invalidation/outline/focus-layers-expected.png
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/paint/invalidation/outline/focus-ring-on-child-move-expected.txt
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/platform/win/paint/invalidation/outline/focus-layers-expected.png
[delete]https://crrev.com/ace1ab9f407fe1db0dcdb37cafe7998d015ef4da/third_party/blink/web_tests/paint/invalidation/outline/transform-focus-ring-repaint.html
[add]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/paint/invalidation/outline/focus-ring-for-anchor1.html
[add]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/paint/invalidation/outline/focus-ring-for-anchor1-expected.html
[delete]https://crrev.com/ace1ab9f407fe1db0dcdb37cafe7998d015ef4da/third_party/blink/web_tests/paint/invalidation/outline/transform-focus-ring-repaint-expected.txt
[modify]https://crrev.com/b52d6acca840fc1d8a07feabcc9b49943048c5ea/third_party/blink/web_tests/platform/linux/paint/invalidation/outline/focus-layers-expected.png
commit b52d6acca840fc1d8a07feabcc9b49943048c5ea
Author: lin <penglin22@huawei.com>
Date: Tue Jan 03 09:47:29 2023
Add focus rings for A tag oof descendants
Add focus rings for all A tag out-of-flow position descendants and
exclude others'.
Delete transform-focus-ring-repaint.html test file which has been
merged into focus-ring-for-anchor1.html
Bug: 1380673
Change-Id: Ia3edf307e2e646a03e43a531f9ef71e027bdd657
Reviewed-on:
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: lin peng <penglin22@huawei.com>
Cr-Commit-Position: refs/heads/main@{#1088147}
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[delete]
[modify]
[delete]
[add]
[modify]
[add]
[modify]
[modify]
[modify]
[delete]
[add]
[add]
[delete]
[modify]
he...@google.com
CL in https://crbug.com/chromium/1380673#c18 breaks paint/invalidation/outline/focus-layers.html on fuchsia bots:
https://ci.chromium.org/ui/p/chromium/builders/ci/fuchsia-x64-rel/2417/test-results?sortby=&groupby=
https://ci.chromium.org/ui/p/chromium/builders/ci/fuchsia-x64-cast-receiver-rel/2322/test-results?sortby=&groupby=
IIUC it also deletes third_party/blink/web_tests/platform/fuchsia/paint/invalidation/outline/focus-layers-expected.png instead of modifying it as with the other platforms. That might already be the issue.
IIUC it also deletes third_party/blink/web_tests/platform/fuchsia/paint/invalidation/outline/focus-layers-expected.png instead of modifying it as with the other platforms. That might already be the issue.
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src/+/eb0da188b94d5fa92e5618174a0ce339800797f1
commit eb0da188b94d5fa92e5618174a0ce339800797f1
Author: Alexander Hendrich <hendrich@chromium.org>
Date: Tue Jan 03 12:15:11 2023
Revert "Add focus rings for A tag oof descendants"
This reverts commit b52d6acca840fc1d8a07feabcc9b49943048c5ea.
Reason for revert:https://crbug.com/1380673#c19
Original change's description:
commit eb0da188b94d5fa92e5618174a0ce339800797f1
Author: Alexander Hendrich <hendrich@chromium.org>
Date: Tue Jan 03 12:15:11 2023
Revert "Add focus rings for A tag oof descendants"
This reverts commit b52d6acca840fc1d8a07feabcc9b49943048c5ea.
Reason for revert:
Original change's description:
Bug: 1380673
Change-Id: Ieec32ef3f9515cf48672ec09e957cee3df5f1c3a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on:
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Alexander Hendrich <hendrich@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Alexander Hendrich <hendrich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1088176}
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[add]
[modify]
[add]
[delete]
[modify]
[delete]
[modify]
[modify]
[modify]
[delete]
[add]
[delete]
[add]
[modify]
pe...@gmail.com
It's all done by rebaseline-cl and I found rebaseline-cl will not generate png expected files for fuchsia.What can I do to fix this?
wa...@chromium.org
pe...@gmail.com
Do you mean https://chromium-review.googlesource.com/c/chromium/src/+/4131459?Sorry,the third_party/blink/web_tests/platform/fuchsia/paint/invalidation/outline/focus-layers-expected.png is just a duplicated one from .../platform/linux/xxx. It looks like rebaseline will not generate expected files for fuchsia and still failed with the baseline from https://ci.chromium.org/ui/p/chromium/builders/try/fuchsia-x64-rel/898/overview .
wa...@chromium.org
Sorry for the wrong link. I only checked the fuchsia job was green. Rebaseline-cl only rebaselines a test if it fail with patch and succeeds without patch. The test succeeded without patch on the bot so rebaseline-cl didn't rebseline it. I'm not sure why this happened. As a workaround, you can try to manually download the actual image and save it as platform/fuchsia/...-expected.png.
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src/+/0b53a4a06796da58ebe051fc4b18ba057392dac6
commit 0b53a4a06796da58ebe051fc4b18ba057392dac6
Author: lin <penglin22@huawei.com>
Date: Wed Jan 04 10:17:33 2023
Reland "Add focus rings for A tag oof descendants"
This is a reland of commit b52d6acca840fc1d8a07feabcc9b49943048c5ea
Reason for revert: It breaks
paint/invalidation/outline/focus-layers.html on fuchsia bots.
Add the third_party/blink/web_tests/platform/fuchsia/paint/invalidation/outline/focus-layers-expected.png as with the other platforms do.
Original change's description:
commit 0b53a4a06796da58ebe051fc4b18ba057392dac6
Author: lin <penglin22@huawei.com>
Date: Wed Jan 04 10:17:33 2023
Reland "Add focus rings for A tag oof descendants"
This is a reland of commit b52d6acca840fc1d8a07feabcc9b49943048c5ea
Reason for revert: It breaks
paint/invalidation/outline/focus-layers.html on fuchsia bots.
Add the third_party/blink/web_tests/platform/fuchsia/paint/invalidation/outline/focus-layers-expected.png as with the other platforms do.
Original change's description:
Bug: 1380673
Change-Id: I26b62ed69f70e569766f20f93b2d80b5da6a8907
Reviewed-on:
Commit-Queue: lin peng <penglin22@huawei.com>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1088644}
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[add]
[delete]
[modify]
[modify]
[add]
[modify]
[modify]
[delete]
[add]
[delete]
[add]
[modify]
we...@google.com
Re https://crbug.com/chromium/1380673#c24 , I don't know what is happening either. Not sure if sshrimp@ has any suggestion for how to debug this.
wa...@chromium.org
Correction to https://crbug.com/chromium/1380673#c24 : "The test succeeded without patch on the bot so rebaseline-cl didn't rebseline it" should be "The test failed with and without patch on the bot so rebaseline-cl didn't rebaseline it".
al...@chromium.org
Are any of these updates causing the weird presubmit errors I'm getting locally? See https://crbug.com/chromium/1405030 .
If I touch a difrerent file then git cl presubmit -u gives me:
** Presubmit ERRORS: 1 **
disable-layout-ng.orig:1565 Test does not exist: paint/invalidation/outline/transform-focus-ring-repaint.html
If I touch a difrerent file then git cl presubmit -u gives me:
** Presubmit ERRORS: 1 **
disable-layout-ng.orig:1565 Test does not exist: paint/invalidation/outline/transform-focus-ring-repaint.html
pe...@gmail.com
paint/invalidation/outline/transform-focus-ring-repaint.html has been deleted in https://chromium-review.googlesource.com/c/chromium/src/+/4131459.I have not got your error after rebasing my code.
al...@chromium.org
@penglin220, I pulled + rebased and still get the same error.
wa...@chromium.org
aleventhal@ I think you can remove **/disable-layout-ng.orig to fix the issue. The file is not in git, but it's still checked by presubmit.
weizhong@ we should probably ignore non-git files in presubmit.
weizhong@ we should probably ignore non-git files in presubmit.
we...@google.com
OK, looks we have such issue for both FlagExpectations and SmokeTests folder. We can add the folder name(relative to web_tests) to the log, and check if that file is tracked by git or not, with command "git ls-files --error-unmatch <file name>".
The presubmit check did not correctly check if a wpt test exists or not, due to this TODO:https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/web_tests/port/base.py;drc=5ae6355eda118fe7c96d002d4b6baf8961114d36;l=1328
The presubmit check did not correctly check if a wpt test exists or not, due to this TODO:
pd...@chromium.org
[Empty comment from Monorail migration]
lu...@appspot.gserviceaccount.com
Because another bug was merged into this bug, LUCI Analysis has merged the failure association rule for that bug into the rule for this bug.
See failure impact and configure the failure association rule for this bug at:https://luci-analysis.appspot.com/b/chromium/1380673
See failure impact and configure the failure association rule for this bug at:
we...@google.com
[Empty comment from Monorail migration]
[Monorail components: Blink>Infra]
[Monorail components: Blink>Infra]
lu...@appspot.gserviceaccount.com
Because:
- Presubmit-Blocking Failures Exonerated (7-day) < 1, and
- Presubmit Runs Failed (7-day) < 1
LUCI Analysis is marking the issue verified.
Why issues are verified and how to stop automatic verification:https://luci-analysis.appspot.com/help#bug-verified
See failure impact and configure the failure association rule for this bug at:https://luci-analysis.appspot.com/b/chromium/1380673
- Presubmit-Blocking Failures Exonerated (7-day) < 1, and
- Presubmit Runs Failed (7-day) < 1
LUCI Analysis is marking the issue verified.
Why issues are verified and how to stop automatic verification:
See failure impact and configure the failure association rule for this bug at:
ac...@chromium.org
[Empty comment from Monorail migration]
[Monorail components: Mobile>WebView]
[Monorail components: Mobile>WebView]
is...@google.com
This issue was migrated from crbug.com/chromium/1380673?no_tracker_redirect=1
[Multiple monorail components: Blink>Infra, Blink>Paint, Mobile>WebView]
[Monorail mergedwith:crbug.com/chromium/1404714 ]
[Monorail components added to Component Tags custom field.]
[Multiple monorail components: Blink>Infra, Blink>Paint, Mobile>WebView]
[Monorail mergedwith:
[Monorail components added to Component Tags custom field.]
Description
Device name/ Android version: Pixel 2 XL/RP1A.200720.009, Moto X4 9.0/PPW29.69-26, Samsung Galaxy S8 +(SM-G955FZVDINS)/ OPM1.180608.001.
WebView version: 109.0.5395.0
Test URL:https://duckduckgo.com/
Reproducible on Chrome App: Dev
Chrome Channel and Version if Yes: Dev(109.0.5395.0)
Per-Version bisect information:
Good build: 102.0.5003.0
Bad build: 102.0.5004.0
Regression range:https://chromium.googlesource.com/chromium/src/+log/102.0.5003.0..102.0.5004.0?pretty=fuller&n=10000
Steps to reproduce:
(1) Launch WebView shell browser > Enter Test URL
(2) Tap on Speaker symbol > Observe
Actual Result: Unwanted Flash observed around Dropdown menu.
Expected Result: No such behavior should be found.
Frequency:5/5
Note: Issue is able to repro on Third Party Browsers as well(Ex: Dolphin, Opera mini).