Bug 132593 – [Mac] Allow focus rings to redraw themselves if necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 230879 [details]
Patch

[Dean Jackson]

This patch won't build until the new WKSI has landed. I'll do that now. It is ready for review though.

[Jon Lee]

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()?

[Tim Horton]

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?

[Dean Jackson]

Created attachment 230883 [details]
Patch

[Dean Jackson]

Now that the WKSI parts have landed, the build should be happy.

[Dean Jackson]

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.

[Dean Jackson]

I've made the changes from Tim and Jon's review locally, but won't upload a new patch until landing time.

[Simon Fraser (smfr)]

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?

[Dean Jackson]

(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.

[Dean Jackson]

Created attachment 230940 [details]
Patch

[Simon Fraser (smfr)]

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()...

[Build Bot]

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

[Build Bot]

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

[Dean Jackson]

Committed r168397: <http://trac.webkit.org/changeset/168397>