MacViews: Quickly hovering through Page Info Bubble buttons does not keep up with mouse |
||||||||
Issue descriptionVersion: 52.0.2713.0 OS: 10.11.4 What steps will reproduce the problem? (1) Enable --enable-mac-views-dialogs (2) Open the Origin Info Bubble (3) Quickly mouse over all the permission popup buttons (4) Highlight effect doesn't keep up with mouse What is the expected output? Responsive UI highlighting. What do you see instead? Jank. Please use labels and text to provide additional information. This may be the same root cause as issue 605131 , but filing separately. Attached is a screen recording and sample.
,
Apr 20 2016
,
Apr 21 2016
Cocoa will disable subpixel AA if you aren't smart about using layers around text. Also we don't use this hover animation in the current Cocoa UI - that's an option if we simply don't like the hover animation :). (also MD tends to do away with hover animations altogether). But this is still a pretty wretched performance bug in TableView. It's in `ChooserBubbleUiViewDelegate` under c/b/ui/views/website_settings. TableView is probably a dumb idea if you're doing animations inside a particular table cell. Converting this to use a better view Layout helper might be one way to fix. Fixing the performance bug in TableView should be doable too. Probably: - the text should be a views::Label which caches rendertext instances - the border needs to be painted *after* the text with a transparent hole in it so that it doesn't invalidate the text solid background canvas and mess up subpixel AA
,
Apr 21 2016
https://codereview.chromium.org/1907833002/ should address the bulk of this (as well as Issue 605131 ). There's still an interesting follow-up in exploring layers for the borders (or just removing the animation altogether). There's also the "offical" MenuButton / drop-down style, which has a blue "shoulder". The current Cocoa OIB doesn't use the native dropdown style though.
,
Apr 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/758d2baa307aa32b9de708e97655663726b8fa9c commit 758d2baa307aa32b9de708e97655663726b8fa9c Author: tapted <tapted@chromium.org> Date: Fri Apr 22 06:19:08 2016 RenderTextMac: Cache the SkTypeface on the TextRun SkiaTextRenderer::SetFontWithStyle() uses gfx::Font::Derive() which is slow. SetFontWithStyle is only used on Mac but, also, there was actually never anything to derive on Mac, since RenderTextMac does (on every *paint*): CTFontRef = <get font from CTRun> gfx::Font = <wrap CTFontRef> CFFontRef = <unwrap from gfx:Font> # cheap style = <extract style from CTFontRef> gfx::Font_2 = Derive(apply <style> to gfx::Font) # A "no-op", but expensive So remove SkiaTextRenderer::SetFontWithStyle() since nothing needs it. Then store the SkTypeface on the RenderTextMac TextRun. This saves a lookup on each paint and is consistent with RenderTextHarfbuzz. BUG= 605131 , 605136 Review URL: https://codereview.chromium.org/1907833002 Cr-Commit-Position: refs/heads/master@{#389042} [modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text.cc [modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text.h [modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_harfbuzz.cc [modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_harfbuzz.h [modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_mac.h [modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_mac.mm
,
Apr 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6371bc49fe4c8e605a1b0a98e417e6aea80a5168 commit 6371bc49fe4c8e605a1b0a98e417e6aea80a5168 Author: kjellander <kjellander@chromium.org> Date: Fri Apr 22 11:24:01 2016 Revert of RenderTextMac: Cache the SkTypeface on the TextRun (patchset #4 id:80001 of https://codereview.chromium.org/1907833002/ ) Reason for revert: Seems to break gfx_unittests on Mac10.11 Tests: https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/1384 See https://findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/1384 for more info. Original issue's description: > RenderTextMac: Cache the SkTypeface on the TextRun > > SkiaTextRenderer::SetFontWithStyle() uses gfx::Font::Derive() which is > slow. SetFontWithStyle is only used on Mac but, also, there was actually > never anything to derive on Mac, since RenderTextMac does (on every > *paint*): > CTFontRef = <get font from CTRun> > gfx::Font = <wrap CTFontRef> > CFFontRef = <unwrap from gfx:Font> # cheap > style = <extract style from CTFontRef> > gfx::Font_2 = Derive(apply <style> to gfx::Font) # A "no-op", but expensive > > So remove SkiaTextRenderer::SetFontWithStyle() since nothing needs it. > > Then store the SkTypeface on the RenderTextMac TextRun. This saves a > lookup on each paint and is consistent with RenderTextHarfbuzz. > > BUG= 605131 , 605136 TBR=asvitkine@chromium.org,tapted@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 605131 , 605136 Review URL: https://codereview.chromium.org/1914443002 Cr-Commit-Position: refs/heads/master@{#389076} [modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text.cc [modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text.h [modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_harfbuzz.cc [modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_harfbuzz.h [modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_mac.h [modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_mac.mm
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59fe54df8c0de55f03c8fb5e1860279d2993b473 commit 59fe54df8c0de55f03c8fb5e1860279d2993b473 Author: tapted <tapted@chromium.org> Date: Tue Apr 26 22:38:09 2016 RenderTextMac: Cache the SkTypeface on the TextRun [reland] SkiaTextRenderer::SetFontWithStyle() uses gfx::Font::Derive() which is slow. SetFontWithStyle is only used on Mac but, also, there was actually never anything to derive on Mac, since RenderTextMac does (on every *paint*): CTFontRef = <get font from CTRun> gfx::Font = <wrap CTFontRef> CFFontRef = <unwrap from gfx:Font> # cheap style = <extract style from CTFontRef> gfx::Font_2 = Derive(apply <style> to gfx::Font) # A "no-op", but expensive So remove SkiaTextRenderer::SetFontWithStyle() since nothing needs it. Then store the SkTypeface on the RenderTextMac TextRun. This saves a lookup on each paint and is consistent with RenderTextHarfbuzz. [reland: prior CL regressed RenderTextTest.Multiline_ZeroWidthChars] BUG= 605131 , 605136 Review URL: https://codereview.chromium.org/1919123002 Cr-Commit-Position: refs/heads/master@{#389924} [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text.cc [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text.h [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_harfbuzz.cc [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_harfbuzz.h [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_mac.h [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_mac.mm [modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_unittest.cc
,
May 2 2016
There are some more patches that landed on Issue 605131 that might have influenced things here wrt CPU usage. But I don't think there's anything left to performance-optimize here. Attaching a sample, but a large portion is just scattered, solitary samples in malloc/free. The only thing that seems worth improving on is that every MouseMove event makes a call to base::Histogram::Add[Count](). Invoking `+[NSGraphicsContext currentContext]` also seems "kinda" expensive for some reason, but "kinda" = 2/3000 samples. The perceived lag isn't due to CPU usage, but merely the way the animations are scheduled interacting with vsync. Setting `const int views::LabelButton::kHoverAnimationDurationMs` to 0 instead of 170ms removes any perception of lag for me. So, should I propose this to toolkit-views OWNERS for Mac? That is, toolbar_button_cocoa.mm doesn't have any logic for fade-in/out: Do we decree that, on Mac, toolbar buttons should not fade-in/out their borders? I can probably just #ifdef that initializer or something.
,
May 3 2016
+shrike in case you have an opinion about > Do we decree that, on Mac, toolbar buttons should not fade-in/out their borders? But of course this may change with MD. The permissions buttons on the OIB currently share the bulk of their code with toolbar buttons. E.g. the border is similar. They just draw the "down" arrow next to the label/image (and toolbar buttons don't). On ChromeOS, the border is already removed from the OIB permissions buttons. So that could Just Happen on all platforms. Which, conveniently, takes it full-circle to look like the existing Cocoa dialog on Mac. So, there might not be anything to do here.
,
May 3 2016
In general buttons won't need a hover effect - the only exception will be toolbar buttons, which under Material Design fade in and out their background (something I need to add to the current MD implementation). For now it's probably good enough to disable the hover completely. Given that the toolbar buttons just host icons we might be able to get away with layers when we get to making this work for the toolbar (since there's no subpixel AA to worry about).
,
Jun 22 2016
Request someone to update on the above issue? Thank you!
,
Jun 22 2016
still in my queue
,
Sep 22 2016
This happens with the radio buttons in the MD views too.
,
Sep 22 2016
These radio button should not be displaying borders like this. I will file a separate bug on this.
,
Nov 16 2016
,
Nov 1 2017
,
Mar 29 2018
MacViews triage: calling this Fixed. Most controls have no hover effect now; those that do seem snappy enough. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by erikc...@chromium.org
, Apr 20 2016Labels: -Pri-2 Pri-1
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)