Implement canvas focus ring methods.

Implement canvas focus ring methods.

BUG=261998
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155185

[Stephen Chennney, 2013-07-19 12:46:36]

My only nit is that the test case rectangle be blue not red. We like to avoid
red as it indicates failure.

But I am not the best person to review canvas work. I've added junov@ and
senorblanco@.

[Stephen White, 2013-07-19 14:37:11]

https://chromiumcodereview.appspot.com/19786002/diff/1/Source/core/html/canva...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://chromiumcodereview.appspot.com/19786002/diff/1/Source/core/html/canva...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:37: #include
"core/accessibility/AXObjectCache.h"
It seems unfortunate to add this dependency directly. Is there a way this could
be avoided, perhaps by adding methods to Element to set its accessibility
bounding box?

[dmazzoni, 2013-07-19 15:37:25]

On 2013/07/19 14:37:11, Stephen White wrote:
>
https://chromiumcodereview.appspot.com/19786002/diff/1/Source/core/html/canva...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/19786002/diff/1/Source/core/html/canva...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:37: #include
> "core/accessibility/AXObjectCache.h"
> It seems unfortunate to add this dependency directly. Is there a way this
could
> be avoided, perhaps by adding methods to Element to set its accessibility
> bounding box?

This is the most consistent with the rest of Blink. Also, I believe
Canvas is exceptional here: I can't think of any other case where
an element's bounding rect would be explicitly set - normally it's
queried lazily from the renderer.

There have been informal discussions on how we could maybe move
accessibility into a separate module with a narrower API now
that Blink has forked, and I'm going to start playing with that
this quarter. But I think this is the most clear in the meantime.

Thanks.

[dmazzoni, 2013-07-19 16:46:57]

I updated the color from red to blue.

If this looks basically okay, should I commit the -expected.png file first so I
can run tryjobs and make sure the image expectations are cross-platform?

In general does this look cross-platform-safe? It seems like it ought to be
possible to test this with an image that will be pixel-identical on all
platforms
since there's no text involved. What about accelerated vs not, would that
affect the pixel accuracy?

[Stephen White, 2013-07-19 21:30:48]

On 2013/07/19 16:46:57, Dominic Mazzoni wrote:
> I updated the color from red to blue.
> 
> If this looks basically okay, should I commit the -expected.png file first so
I
> can run tryjobs and make sure the image expectations are cross-platform?

That's an interesting idea, but we don't usually do that. Just mark it as [
NeedsRebaseline ] in LayoutTests/TestExpectations.  The bots won't go red, and
the auto-rebaseline-bot will create platform-specific baselines for you.

You'll also need to mark canvas-2d-api-changes as failing (due to new API). It
doesn't get rebaselined in the normal way, so just mark it as [ Failure ] in
TestExpectations. You can git cl try that patch, and the bots should stay green.

> In general does this look cross-platform-safe? It seems like it ought to be
> possible to test this with an image that will be pixel-identical on all
> platforms
> since there's no text involved. What about accelerated vs not, would that
> affect the pixel accuracy?

Without text, Linux and Windows often match, but Mac is usually an outlier (due
to gamma issues). You're unlikely to get all three to match. And yes,
acceleration often tweaks things a bit too.

[Stephen White, 2013-07-19 21:31:10]

https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2367: 
I notice the spec also says that the focus ring should not be subject to the
global composition operators. Should we force that to SrcOver here as well?

[dmazzoni, 2013-07-19 22:24:54]

On 2013/07/19 21:30:48, Stephen White wrote:
> That's an interesting idea, but we don't usually do that. Just mark it as [
> NeedsRebaseline ] in LayoutTests/TestExpectations.  The bots won't go red, and
> the auto-rebaseline-bot will create platform-specific baselines for you.

Done.

> You'll also need to mark canvas-2d-api-changes as failing (due to new API). It
> doesn't get rebaselined in the normal way, so just mark it as [ Failure ] in
> TestExpectations. You can git cl try that patch, and the bots should stay
green.

Done.

> Without text, Linux and Windows often match, but Mac is usually an outlier
(due
> to gamma issues). You're unlikely to get all three to match. And yes,
> acceleration often tweaks things a bit too.

OK. I fixed the test slightly so the js part passes on all three platforms.
I'll baseline the images afterwards.

[dmazzoni, 2013-07-19 22:25:50]

On 2013/07/19 21:31:10, Stephen White wrote:
>
https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2367: 
> I notice the spec also says that the focus ring should not be subject to the
> global composition operators. Should we force that to SrcOver here as well?

You're right, good catch. Done.

[Stephen White, 2013-07-22 16:27:59]

On 2013/07/19 22:25:50, Dominic Mazzoni wrote:
> On 2013/07/19 21:31:10, Stephen White wrote:
> >
>
https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
> > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > 
> >
>
https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2367: 
> > I notice the spec also says that the focus ring should not be subject to the
> > global composition operators. Should we force that to SrcOver here as well?
> 
> You're right, good catch. Done.

Judging by the bot results, you'll also need to add a NeedsRebaseline line for 

virtual/gpu/fast/canvas/draw-system-focus-ring.html

(this is the new test, as run with GPU acceleration).

[Stephen White, 2013-07-22 16:30:28]

On 2013/07/22 16:27:59, Stephen White wrote:
> On 2013/07/19 22:25:50, Dominic Mazzoni wrote:
> > On 2013/07/19 21:31:10, Stephen White wrote:
> > >
> >
>
https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
> > > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/19786002/diff/9001/Source/core/html/canvas/Ca...
> > > Source/core/html/canvas/CanvasRenderingContext2D.cpp:2367: 
> > > I notice the spec also says that the focus ring should not be subject to
the
> > > global composition operators. Should we force that to SrcOver here as
well?
> > 
> > You're right, good catch. Done.
> 
> Judging by the bot results, you'll also need to add a NeedsRebaseline line for

> 
> virtual/gpu/fast/canvas/draw-system-focus-ring.html
> 
> (this is the new test, as run with GPU acceleration).

Actually, looking at the test again, it looks like it's a text-based text, ie.,
the JS shouldBe() calls should test the results programmatically, and you don't
need pixel results.

If that's the case, you can call dumpAsText(false) instead of dumpAsText(true),
to indicate that you don't need pixel results, and you won't need to mark the
test as NeedsRebaseline.

[dmazzoni, 2013-07-22 18:49:42]

On 2013/07/22 16:30:28, Stephen White wrote:
> Actually, looking at the test again, it looks like it's a text-based text,
ie.,
> the JS shouldBe() calls should test the results programmatically, and you
don't
> need pixel results.
> 
> If that's the case, you can call dumpAsText(false) instead of
dumpAsText(true),
> to indicate that you don't need pixel results, and you won't need to mark the
> test as NeedsRebaseline.

OK, let's do that. I originally thought having the pixel results could be useful
too - but I guess
if the text result ever fails it'd be trivial for someone to open it and see
what it looks like.

[Rik, 2013-07-23 03:45:15]

I'm unsure if you should already implement the customfocusring methods that take
a path object.
The patch also pull information out of the element's CSS style which seems wrong

[dmazzoni, 2013-07-23 04:57:43]

On 2013/07/23 03:45:15, Rik wrote:
> I'm unsure if you should already implement the customfocusring methods that
take
> a path object.

Is that because the path object support is unfinished? I'm happy to remove it
for now.

> The patch also pull information out of the element's CSS style which seems
wrong

Well, we need to pull information about how to draw the focus ring from
somewhere. Do you
think it makes more sense to pull it from the canvas? That wouldn't allow for as
much
flexibility.

[dmazzoni, 2013-07-24 17:44:36]

Stephen, any thoughts?

In terms of where to get the parameters to draw the focus ring, here are the
options I can think of:

1. Hard-code default values. I don't think this would be very useful, it
wouldn't look native and wouldn't match the focus color of other elements.
2. Get them from the defaults for a generic element. This might make testing
harder because I believe the focus color can be controlled by the system, unless
we already override this for tests. I also think this is less flexible for
developers, if they picked a custom outline color for every other element in
their site but would have no way to override it for canvas (otehr than
drawCustomFocusRing).
3. Get them from the canvas element. This means every focus ring within the
canvas would be the same, but otherwise I have no objection.
4. Get them from the element passed to drawSystemFocusRing.

I still think #4 is the best choice, because the spec essentially implies that
the focus ring drawn on the canvas is a proxy for the element passed to the
drawSystemFocusRing method - it's only drawn if the element has focus, for
example. So it makes sense to me that the style of the focus ring should come
from that element as well.

Let's pick what we think is best and ship it in dev channel, then throw this
back to spec authors to see what they think - then whether they agree or
disagree, we can ask them to clarify the spec.

If there's still disagreement about the right solution at the M30 branch point,
I'm happy to disable this feature on that branch.

[Stephen White, 2013-07-24 19:24:02]

On 2013/07/24 17:44:36, Dominic Mazzoni wrote:
> Stephen, any thoughts?
> 
> In terms of where to get the parameters to draw the focus ring, here are the
> options I can think of:
> 
> 1. Hard-code default values. I don't think this would be very useful, it
> wouldn't look native and wouldn't match the focus color of other elements.
> 2. Get them from the defaults for a generic element. This might make testing
> harder because I believe the focus color can be controlled by the system,
unless
> we already override this for tests. I also think this is less flexible for
> developers, if they picked a custom outline color for every other element in
> their site but would have no way to override it for canvas (otehr than
> drawCustomFocusRing).
> 3. Get them from the canvas element. This means every focus ring within the
> canvas would be the same, but otherwise I have no objection.
> 4. Get them from the element passed to drawSystemFocusRing.
> 
> I still think #4 is the best choice, because the spec essentially implies that
> the focus ring drawn on the canvas is a proxy for the element passed to the
> drawSystemFocusRing method - it's only drawn if the element has focus, for
> example. So it makes sense to me that the style of the focus ring should come
> from that element as well.
> 
> Let's pick what we think is best and ship it in dev channel, then throw this
> back to spec authors to see what they think - then whether they agree or
> disagree, we can ask them to clarify the spec.
> 
> If there's still disagreement about the right solution at the M30 branch
point,
> I'm happy to disable this feature on that branch.

I don't think I have anything intelligent to say -- you've covered all the
possibilities I can think of. Definitely would be good to ping whatwg@ to
discover the intent on the spec side.

BTW, was there a blink-dev discussion about this feature? I think we need an
intent-to-ship before we can expose this to the web at large (in particular, the
IDL changes). If you want to land it sooner, you should send an
"intent-to-implement" and land it behind the "experimental canvas features
flag".

[dmazzoni, 2013-07-26 23:04:00]

I moved it behind the experimentalCanvasFeatures flag
and made it use the default focus ring style rather
than the one from the selected element.

I'm going to send the Intent to Implement email now.
Please review when you have a chance; I promise I won't
land until I get an okay on the Intent to Implement
thread too.

Thanks!

[jbroman, 2013-07-29 17:25:07]

moved comment from blink-dev to code review as requested

https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2388:
c->drawFocusRing(rects, style->outlineWidth(), style->outlineOffset(),
focusRingColor);
Why not draw the path itself? GraphicsContext::drawFocusRing can take a path,
and the spec seems to suggest that is the correct behaviour.

If you'd like to close the path, there's Path::closeSubpath which should do the
trick (though it may make sense to leave that up to the content author). That's
how paths are closed in CanvasPathMethods::closePath, for instance.

[Rik, 2013-07-29 20:09:52]

On 2013/07/23 04:57:43, Dominic Mazzoni wrote:
> On 2013/07/23 03:45:15, Rik wrote:
> > I'm unsure if you should already implement the customfocusring methods that
> take
> > a path object.
> 
> Is that because the path object support is unfinished? I'm happy to remove it
> for now.

Great!
If we conclude that we should draw around the path, you can bring it back.
> 
> > The patch also pull information out of the element's CSS style which seems
> wrong
> 
> Well, we need to pull information about how to draw the focus ring from
> somewhere. Do you
> think it makes more sense to pull it from the canvas? That wouldn't allow for
as
> much
> flexibility.

Did this get resolved? Are you pulling the info from the focus style now?

You also did not implement drawCustomFocusRing correctly. It should return true
if the element is focused and not do any of the drawing itself.
Confusing!

[Rik, 2013-07-29 20:09:59]

https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2388:
c->drawFocusRing(rects, style->outlineWidth(), style->outlineOffset(),
focusRingColor);
On 2013/07/29 17:25:08, jbroman wrote:
> Why not draw the path itself? GraphicsContext::drawFocusRing can take a path,
> and the spec seems to suggest that is the correct behaviour.
> 
> If you'd like to close the path, there's Path::closeSubpath which should do
the
> trick (though it may make sense to leave that up to the content author).
That's
> how paths are closed in CanvasPathMethods::closePath, for instance.

I'm unsure this is true. The spec is confusing here by mentioning "around the
path" and "along the path"

[dmazzoni, 2013-07-29 21:44:21]

On 2013/07/29 20:09:52, Rik wrote:
> Did this get resolved? Are you pulling the info from the focus style now?

I'm pulling the style from the default focus style now, not from the element.
That's what Ian suggested on whatwg.

> You also did not implement drawCustomFocusRing correctly. It should return
true
> if the element is focused and not do any of the drawing itself.
> Confusing!

Thanks, good catch. I reorganized the code a bit to make the differences between
drawSystemFocusRing and drawCustomFocusRing more clear, and I added a new
drawCustomFocusRing test that just tests for the correct return value.

https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2388:
c->drawFocusRing(rects, style->outlineWidth(), style->outlineOffset(),
focusRingColor);
On 2013/07/29 20:10:00, Rik wrote:
> I'm unsure this is true. The spec is confusing here by mentioning "around the
> path" and "along the path"

It's unclear to me too.

Since this is behind a flag, I don't really care too much, I
just want to throw it in front of a few web developers and see
if they find it confusing or not.

I currently changed it to just draw the path without closing it.
Let's see how that goes.

[Rik, 2013-07-30 03:46:20]

On 2013/07/29 21:44:21, Dominic Mazzoni wrote:
> On 2013/07/29 20:09:52, Rik wrote:
> > Did this get resolved? Are you pulling the info from the focus style now?
> 
> I'm pulling the style from the default focus style now, not from the element.
> That's what Ian suggested on whatwg.
> 
> > You also did not implement drawCustomFocusRing correctly. It should return
> true
> > if the element is focused and not do any of the drawing itself.
> > Confusing!
> 
> Thanks, good catch. I reorganized the code a bit to make the differences
between
> drawSystemFocusRing and drawCustomFocusRing more clear, and I added a new
> drawCustomFocusRing test that just tests for the correct return value.
> 
>
https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/19786002/diff/58001/Source/core/html/canvas/C...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:2388:
> c->drawFocusRing(rects, style->outlineWidth(), style->outlineOffset(),
> focusRingColor);
> On 2013/07/29 20:10:00, Rik wrote:
> > I'm unsure this is true. The spec is confusing here by mentioning "around
the
> > path" and "along the path"
> 
> It's unclear to me too.
> 
> Since this is behind a flag, I don't really care too much, I
> just want to throw it in front of a few web developers and see
> if they find it confusing or not.
> 
> I currently changed it to just draw the path without closing it.
> Let's see how that goes.

Great!
LGTM

[dmazzoni, 2013-07-30 15:35:27]

We have support on the "Intent to implement" thread as well.

@senorblanco, any last feedback from you or is this ready
to commit?

[Rik, 2013-07-30 15:43:48]

On 2013/07/30 15:35:27, Dominic Mazzoni wrote:
> We have support on the "Intent to implement" thread as well.
> 
> @senorblanco, any last feedback from you or is this ready
> to commit?

The try bots are all red. Shouldn't that be fixed first?

[dmazzoni, 2013-07-30 15:46:29]

On 2013/07/30 15:43:48, Rik wrote:
> The try bots are all red. Shouldn't that be fixed first?

Yes, of course.

At first glance the failure looks unrelated, but I'm checking.

[jbroman, 2013-07-30 15:58:37]

On 2013/07/30 15:46:29, Dominic Mazzoni wrote:
> On 2013/07/30 15:43:48, Rik wrote:
> > The try bots are all red. Shouldn't that be fixed first?
> 
> Yes, of course.
> 
> At first glance the failure looks unrelated, but I'm checking.

The colours shown in Rietveld for *_blink_rel are misleading. I  believe the
trybots for patch set 9 are in fact green as far as CQ is concerned (since
webkit_tests passed -- i.e. the failures were the same with and without the
patch).

[Stephen White, 2013-07-30 16:24:22]

LGTM, although IWBN to remove the "rects" stuff if possible.

https://codereview.chromium.org/19786002/diff/75001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19786002/diff/75001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2410:
rects.append(pixelSnappedIntRect(LayoutRect(path.boundingRect())));
After this append, "rects" seems to be unused. Am I missing something?

[dmazzoni, 2013-07-30 16:27:10]

https://codereview.chromium.org/19786002/diff/75001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19786002/diff/75001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2410:
rects.append(pixelSnappedIntRect(LayoutRect(path.boundingRect())));
On 2013/07/30 16:24:23, Stephen White wrote:
> After this append, "rects" seems to be unused. Am I missing something?

Oops, you're right. Thanks!

[commit-bot: I haz the power, 2013-07-30 16:27:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/19786002/81001

[commit-bot: I haz the power, 2013-07-30 22:29:38]

Change committed as 155185

[ojan, 2013-07-31 16:54:52]

https://chromiumcodereview.appspot.com/19786002/diff/81001/LayoutTests/TestEx...
File LayoutTests/TestExpectations (right):

https://chromiumcodereview.appspot.com/19786002/diff/81001/LayoutTests/TestEx...
LayoutTests/TestExpectations:1576: crbug.com/261998
inspector/profiler/canvas2d/canvas2d-api-changes.html [ Failure ]
Should this have been a NeedsRebaseline line instead of Failure?

[dmazzoni, 2013-07-31 17:00:22]

https://chromiumcodereview.appspot.com/19786002/diff/81001/LayoutTests/TestEx...
File LayoutTests/TestExpectations (right):

https://chromiumcodereview.appspot.com/19786002/diff/81001/LayoutTests/TestEx...
LayoutTests/TestExpectations:1576: crbug.com/261998
inspector/profiler/canvas2d/canvas2d-api-changes.html [ Failure ]
On 2013/07/31 16:54:52, ojan wrote:
> Should this have been a NeedsRebaseline line instead of Failure?

I was told api changes are not rebaseline-able.

Perhaps the inspector team wants to manually look at api changes?