Bug 132593 - [Mac] Allow focus rings to redraw themselves if necessary
Summary: [Mac] Allow focus rings to redraw themselves if necessary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-05 18:01 PDT by Dean Jackson
Modified: 2014-05-06 18:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch (35.83 KB, patch)
2014-05-05 19:00 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (35.77 KB, patch)
2014-05-05 19:52 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (38.88 KB, patch)
2014-05-06 15:45 PDT, Dean Jackson
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (567.19 KB, application/zip)
2014-05-06 18:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-05-05 18:01:29 PDT
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.
Comment 1 Dean Jackson 2014-05-05 19:00:13 PDT
Created attachment 230879 [details]
Patch
Comment 2 Dean Jackson 2014-05-05 19:00:48 PDT
This patch won't build until the new WKSI has landed. I'll do that now. It is ready for review though.
Comment 3 Jon Lee 2014-05-05 19:46:42 PDT
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 4 Tim Horton 2014-05-05 19:51:53 PDT
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?
Comment 5 Dean Jackson 2014-05-05 19:52:54 PDT
Created attachment 230883 [details]
Patch
Comment 6 Dean Jackson 2014-05-05 19:53:22 PDT
Now that the WKSI parts have landed, the build should be happy.
Comment 7 Dean Jackson 2014-05-05 20:00:52 PDT
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.
Comment 8 Dean Jackson 2014-05-05 20:01:47 PDT
I've made the changes from Tim and Jon's review locally, but won't upload a new patch until landing time.
Comment 9 Simon Fraser (smfr) 2014-05-05 20:22:13 PDT
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?
Comment 10 Dean Jackson 2014-05-05 20:27:06 PDT
(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.
Comment 11 Dean Jackson 2014-05-06 15:45:13 PDT
Created attachment 230940 [details]
Patch
Comment 12 Simon Fraser (smfr) 2014-05-06 15:58:13 PDT
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 13 Build Bot 2014-05-06 18:04:37 PDT
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
Comment 14 Build Bot 2014-05-06 18:04:41 PDT
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
Comment 15 Dean Jackson 2014-05-06 18:08:43 PDT
Committed r168397: <http://trac.webkit.org/changeset/168397>