Mac allows focus rings to animate, changing their appearance over time. Expose this to the render code, such that it can be notified if a focused element wants to redraw itself, as well as remember how long it has been since the element was focused.
Created attachment 230879 [details] Patch
This patch won't build until the new WKSI has landed. I'll do that now. It is ready for review though.
Comment on attachment 230879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230879&action=review Unofficial r=me. > Source/WebCore/page/FocusController.cpp:928 > + focusedElement->renderer()->repaint(); Need to check the renderer()?
Comment on attachment 230879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230879&action=review > Source/WebCore/ChangeLog:16 > + to port the code from their into WebKit. s/their/there/ > Source/WebCore/page/FocusController.cpp:933 > + return currentTime() - m_focusSetTime; I think you want monotonicallyIncreasingTime, not currentTime!! > Source/WebCore/rendering/RenderView.cpp:106 > + , m_maximalOutlineSize(9) this seems ... unfortunate. does this inflate layers too?
Created attachment 230883 [details] Patch
Now that the WKSI parts have landed, the build should be happy.
Comment on attachment 230879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230879&action=review >> Source/WebCore/page/FocusController.cpp:928 >> + focusedElement->renderer()->repaint(); > > Need to check the renderer()? Will do. >> Source/WebCore/page/FocusController.cpp:933 >> + return currentTime() - m_focusSetTime; > > I think you want monotonicallyIncreasingTime, not currentTime!! Yes >> Source/WebCore/rendering/RenderView.cpp:106 >> + , m_maximalOutlineSize(9) > > this seems ... unfortunate. does this inflate layers too? Yep :( All layers will grow by 6px on each side. And we'll be repainting more area too.
I've made the changes from Tim and Jon's review locally, but won't upload a new patch until landing time.
Comment on attachment 230883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230883&action=review > Source/WebCore/rendering/RenderView.cpp:107 > +#if PLATFORM(MAC) > + , m_maximalOutlineSize(9) > +#else We can't do this. It will bloat all layers, and make all opaque things non-opaque. Why can't you just do this when a focus ring is showing?
(In reply to comment #9) > (From update of attachment 230883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230883&action=review > > > Source/WebCore/rendering/RenderView.cpp:107 > > +#if PLATFORM(MAC) > > + , m_maximalOutlineSize(9) > > +#else > > We can't do this. It will bloat all layers, and make all opaque things non-opaque. We already do this, it's just that the value has increased from 3 to 9. Yes, it bloats the layers. > Why can't you just do this when a focus ring is showing? Because the crazy logic that determines repaint size and layer size doesn't actually care about whether or not a focus ring is showing. It applies this fudge factor to everything. Fixing that would be a pretty huge change.
Created attachment 230940 [details] Patch
Comment on attachment 230940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230940&action=review > Source/WebCore/page/FocusController.cpp:172 > + m_focusRepaintTimer.stop(); I think timers stop themselves on destruction. > Source/WebCore/page/FocusController.cpp:915 > + m_focusRepaintTimer.startOneShot(0); Why repaint at > 60fps? > Source/WebCore/page/FocusController.h:93 > + void focusedElementNeedsRepaint(); This sounds like it is returning state. I think it should be setFocusedElementNeedsRepaintSoon(). > Source/WebCore/page/FocusController.h:94 > + double timeSinceFocusWasSet() const; Aren't the cool kids using std::chromo these days? > Source/WebCore/platform/ControlStates.h:108 > + double m_timeSinceFocusWasSet; Hmm, wouldn't this value be continually changing? Seems odd to store it. Is it something else here, like time used for painting the next focus ring state? > Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:73 > +void GraphicsContext::drawFocusRing(const Path& path, int /* width */, int /*offset*/, const Color&) Spacing in comments is inconsistent. > Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:91 > + for (IntRect rect : rects) const IntRect& ? Or auto&? > Source/WebCore/platform/graphics/mac/GraphicsContextMac.mm:107 > + for (IntRect rect : rects) Ditto. > Source/WebCore/platform/mac/ThemeMac.mm:287 > - { 3, 4, 4, 2 }, > - { 4, 3, 3, 3 }, > - { 4, 3, 3, 3 }, > + { 7, 8, 8, 6 }, > + { 8, 7, 7, 7 }, > + { 8, 8, 7, 7 }, Why aren't these different on different OSes? > Source/WebCore/rendering/RenderElement.cpp:374 > + view().setMaximalOutlineSize(std::max(theme().platformFocusRingMaxWidth(), static_cast<int>(m_style->outlineSize()))); So m_style->outlineSize() is a lie now? Won't that break repainting, e.g. in RenderElement::styleWillChange() which does if (m_parent && (newStyle.outlineSize() < m_style->outlineSize()...
Comment on attachment 230940 [details] Patch Attachment 230940 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4990262187130880 New failing tests: svg/text/non-bmp-positioning-lists.svg compositing/geometry/ancestor-overflow-change.html compositing/layer-creation/overlap-animation-container.html compositing/contents-opaque/control-layer.html
Created attachment 230961 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Committed r168397: <http://trac.webkit.org/changeset/168397>