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

Issue 605136 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug


Participants' hotlists:
MacViews-Task-Queue


Sign in to add a comment

MacViews: Quickly hovering through Page Info Bubble buttons does not keep up with mouse

Project Member Reported by rsesek@chromium.org, Apr 20 2016

Issue description

Version: 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.
 
hover-oib.mov
2.9 MB Download
hover-oib-sample.txt
674 KB View Download
Cc: -tapted@chromium.org pinkerton@chromium.org ellyjo...@chromium.org erikc...@chromium.org ccameron@chromium.org
Labels: -Pri-2 Pri-1
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)
I think this is pretty high priority. Performance is one of the concerns about non-native toolkit. 

It looks like the bulk of time is spent in text drawing. But looking at rsesek's video, the actual text isn't changing at all, just a border around the text to make it looks "raised". 

If I had to implement this animation in Cocoa, I would just use two layers, with the bottom layer being a 9-sliceable image of a border. This bottom layer could be animated to full opacity (from 0) with an implicit animation. 

I expect that we'll need a lot of border effects for views. Rather than implementing a bunch of different border-related animations, and then trying to optimize [and add compositor plumbing], can we figure out what we want to do, add plumbing, and then add animations?
Labels: -Hotlist-MacViews Proj-MacViews

Comment 3 by tapted@chromium.org, 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

Comment 4 by tapted@chromium.org, 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.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Labels: Needs-Feedback
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.
OIB_Sample_52.0.2721.0.txt
290 KB View Download
Cc: shrike@chromium.org
Labels: -Pri-1 Pri-3
+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.
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).
Request someone to update on the above issue?

Thank you!
Labels: -Needs-Feedback
still in my queue
This happens with the radio buttons in the MD views too.
button-scrub.mov
277 KB Download
These radio button should not be displaying borders like this. I will file a separate bug on this.
Screen Shot 2016-09-22 at 8.29.38 AM.png
47.0 KB View Download
Summary: MacViews: Quickly hovering through Page Info Bubble buttons does not keep up with mouse (was: MacViews: Quickly hovering through OIB popup buttons does not keep up with mouse)
Components: Internals>Views
Status: Fixed (was: Assigned)
MacViews triage: calling this Fixed. Most controls have no hover effect now; those that do seem snappy enough.

Sign in to add a comment