Bug 132584 - Add support for drawFocusIfNeeded
Summary: Add support for drawFocusIfNeeded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rik Cabanier
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-05 15:45 PDT by Rik Cabanier
Modified: 2014-05-08 10:57 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.48 KB, patch)
2014-05-05 15:49 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2014-05-07 16:50 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2014-05-07 19:52 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (5.77 KB, patch)
2014-05-08 06:19 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (5.85 KB, patch)
2014-05-08 07:20 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rik Cabanier 2014-05-05 15:45:57 PDT
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...
Comment 1 Rik Cabanier 2014-05-05 15:49:02 PDT
Created attachment 230863 [details]
Patch
Comment 2 Dirk Schulze 2014-05-07 00:56:09 PDT
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;
Comment 3 Rik Cabanier 2014-05-07 14:05:28 PDT
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.
Comment 4 Dirk Schulze 2014-05-07 14:47:52 PDT
(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.
Comment 5 Rik Cabanier 2014-05-07 16:50:58 PDT
Created attachment 231029 [details]
Patch
Comment 6 Darin Adler 2014-05-07 18:20:50 PDT
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?
Comment 7 Rik Cabanier 2014-05-07 19:52:53 PDT
Created attachment 231037 [details]
Patch
Comment 8 Dirk Schulze 2014-05-08 00:35:57 PDT
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
Comment 9 Dirk Schulze 2014-05-08 00:37:29 PDT
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.
Comment 10 Rik Cabanier 2014-05-08 06:19:17 PDT
Created attachment 231062 [details]
Patch
Comment 11 Rik Cabanier 2014-05-08 06:21:07 PDT
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
Comment 12 Dirk Schulze 2014-05-08 06:51:41 PDT
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.
Comment 13 Rik Cabanier 2014-05-08 07:20:55 PDT
Created attachment 231064 [details]
Patch
Comment 14 WebKit Commit Bot 2014-05-08 08:23:50 PDT
Comment on attachment 231064 [details]
Patch

Rejecting attachment 231064 [details] from review queue.

cabanier@adobe.com 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.
Comment 15 Dirk Schulze 2014-05-08 08:42:53 PDT
Comment on attachment 231064 [details]
Patch

Please wait for dino this time before submitting.
Comment 16 WebKit Commit Bot 2014-05-08 10:57:38 PDT
Comment on attachment 231064 [details]
Patch

Clearing flags on attachment: 231064

Committed r168476: <http://trac.webkit.org/changeset/168476>
Comment 17 WebKit Commit Bot 2014-05-08 10:57:44 PDT
All reviewed patches have been landed.  Closing bug.