540456 - Support HTML5 canvas draw{Custom,System}FocusRing()

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, enhancement)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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] (needinfo me, slow to respond)]

I can't find any discussion on this -- was there a mailing list thread about it?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

No, it was added in response to a bug: <http://www.w3.org/Bugs/Public/show_bug.cgi?id=8210>

Comment 3

[Martin Kliehm]

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 4

[Martin Kliehm]

For the discussion, please see:
http://www.w3.org/Bugs/Public/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=a11y_canvas&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&emailtype1=substring&email1=&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

<smaug____> drawFocusRing looks pretty useless to me

Comment 6

[Jean-Yves Perrier [:teoli]]

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 [:reznord]]

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]

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]

@Anup please send me your email address and I will give you a test file to work with. 

email to [e-mail]

Comment 10

[Rich Schwerdtfeger]

I am working on it.

Comment 11

[Rich Schwerdtfeger]

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]

Attachment: Add support and testing for system and custom focusring

Comment 13

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]

Attachment: Add support and testing for system and custom focusring

Comment 16

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 17

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 18

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

patch queue was confused

Comment 19

[alexander :surkov (:asurkov)]

Rik, is it ready for feedback/review?

Comment 20

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 21

[Rik Cabanier]

Successful try server: https://tbpl.mozilla.org/?tree=Try&rev=d95868c19ac7

Comment 22

[Rik Cabanier]

(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]

Attachment: Add support and testing for system and custom focusring

Comment 24

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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]

(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]

Attachment: Add support and testing for system and custom focusring

Comment 29

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

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]

(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] (NeedInfo me for attention)]

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]

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]

Attachment: Add support and testing for system and custom focusring

changed patch so focus rings can be turned on in flags

Comment 35

[Rik Cabanier]

(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] (NeedInfo me for attention)]

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] (NeedInfo me for attention)]

(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]

(In reply to Olli Pettay [:smaug] from comment #26)

Any chance you could review this?

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

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]

(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][[e-mail]]]

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]

Attachment: Add support and testing for system and custom focusring

Comment 43

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=2fc81efe3ea3

Comment 46

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8517afe50156

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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]

Is this in a nightly build I can try out.

Comment 50

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 51

[Rik Cabanier]

try run: https://tbpl.mozilla.org/?tree=Try&rev=e201b4c3f971

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Your Try run is still showing reftest failures....

Comment 53

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 54

[Rik Cabanier]

I'm having a hard time getting focus to work in reftests.
Can I skip them for now?

Comment 55

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Try adding the needs-focus keyword?

Comment 56

[Rik Cabanier]

Where is that flag set?

Comment 57

[Rik Cabanier]

Attachment: Add support and testing for system and custom focusring

Comment 58

[Rik Cabanier]

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]

Attachment: 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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10cf6a630944

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/10cf6a630944

Comment 62

[Florian Scholz (Open Web Docs)]

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]

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."

Comment 64

[Rich Schwerdtfeger]

See the following detailed text regarding scrolling into view:

https://www.w3.org/wiki/HTML/Canvas_Task_Force/CR-Test#drawFocusIfNeeded_when_a_default_path_is_provided_and_the_associated_fallback_element_is_descendant_of_the_element_with_focus._Scroll_the_window_so_that_the_canvas_moves.