Bug 132584 – Add support for drawFocusIfNeeded

• Component: Canvas
• Status: RESOLVED FIXED

[Rik Cabanier]

Spec: http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas_CR/#dom-context-2d-drawfocusifneeded
Blink and Mozilla are also shipping this API.

I'm unsure how I can write a test for this...

[Rik Cabanier]

Created attachment 230863 [details]
Patch

[Dirk Schulze]

Comment on attachment 230863 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230863&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests.

Why are there no tests? Seems like it could be emulated, no?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1969
> +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element)

It would definitely be great to have tests for that!

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1978
> +    if (element->focused()) {

Early return:
if (!element->focused())
    return;

[Rik Cabanier]

Comment on attachment 230863 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230863&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1969
>> +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element)
> 
> It would definitely be great to have tests for that!

How would I create a test for this?
I could manually draw the ring but that might not match what the browser does. Maybe I should just check if something changed?

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1978
>> +    if (element->focused()) {
> 
> Early return:
> if (!element->focused())
>     return;

will do.

[Dirk Schulze]

(In reply to comment #3)
> (From update of attachment 230863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230863&action=review
> 
> >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1969
> >> +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element)
> > 
> > It would definitely be great to have tests for that!
> 
> How would I create a test for this?
> I could manually draw the ring but that might not match what the browser does. Maybe I should just check if something changed?

That would be the easiest way probably.

Why doesn't it work to draw the focus ring of the element in canvas in one test and without the canvas in another HTML file?

> 
> >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1978
> >> +    if (element->focused()) {
> > 
> > Early return:
> > if (!element->focused())
> >     return;
> 
> will do.

[Rik Cabanier]

Created attachment 231029 [details]
Patch

[Darin Adler]

Comment on attachment 231029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231029&action=review

Not sure I can review+ this. Dean Jackson should probably take a look.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1974
> +void CanvasRenderingContext2D::drawFocusIfNeeded(Element* element)

Since this is exposed to the DOM, it needs to handle null element pointers. Probably just need to return early if it’s null.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1983
> +    if (element->focused()) {

Why not early return for this check too?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1984
> +        GraphicsContext* c = drawingContext();

Would you be willing to consider use a word instead of a letter for this local variable?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1985
> +        if (c)

Again, why not an early return for this too?

[Rik Cabanier]

Created attachment 231037 [details]
Patch

[Dirk Schulze]

Comment on attachment 231037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231037&action=review

r- because I asked for the changes last review. Patch itself looks good though.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1985
> +    if (!element)
> +        return;
> +    if (!state().m_hasInvertibleTransform)
> +        return;
> +    if (m_path.isEmpty())
> +        return;
> +    if (!element->isDescendantOf(canvas()))
> +        return;
> +    
> +    if (element->focused()) {

you can collapse those into one if clause. And (as said in previous review!) make it if (!element->focused()) return

> LayoutTests/fast/canvas/draw-focus-if-needed.html:3
> +    <button id="button1"></button>

Change all tabs to spaces.

> LayoutTests/fast/canvas/draw-focus-if-needed.html:23
> +else
> +	{

else {

> LayoutTests/fast/canvas/draw-focus-if-needed.html:34
> +	}

no indentation

[Dirk Schulze]

Comment on attachment 231037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231037&action=review

> Source/WebCore/ChangeLog:7
> +

Add some lines what is actually expected by the function. "When called, draws focus ring for descendants of canvas element." or so.

[Rik Cabanier]

Created attachment 231062 [details]
Patch

[Rik Cabanier]

Comment on attachment 231037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231037&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> Add some lines what is actually expected by the function. "When called, draws focus ring for descendants of canvas element." or so.

done

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1985
>> +    if (element->focused()) {
> 
> you can collapse those into one if clause. And (as said in previous review!) make it if (!element->focused()) return

done.
Sorry that I missed that.

>> LayoutTests/fast/canvas/draw-focus-if-needed.html:3
>> +    <button id="button1"></button>
> 
> Change all tabs to spaces.

done

[Dirk Schulze]

Comment on attachment 231062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231062&action=review

Snippet in changelog. Otherwise looks good to me. Please give Dean time to look at it before landing.

> Source/WebCore/ChangeLog:8
> +        This API will draw a focus ring if the element is focused.

"the element"? What kind of element? It is a child of <canvas>, right? And it is also just drawn when the element is focused and the function is called.

[Rik Cabanier]

Created attachment 231064 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 231064 [details]
Patch

Rejecting attachment 231064 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Dirk Schulze]

Comment on attachment 231064 [details]
Patch

Please wait for dino this time before submitting.

[WebKit Commit Bot]

Comment on attachment 231064 [details]
Patch

Clearing flags on attachment: 231064

Committed r168476: <http://trac.webkit.org/changeset/168476>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.