Issue metadata
Sign in to add a comment
|
Regression: Unnecessary trace of highlight is seen on ‘MD History’ page.
Reported by
dmascare...@etouch.net,
May 5 2016
|
||||||||||||||||||||||
Issue descriptionChrome Version:52.0.2725.0 (Official Build) 3bfc96772d01d42a50683cae507a53cf07bb606d-refs/heads/master@{#391707} 32/64 bit OS: Windows (7,8,10) , Linux Pre-condition: 1. Enable ‘Material Design History’ flag. 2. Navigate to any 2-3 webpages such that history is generated. What steps will reproduce the problem? 1. Launch chrome and navigate to ‘chrome://history' 2. Click on first check box of the page and press ‘Tab’ such that focus travel over every link. 3. Observe trace beside the links Actual: Unnecessary traces of highlight is seen. Expected: Traces should not be seen. This is regression issue, broken in ‘M 52’ and below is narrow bisect: https://chromium.googlesource.com/chromium/src/+log/74d1a68fa838d973b6104ddb4af4d88881fb901c..de7ae34361ba945b6d1ec7b3c679fecf5d94d35b?pretty=fuller&n=100 Suspecting: r387862 ? Good build: 52.0.2711.0 Bad build: 52.0.2712.0 Note: Above issue is not reproducible on Mac OS.
,
May 5 2016
Marking the above issue as RB-Stable as this is a recent regression. Feel free to change accordingly. Thank you!
,
May 8 2016
@wangxianzhu: when you tab on to the #title in this test case it gets a focus ring, even though there's an overflow clip that should prevent it:
<!DOCTYPE HTML>
<div style="display:inline-block; height: 20px; width: 20px;" tabindex=0></div>
<div id="test" style="overflow:hidden;">
<a id="title" style="display: inline-block;" href="">TEXT</a>
</div>
What do you think, should we display the focus ring or not?
Do you happen to know what is allowing the ring to paint despite the overflow clip? I can't see what prevents it getting treated like a normal outline (which does get clipped).
,
May 8 2016
In WebKit/blink focus-rings are just special outlines. They are clipped by overflow clip like normal outlines. How does you change affect this?
,
May 9 2016
Strangely enough they're not. Try putting a normal outline on #title in my example above - it gets clipped. However a focus ring does not. Any idea why? (FF clips them, not clipping them seems like a bug.) My change bumps up against this because we're now relying on paint invalidations to clean up the highlight. Because it's not getting clipped when it ought to be the paint invalidation doesn't know it's there and needs cleaning up.
,
May 9 2016
This doesn't look like an under-invalidation because we actually invalidate rasterization per tile which is normally bigger than the invalidation rect and it's unlikely to just leave all outsets of the focus ring not invalidated. Just did some debug and found we just miss the clipping. With your change reverted, we do output the clipping. It seems that with your change we miss updating of ancestor contentsVisualOverflowRect() when a descendant changes visual overflow (where 'ancestor' is the 'div id="test"' and 'descendant' is the 'a' element in the test case). In BoxClipper.cpp, we output clipping only if the contents really overflows the container. The clipping is missing because the contentsVisualOverflowRect() is wrong.
,
May 9 2016
So the focus ring shouldn't be clipped out in the first place? Like in FF?
,
May 9 2016
It should be clipped. It's not clipped because we miss updating ancestor contentsVisualOverflowRect() when descendant changes overflow.
,
May 13 2016
Friendly Ping. Request someone to update on the above issue? I really appreciate the help. Thank you!
,
May 13 2016
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed8035039a5d5a4424ae8f11d80d7b20ebb92537 commit ed8035039a5d5a4424ae8f11d80d7b20ebb92537 Author: robhogan <robhogan@gmail.com> Date: Tue May 17 19:22:50 2016 Clear knownToHaveNoOverflow() for line boxes when recomputing overflow without layout knownToHaveNoOverflow() typically gets reset during layout. When we're just recomputing overflow without a layout we have to clear it ourselves on any lineboxes that we identify as needing overflow recalculated, otherwise computeOverflow() on the linebox returns early. BUG= 609414 Review-Url: https://codereview.chromium.org/1980733002 Cr-Commit-Position: refs/heads/master@{#394186} [add] https://crrev.com/ed8035039a5d5a4424ae8f11d80d7b20ebb92537/third_party/WebKit/LayoutTests/fast/repaint/inline-box-overflow-repaint-expected.html [add] https://crrev.com/ed8035039a5d5a4424ae8f11d80d7b20ebb92537/third_party/WebKit/LayoutTests/fast/repaint/inline-box-overflow-repaint.html [modify] https://crrev.com/ed8035039a5d5a4424ae8f11d80d7b20ebb92537/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
,
May 18 2016
Still able to reproduce the issue on windows 7, Linux Ubuntu 14.04 using chrome latest version 52.0.2740.0.Able to see the traces of highlight before the links. robhoga@ Could you please find the attached screen cast and confirm any other fix yet to land. Thanks,
,
May 20 2016
Tested the same, along with kavvaru@(from C#14) and this seems to be working fine on the latest canary(52.0.2743.0) on Windows-7, Linux Ubuntu 14.04. Hence marking this as Verified.
,
May 21 2016
,
Sep 28 2016
[Auto-generated comment by a script] We noticed that this issue is targeted for M-52; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-52 label, otherwise remove Merge-TBD label. Thanks.
,
Sep 28 2016
[Bulk edit] Our blockerbot script was offline; it was recently brought back online, and thus labeled many old issues (including this one) erroneously. Removing Merge-TBD label since all milestones for this issue are already completed; no further work should be done. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by dmascare...@etouch.net
, May 5 2016