Last Comment Bug 540456 - Support HTML5 canvas draw{Custom,System}FocusRing()
: Support HTML5 canvas draw{Custom,System}FocusRing()
Status: RESOLVED FIXED
[parity-chrome-canary but hidden behi...
: access, dev-doc-complete, html5
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: All All
: -- enhancement with 2 votes (vote)
: mozilla28
Assigned To: Rik Cabanier
:
: Milan Sreckovic [:milan]
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 1120371
Blocks: 1119470
  Show dependency treegraph
 
Reported: 2010-01-18 10:29 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2015-02-16 07:53 PST (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add support and testing for system and custom focusring (11.04 KB, patch)
2013-09-29 13:20 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.23 KB, patch)
2013-09-29 13:29 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.94 KB, patch)
2013-09-29 15:18 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.94 KB, patch)
2013-09-29 15:20 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.94 KB, patch)
2013-09-29 15:22 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.91 KB, patch)
2013-09-29 15:27 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (258.71 KB, patch)
2013-10-01 13:09 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.62 KB, patch)
2013-10-01 13:29 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.63 KB, patch)
2013-10-01 13:33 PDT, Rik Cabanier
bugs: review-
Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.62 KB, patch)
2013-10-02 10:27 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (10.62 KB, patch)
2013-10-02 10:28 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (12.12 KB, patch)
2013-10-02 20:30 PDT, Rik Cabanier
bugs: review-
dbolter: feedback+
Details | Diff | Splinter Review
Add support and testing for system and custom focusring (15.75 KB, patch)
2013-10-08 14:49 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (15.82 KB, patch)
2013-10-08 17:11 PDT, Rik Cabanier
bugs: review+
Details | Diff | Splinter Review
Add support and testing for system and custom focusring (15.51 KB, patch)
2013-10-09 09:29 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (15.52 KB, patch)
2013-10-14 16:50 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (15.63 KB, patch)
2013-10-15 11:31 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (15.78 KB, patch)
2013-11-01 19:15 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
Add support and testing for system and custom focusring (16.47 KB, patch)
2013-11-03 18:31 PST, Rik Cabanier
no flags Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2010-01-18 10:29:01 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100115 Minefield/3.7a1pre
Build Identifier: 

A drawFocusRing() was added to HTML5. It should probably be supported.

Reproducible: Always
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2010-01-18 15:05:55 PST
I can't find any discussion on this -- was there a mailing list thread about it?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2010-01-19 00:26:47 PST
No, it was added in response to a bug: <http://www.w3.org/Bugs/Public/show_bug.cgi?id=8210>
Comment 3 Martin Kliehm 2010-08-31 05:07:05 PDT
There's work on this topic going on in the W3C HTML Accessibility Task Force, mainly by Richard Schwerdtfeger (IBM) and Steve Faulkner (Paciello Group): http://dev.w3.org/html5/2dcontext/#dom-context-2d-drawfocusring

If there's an #accessibility keyword here I'd suggest to add it to this issue.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-04-17 08:22:50 PDT
<smaug____> drawFocusRing looks pretty useless to me
Comment 6 Jean-Yves Perrier [:teoli] 2011-09-18 04:35:12 PDT
In http://dev.w3.org/html5/html4-differences/ there is:

"The drawFocusRing() method on the canvas 2d context has been split into two methods, drawSystemFocusRing() and drawCustomFocusRing()."

so updating the title of that bug accordingly.

(The reference for these 2 methods in the spec are:
http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#dom-context-2d-drawsystemfocusring
http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#dom-context-2d-drawcustomfocusring )
Comment 7 Anup Kumar [:Anupkumar] 2013-08-12 09:21:14 PDT
Hi, 

I would like to work on this bug. I will try my level best to fix this bug.

Thanks in Advance,

Regards,
A.Anup.
Comment 8 Rich Schwerdtfeger 2013-08-12 09:22:25 PDT
The critical importance of focus rings is that it sets the location of the focus ring bound to an element in fallback content. This gives it its location. Without this a magnifier cannot zoom into the focused object. Fallback elements don't have a position on screen as they are not drawn by the layout engine. 

Both drawSystemFocusRing(Element e) and drawCustomFocusRing(Element e) are running in Chrome Canary and for the first time we can have a magnifier zoom to important focused items on canvas. I have sent David Bolter a test file to assist in testing.
Comment 9 Rich Schwerdtfeger 2013-08-12 09:23:39 PDT
@Anup please send me your email address and I will give you a test file to work with. 

email to schwer@us.ibm.com
Comment 10 Rich Schwerdtfeger 2013-08-16 11:26:01 PDT
I am working on it.
Comment 11 Rich Schwerdtfeger 2013-08-31 21:40:27 PDT
dev-doc:

drawSystemFocusRing(element) and drawCustomFocusRing(element) under the covers:

When these methods are called a corresponding fallback element is passed.

The element passed to these methods MUST be in fallback content ( a descendant of the canvas element ). If not the function should simply return. (In the case of drawCustomFocusRing it should return false.)

Whether the element is focused or not, a call to either drawSystemFocusRing or drawCustomFocusRing implies an association between the element in fallback content and the current path. If the Browser is currently in a mode where it is maintaining accessibility information and communicating with assistive technology, it should set that fallback content element's accessible on-screen location to equal the current path (or the bounding box of that path, if that's all the platform accessibility APIs allow). The element's accessible onscreen location is typically stored in an accessible object, in an accessibility tree corresponding to the element. If accessibility support is not currently enabled this step is not needed.

If the user moves the browser window then the accessible bounding rectangle should be adjusted along with the other elements drawn by the layout engine.

If the element is not focused, there's nothing more to do and the function should return. (In the case of drawCustomFocusRing it should return false.) Note that if the element is becoming focused and we're inside its focus event handler, this function must act as if focus has already moved to this new element. It should not matter whether the element received focus while tabbing, via the element's focus() method, or any other way.

For drawCustomFocusRing, the Browser may want to check to see if the user has requested to override page styles and always use a Browser-drawn focus ring, for accessibility. If so, it should keep going as if the author had called drawSystemFocusRing. If not, it should return true, indicating that the web application should draw its own focus ring.

To draw the focus ring, Browsers must use the convention used by the browser to style the focus ring to match its interpretation of the OS platform system focus ring. In some browsers, this means using the outline style. In these cases drawSystemFocusRing will use the current path and the default outline style used when elements receive focus (e.g. anchor tags). In cases where it's impossible to draw a focus ring along the path itself, it may draw a ring around the path's bounding box.

Note: In addition to drawSystemFocusRing(Element e) and drawCustomFocusRing(Element e) the current Canvas2D API spec. supports a Path object and there are additional methods in the Canvas2D Context that include the Path object: drawSystemFocusRing(Path p, Element e) and drawCustomFocusRing(Path p, Element e). If you have implemented Path consequently these functions use the  Path passed over the current path in calculating the onscreen location in these methods. 

It is the responsibility of the author to make sure the canvas path is complete and closed. If the current path is a line or an unclosed path, the system should draw a focus outline around the path as if it was stroking the path, or draw a focus ring around the bounding box. It should not close the path.

The focus ring should be drawn into the canvas pixels. Any canvas drawing operations that follow this one may be drawn on top of the focus ring, and the focus ring can be read back from the canvas pixels. It is the responsibility of the web author to make sure that the canvas is large enough for the ring to be drawn around any focused element without extending beyond the extent of the canvas.

None of the focus ring drawing functions listed above move DOM focus in the browser.

Also referring to CSS: http://www.w3.org/TR/CSS2/ui.html#dynamic-outlines: "Graphical user interfaces may use outlines around elements to tell the user which element on the page has the focus. These outlines are in addition to any borders, and switching outlines on and off should not cause the document to reflow. The focus is the subject of user interaction in a document (e.g., for entering text, selecting a button, etc.). User agents supporting the interactive media group must keep track of where the focus lies and must also represent the focus. This may be done by using dynamic outlines in conjunction with the :focus pseudo-class.

After drawing, drawSystemFocusRing should return. If drawCustomFocusRing has actually drawn anything to the canvas it should return false, indicating that the web author should not actually render anything.
Comment 12 Rik Cabanier 2013-09-29 13:20:53 PDT
Created attachment 811734 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 13 Rik Cabanier 2013-09-29 13:29:33 PDT
Created attachment 811736 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2013-09-29 13:32:05 PDT
Comment on attachment 811734 [details] [diff] [review]
Add support and testing for system and custom focusring

Review of attachment 811734 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/test_canvas.html
@@ +21487,5 @@
>      ok(same, "changing dimensions broke canvas");
>  }
>  </script>
>  
> +<p>Canvas test: drawCustomFocusRing</p>

This file was/is for Philip Taylor's canvas tests; I'd rather you put these tests in a new file. (Bonus points for also submitting them to web-platform-tests, if you haven't yet.)
Comment 15 Rik Cabanier 2013-09-29 15:18:47 PDT
Created attachment 811749 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 16 Rik Cabanier 2013-09-29 15:20:20 PDT
Created attachment 811750 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 17 Rik Cabanier 2013-09-29 15:22:35 PDT
Created attachment 811751 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 18 Rik Cabanier 2013-09-29 15:27:56 PDT
Created attachment 811752 [details] [diff] [review]
Add support and testing for system and custom focusring

patch queue was confused
Comment 19 alexander :surkov 2013-09-30 05:06:35 PDT
Rik, is it ready for feedback/review?
Comment 20 Rik Cabanier 2013-10-01 13:09:18 PDT
Created attachment 812794 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 21 Rik Cabanier 2013-10-01 13:10:50 PDT
Successful try server: https://tbpl.mozilla.org/?tree=Try&rev=d95868c19ac7
Comment 22 Rik Cabanier 2013-10-01 13:11:47 PDT
(In reply to alexander :surkov from comment #19)
> Rik, is it ready for feedback/review?

I think it is now. Finally got it to pass on all bots.
Comment 23 Rik Cabanier 2013-10-01 13:29:41 PDT
Created attachment 812811 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 24 Rik Cabanier 2013-10-01 13:33:07 PDT
Created attachment 812813 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 25 Olli Pettay [:smaug] 2013-10-01 23:46:32 PDT
Comment on attachment 812794 [details] [diff] [review]
Add support and testing for system and custom focusring

This was marked obsolete, so I assume no review is needed.
Comment 26 Olli Pettay [:smaug] 2013-10-02 01:17:33 PDT
Comment on attachment 812813 [details] [diff] [review]
Add support and testing for system and custom focusring

This will need a review also from a more graphics person.

>+void CanvasRenderingContext2D::DrawSystemFocusRing(mozilla::dom::Element& element)
should be aElement, but the file is on crack on the coding style.

>+{
>+  EnsureUserSpacePath();
>+
>+  if (!mPath) {
>+    return;
>+  }
>+
>+  if(DrawCustomFocusRing(element)) {
>+    Save();
>+
>+    // set state to conforming focus state
>+    CurrentState().globalAlpha = 1.0;
>+    CurrentState().shadowBlur = 0;
>+    CurrentState().shadowOffset.x = 0;
>+    CurrentState().shadowOffset.y = 0;
>+    CurrentState().op = mozilla::gfx::OP_OVER;

ContextState& state = CurrentState();
and then operate on state.

>+
>+    CurrentState().lineCap = CAP_ROUND;
>+    CurrentState().lineJoin = mozilla::gfx::JOIN_MITER_OR_BEVEL;
>+    CurrentState().lineWidth = 1;
>+    FallibleTArray<mozilla::gfx::Float> dash;
>+    CurrentState().dash = dash;
Why not just CurrentState().dash.Clear();

>+
>+    // get the background focus color
>+    nscolor color = LookAndFeel::GetColor(LookAndFeel::eColorID_TextHighlightBackground);
>+    CurrentState().SetColorStyle(STYLE_STROKE, color);
>+    // draw the focus ring
>+    Stroke();
>+
>+    // sert dashing for foreground
sert?

>+    dash.AppendElement(0);
>+    dash.AppendElement(2);
FallibleTArray<mozilla::gfx::Float>& dash = CurrentState().dash;
dash.Append...

But I wonder why such dash [0, 2]
especially given http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#685
I guess I'm missing something here.


>+bool CanvasRenderingContext2D::DrawCustomFocusRing(mozilla::dom::Element& element)
>+{
>+  EnsureUserSpacePath();
>+
>+  HTMLCanvasElement* canvas = GetCanvas();
>+
>+  if (!canvas|| !nsContentUtils::ContentIsDescendantOf(&element, canvas))
>+    return false;
{} with if


>+
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  if (fm) {
>+    // ensure that the element is actually focused
>+    nsCOMPtr<nsIDOMElement> focusedElement;
>+    fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+    if (SameCOMIdentity(element.AsDOMNode(), focusedElement)) {
return early if fails.
perhaps
NS_ENSURE_TRUE(SameCOMIdentity(element.AsDOMNode(), focusedElement), false);


>+      // get the bounds of the current path
>+      mgfx::Rect bounds;
>+      bounds = mPath->GetBounds(mTarget->GetTransform());
>+
>+      // and set them as the accessible area
>+      nsRect R(canvas->ClientLeft() + bounds.x, canvas->ClientTop() + bounds.y,
>+               bounds.width, bounds.height);
variables should start with lowercase letter.


>+      R.x *= AppUnitsPerCSSPixel();
>+      R.y *= AppUnitsPerCSSPixel();
>+      R.width *= AppUnitsPerCSSPixel();
>+      R.height *= AppUnitsPerCSSPixel();
>+
>+      nsIFrame* frame = element.GetPrimaryFrame();
>+      if(frame)
>+        frame->SetRect(R);
{} with if.
aha, this is the odd a11y case. Needs review also from an a11y person.
But should we limit the size.



>+++ b/content/canvas/test/test_canvas.html
We should have also some reftests for this.


>+<p>Canvas test: drawCustomFocusRing</p>
>+<canvas id="c687" class="output" width="100" height="50">
>+    <input id="button1" type="range" min="1" max="12"></input>
>+    <input id="button2" type="range" min="1" max="12"></input>
>+</canvas>
>+<script type="text/javascript">
>+function test_drawCustomFocusRing_canvas() {
>+    var c = document.getElementById("c687");
>+    var ctx = c.getContext("2d");
>+	ctx.beginPath();
You have some tabs here.
Comment 27 Rik Cabanier 2013-10-02 10:16:42 PDT
(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 812813 [details] [diff] [review]
> Add support and testing for system and custom focusring
> 
> This will need a review also from a more graphics person.
> 
> >+void CanvasRenderingContext2D::DrawSystemFocusRing(mozilla::dom::Element& element)
> should be aElement, but the file is on crack on the coding style.

fixed

> 
> >+{
> >+  EnsureUserSpacePath();
> >+
> >+  if (!mPath) {
> >+    return;
> >+  }
> >+
> >+  if(DrawCustomFocusRing(element)) {
> >+    Save();
> >+
> >+    // set state to conforming focus state
> >+    CurrentState().globalAlpha = 1.0;
> >+    CurrentState().shadowBlur = 0;
> >+    CurrentState().shadowOffset.x = 0;
> >+    CurrentState().shadowOffset.y = 0;
> >+    CurrentState().op = mozilla::gfx::OP_OVER;
> 
> ContextState& state = CurrentState();
> and then operate on state.

fixed

> 
> >+
> >+    CurrentState().lineCap = CAP_ROUND;
> >+    CurrentState().lineJoin = mozilla::gfx::JOIN_MITER_OR_BEVEL;
> >+    CurrentState().lineWidth = 1;
> >+    FallibleTArray<mozilla::gfx::Float> dash;
> >+    CurrentState().dash = dash;
> Why not just CurrentState().dash.Clear();

fixed

> 
> >+
> >+    // get the background focus color
> >+    nscolor color = LookAndFeel::GetColor(LookAndFeel::eColorID_TextHighlightBackground);
> >+    CurrentState().SetColorStyle(STYLE_STROKE, color);
> >+    // draw the focus ring
> >+    Stroke();
> >+
> >+    // sert dashing for foreground
> sert?

fixed

> 
> >+    dash.AppendElement(0);
> >+    dash.AppendElement(2);
> FallibleTArray<mozilla::gfx::Float>& dash = CurrentState().dash;
> dash.Append...
> 
> But I wonder why such dash [0, 2]
> especially given
> http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#685
> I guess I'm missing something here.

Yeah. I had 1px originally, but it was just very hard to see the focus against the content of the canvas.
I could change it back if you want me to.

> 
> 
> >+bool CanvasRenderingContext2D::DrawCustomFocusRing(mozilla::dom::Element& element)
> >+{
> >+  EnsureUserSpacePath();
> >+
> >+  HTMLCanvasElement* canvas = GetCanvas();
> >+
> >+  if (!canvas|| !nsContentUtils::ContentIsDescendantOf(&element, canvas))
> >+    return false;
> {} with if

fixed

> 
> 
> >+
> >+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> >+  if (fm) {
> >+    // ensure that the element is actually focused
> >+    nsCOMPtr<nsIDOMElement> focusedElement;
> >+    fm->GetFocusedElement(getter_AddRefs(focusedElement));
> >+    if (SameCOMIdentity(element.AsDOMNode(), focusedElement)) {
> return early if fails.
> perhaps
> NS_ENSURE_TRUE(SameCOMIdentity(element.AsDOMNode(), focusedElement), false);

ah. sorry, this function is supposed to check if the element that is passed in, is focused. There's no need for an assert.
I updated the comment to make this more clear.

> 
> 
> >+      // get the bounds of the current path
> >+      mgfx::Rect bounds;
> >+      bounds = mPath->GetBounds(mTarget->GetTransform());
> >+
> >+      // and set them as the accessible area
> >+      nsRect R(canvas->ClientLeft() + bounds.x, canvas->ClientTop() + bounds.y,
> >+               bounds.width, bounds.height);
> variables should start with lowercase letter.
> 
> 
> >+      R.x *= AppUnitsPerCSSPixel();
> >+      R.y *= AppUnitsPerCSSPixel();
> >+      R.width *= AppUnitsPerCSSPixel();
> >+      R.height *= AppUnitsPerCSSPixel();
> >+
> >+      nsIFrame* frame = element.GetPrimaryFrame();
> >+      if(frame)
> >+        frame->SetRect(R);
> {} with if.
> aha, this is the odd a11y case. Needs review also from an a11y person.
> But should we limit the size.

What do you mean by that?

> 
> 
> 
> >+++ b/content/canvas/test/test_canvas.html
> We should have also some reftests for this.

I thought about creating a reftest but I'm not sure how I can do that.

> 
> 
> >+<p>Canvas test: drawCustomFocusRing</p>
> >+<canvas id="c687" class="output" width="100" height="50">
> >+    <input id="button1" type="range" min="1" max="12"></input>
> >+    <input id="button2" type="range" min="1" max="12"></input>
> >+</canvas>
> >+<script type="text/javascript">
> >+function test_drawCustomFocusRing_canvas() {
> >+    var c = document.getElementById("c687");
> >+    var ctx = c.getContext("2d");
> >+	ctx.beginPath();
> You have some tabs here.

fixed
Comment 28 Rik Cabanier 2013-10-02 10:27:11 PDT
Created attachment 813181 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 29 Rik Cabanier 2013-10-02 10:28:40 PDT
Created attachment 813182 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 30 David Bolter [:davidb] 2013-10-02 18:19:53 PDT
Comment on attachment 813182 [details] [diff] [review]
Add support and testing for system and custom focusring

Review of attachment 813182 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Rik. Thanks for hacking on this. The patch didn't apply cleanly for me and I didn't build and test it yet.

One thing I'm concerned about is that (as you know) the API is under active discussion[1]. In particular "drawCustomFocusRing" is seen as poorly named, and as a stop-gap with potentially better solutions a bit further out. I'd like to see the API you introduce here hidden behind a config option. If there is agreement on approach and naming we can then flip the switch. Does this sound reasonable?

1. http://lists.w3.org/Archives/Public/public-html-a11y/2013Sep/0138.html
Comment 31 Rik Cabanier 2013-10-02 18:30:36 PDT
(In reply to David Bolter [:davidb] from comment #30)
> Comment on attachment 813182 [details] [diff] [review]
> Add support and testing for system and custom focusring
> 
> Review of attachment 813182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Rik. Thanks for hacking on this. The patch didn't apply cleanly for me
> and I didn't build and test it yet.
> 
> One thing I'm concerned about is that (as you know) the API is under active
> discussion[1]. In particular "drawCustomFocusRing" is seen as poorly named,
> and as a stop-gap with potentially better solutions a bit further out. I'd
> like to see the API you introduce here hidden behind a config option. If
> there is agreement on approach and naming we can then flip the switch. Does
> this sound reasonable?
> 
> 1. http://lists.w3.org/Archives/Public/public-html-a11y/2013Sep/0138.html

We've continued to discuss this: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-September/040925.html

I'm OK with landing this behind a flag. Are there examples on how to do this in IDL?
Comment 32 David Bolter [:davidb] 2013-10-02 18:34:42 PDT
Thanks for the link.

> I'm OK with landing this behind a flag. Are there examples on how to do this
> in IDL?

I don't know off hand but I just asked in #developers...
Comment 33 Rik Cabanier 2013-10-02 18:41:17 PDT
It's still applying for me with the latest from http://hg.mozilla.org/mozilla-central/
Are you on that tree too?
Comment 34 Rik Cabanier 2013-10-02 20:30:09 PDT
Created attachment 813430 [details] [diff] [review]
Add support and testing for system and custom focusring

changed patch so focus rings can be turned on in flags
Comment 35 Rik Cabanier 2013-10-02 20:31:50 PDT
(In reply to David Bolter [:davidb] from comment #32)
> Thanks for the link.
> 
> > I'm OK with landing this behind a flag. Are there examples on how to do this
> > in IDL?
> 
> I don't know off hand but I just asked in #developers...

I figured it out and updated the patch
Comment 36 David Bolter [:davidb] 2013-10-03 06:51:20 PDT
Comment on attachment 813430 [details] [diff] [review]
Add support and testing for system and custom focusring

Review of attachment 813430 [details] [diff] [review]:
-----------------------------------------------------------------

f=me thanks.

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +77,5 @@
>    void fill([TreatUndefinedAs=Missing] optional CanvasWindingRule winding = "nonzero");
>  // NOT IMPLEMENTED  void fill(Path path);
>    void stroke();
>  // NOT IMPLEMENTED  void stroke(Path path);
> +  [Pref="canvas.focusring.enabled"] void drawSystemFocusRing(Element element);

Neato.
Comment 37 David Bolter [:davidb] 2013-10-03 06:52:15 PDT
(In reply to Rik Cabanier from comment #33)
> It's still applying for me with the latest from
> http://hg.mozilla.org/mozilla-central/
> Are you on that tree too?

Aha. I'm on inbound but for no good reason.
Comment 38 Rik Cabanier 2013-10-08 10:07:33 PDT
(In reply to Olli Pettay [:smaug] from comment #26)

Any chance you could review this?
Comment 39 Olli Pettay [:smaug] 2013-10-08 11:53:12 PDT
Could you still explain why the focus ring is so different from what is used normally.
And the patch is missing reftests.
https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests
Comment 40 Rik Cabanier 2013-10-08 12:11:01 PDT
(In reply to Olli Pettay [:smaug] from comment #39)
> Could you still explain why the focus ring is so different from what is used
> normally.

Normally, you use the color of the element to draw the focus ring. Since there is no element for canvas  (since it's immediate), I chose a default focus color. The style of the ring should be the same since I copied that from another place in the ff codebase.

> And the patch is missing reftests.
> https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests

I'm unsure how to create a reftest since the focus ring color might change per platform...
Comment 41 Olli Pettay [:smaug] 2013-10-08 14:10:15 PDT
Comment on attachment 813430 [details] [diff] [review]
Add support and testing for system and custom focusring

I assume a new patch is coming (something which gives better focus ring and which has at least some reftest).
Comment 42 Rik Cabanier 2013-10-08 14:49:25 PDT
Created attachment 814579 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 43 Rik Cabanier 2013-10-08 17:11:53 PDT
Created attachment 814645 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 44 Olli Pettay [:smaug] 2013-10-09 03:32:47 PDT
Comment on attachment 814645 [details] [diff] [review]
Add support and testing for system and custom focusring

>@@ -83,16 +84,17 @@
> #include "mozilla/unused.h"
> #include "nsCCUncollectableMarker.h"
> #include "nsWrapperCacheInlines.h"
> #include "mozilla/dom/CanvasRenderingContext2DBinding.h"
> #include "mozilla/dom/HTMLImageElement.h"
> #include "mozilla/dom/HTMLVideoElement.h"
> #include "mozilla/dom/TextMetrics.h"
> #include "mozilla/dom/UnionTypes.h"
>+#include "mozilla/LookAndFeel.h"
You don't actually use this anymore.


>+    // set the background focus color
Could you add a comment here that we try to draw similar focus ring as we draw
for image maps.


>+SpecialPowers.setBoolPref("canvas.focusring.enabled", true);
You should reset the pref value to the initial once the test has been run.
Comment 45 Olli Pettay [:smaug] 2013-10-09 04:11:01 PDT
https://tbpl.mozilla.org/?tree=Try&rev=2fc81efe3ea3
Comment 46 Rik Cabanier 2013-10-09 09:29:27 PDT
Created attachment 814946 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 47 Ryan VanderMeulen [:RyanVM] 2013-10-09 10:03:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8517afe50156
Comment 48 Ryan VanderMeulen [:RyanVM] 2013-10-09 12:13:17 PDT
Backed out for reftest failures. Might just need some fuzzy-if love, but I'd rather you decide that after a Try run that me doing it on a closed inbound tree.
https://hg.mozilla.org/integration/mozilla-inbound/rev/541e79e84ce9

https://tbpl.mozilla.org/php/getParsedLog.php?id=28885868&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28887364&tree=Mozilla-Inbound
Comment 49 Rich Schwerdtfeger 2013-10-11 11:38:20 PDT
Is this in a nightly build I can try out.
Comment 50 Rik Cabanier 2013-10-14 16:50:36 PDT
Created attachment 816861 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 51 Rik Cabanier 2013-10-14 22:00:01 PDT
try run: https://tbpl.mozilla.org/?tree=Try&rev=e201b4c3f971
Comment 52 Ryan VanderMeulen [:RyanVM] 2013-10-15 08:41:31 PDT
Your Try run is still showing reftest failures....
Comment 53 Rik Cabanier 2013-10-15 11:31:24 PDT
Created attachment 817294 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 54 Rik Cabanier 2013-10-27 21:20:18 PDT
I'm having a hard time getting focus to work in reftests.
Can I skip them for now?
Comment 55 :Ms2ger (⌚ UTC+1/+2) 2013-10-28 03:19:01 PDT
Try adding the needs-focus keyword?
Comment 56 Rik Cabanier 2013-10-28 09:45:47 PDT
Where is that flag set?
Comment 57 Rik Cabanier 2013-11-01 19:15:56 PDT
Created attachment 826225 [details] [diff] [review]
Add support and testing for system and custom focusring
Comment 58 Rik Cabanier 2013-11-01 19:16:53 PDT
try server: https://tbpl.mozilla.org/?tree=Try&rev=bbe2422e3132
It seems that the b2g build did not pick up the change.
Comment 59 Rik Cabanier 2013-11-03 18:31:47 PST
Created attachment 826568 [details] [diff] [review]
Add support and testing for system and custom focusring

successfull try run: https://tbpl.mozilla.org/?tree=Try&rev=4fb87080abf0
Comment 60 Ryan VanderMeulen [:RyanVM] 2013-11-04 11:58:28 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/10cf6a630944
Comment 61 Wes Kocher (:KWierso) 2013-11-04 21:13:53 PST
https://hg.mozilla.org/mozilla-central/rev/10cf6a630944
Comment 62 Florian Scholz [:fscholz] (MDN) 2015-01-12 03:10:36 PST
https://developer.mozilla.org/en-US/Firefox/Releases/28
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.drawSystemFocusRing which redirects to https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.drawFocusIfNeeded

I am not documenting drawCustomFocusRing as it is behind a flag, not in the spec, no where else implemented, and I filed bug 1120371 so that we get rid of it, too.
Comment 63 Rich Schwerdtfeger 2015-02-16 07:45:15 PST
one bug is still outstanding on this. When you tab to the focused item in fallback content the location of that object must scroll into view. 

http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas_CR/#dom-context-2d-drawfocusifneeded

See this text: 

"If the focus area is not on the screen, then scroll the focus outline into view by aligning it to the top when it receives focus."

Note You need to log in before you can comment on or make changes to this bug.