Comment on attachment 8415990 [details] [diff] [review] Change pref so drawFocusIfNeeded is enabled by default I'm not the reviewer you're looking for.

From Ehsan: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > Can you give me an example where 1004499 would be triggered? I will > update that bug with an extra test. We only draw the focus ring if something is focused by the keyboard. In order to simulate that in a mochitest, you can give your DOM element a @tabindex attribute, and then use syntehsizeKey to generate VK_TAB events to focus that DOM element. In order to test the opposite case, you can use synthesizeMouse to simulate setting the focus using the mouse. > Is that always the case? I think so, for content at least. > In our tests, we can also use elements that can get the focus by default (such as <input> and <a>) > and we can focus them by calling 'focus()' on the element. Please check with Neil Deakin, he really knows this stuff the best. I just wanted to make sure we have good test coverage here. <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Neil, do you think we need a test for what Ehsan describes? Note that the fallback elements inside a canvas are not visible on screen (in case that makes a difference) I assumed that calling 'focus' on an element was enough.

Can you explain how one is normally expected to use the drawFocusIfNeeded method? How would one typically focus the elements that get passed to it?

(In reply to Neil Deakin from comment #4) > Can you explain how one is normally expected to use the drawFocusIfNeeded > method? How would one typically focus the elements that get passed to it? The typical use case is to use the tab key. (You can also set it with the focus() method)

(In reply to Neil Deakin from comment #4) > Can you explain how one is normally expected to use the drawFocusIfNeeded > method? How would one typically focus the elements that get passed to it? my understanding that drawFocusIfNeeded draws a focus ring around focused element. If element is not focused then it has zero outcome.

I understand what the function does, but what I don't fully understand when it is meant to be called. My understanding is that the element passed to drawFocusIfNeeded is not visible, so there is no means of clicking on it with the mouse to focus it. A key distinction as to whether focus rings should be drawn on Windows is whether the element was focused with the mouse or the keyboard. Since the former isn't possible, the method cannot properly determine whether a focus ring should be drawn or not.

If I understand correct then drawFocusIfNeeded draws the focus unconditionally not depending how the element got focused. If so then the author is supposed to figure out that himself to decide whether drawFocusIfNeeded should be called or not. I guess he can do some tricks like sniffing tab key presses and consequent focus event but it may be difficult. Is that your point?

(In reply to alexander :surkov from comment #8) > If I understand correct then drawFocusIfNeeded draws the focus > unconditionally not depending how the element got focused. If so then the > author is supposed to figure out that himself to decide whether > drawFocusIfNeeded should be called or not. The author doesn't have access to this information. The specification says the drawFocusIfNeeded function should perform this task. The goal here is to have a test that can verify the correct behaviour but it's not clear to me how the function is expected to be used to determine what the correct behaviour is.

(In reply to Neil Deakin from comment #7) > I understand what the function does, but what I don't fully understand when > it is meant to be called. (In reply to Neil Deakin from comment #7) > I understand what the function does, but what I don't fully understand when > it is meant to be called. The author is supposed to call it when it is drawing a user interface in canvas. For instance, after drawing a button, he would set up the outline for the button and then call drawFocusIfNeeded with a fallback element that represents the button. If that button was focused, the browser would then draw a line with the focus style around the outline. Here is an example: http://codepen.io/anon/pen/avCnD Make sure about:config -> canvas.focusring.enabled = true Then click on the clock and use the tab key to highlight the minute and second hands

(In reply to alexander :surkov from comment #8) > If I understand correct then drawFocusIfNeeded draws the focus > unconditionally not depending how the element got focused. No, as the name implies, drawFocusIfNeeded will only draw the outline if it is needed :-)

(In reply to Neil Deakin from comment #9) > The goal here is to have a test that can verify the correct behaviour but > it's not clear to me how the function is expected to be used to determine > what the correct behaviour is. I think I still miss your point. Does Rik's comment #10 address it? Also do I understand right that we still don't have good test coverage to turn the feature on by default?

Here is the point. The focus should only be drawn when the system would draw a focus ring for the element passed in. But you can't focus the element in such a way that would not draw the focus ring, since you can't click on it with the mouse. Thus, you cannot test the change made in bug 1004499, nor can an author properly draw focus rings. It seems to me like this whole function is some broken misfeature, designed only to let authors do the wrong thing.

(In reply to Neil Deakin from comment #13) > Here is the point. The focus should only be drawn when the system would draw > a focus ring for the element passed in. But you can't focus the element in > such a way that would not draw the focus ring, since you can't click on it > with the mouse. true at least until we implement event redirection (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvas-mouseevent-rerouting-steps) > Thus, you cannot test the change made in bug 1004499, nor can an author > properly draw focus rings. agree > It seems to me like this whole function is some broken misfeature, designed > only to let authors do the wrong thing. what's wrong about it?

(In reply to Neil Deakin from comment #13) > Here is the point. The focus should only be drawn when the system would draw > a focus ring for the element passed in. But you can't focus the element in > such a way that would not draw the focus ring, since you can't click on it > with the mouse. How about using the keyboard? That would focus the element. > Thus, you cannot test the change made in bug 1004499, nor can an author > properly draw focus rings. Is that change needed? Note that I asked how I could test it. > It seems to me like this whole function is some broken misfeature, designed > only to let authors do the wrong thing. I don't understand this either. What wrong thing would the author do? Note that fallback content CAN become visible if canvas rendering is turned off for people with severe vision problems.

(In reply to Rik Cabanier from comment #15) > (In reply to Neil Deakin from comment #13) > > Here is the point. The focus should only be drawn when the system would draw > > a focus ring for the element passed in. But you can't focus the element in > > such a way that would not draw the focus ring, since you can't click on it > > with the mouse. > > How about using the keyboard? That would focus the element. the point is as long as you cannot click on shadow DOM element then drawFocusIfNeeded always draws focus ring around focused element, so that part is not testable (perhaps this is ok for now).

(In reply to alexander :surkov from comment #16) > (In reply to Rik Cabanier from comment #15) > > (In reply to Neil Deakin from comment #13) > > > Here is the point. The focus should only be drawn when the system would draw > > > a focus ring for the element passed in. But you can't focus the element in > > > such a way that would not draw the focus ring, since you can't click on it > > > with the mouse. > > > > How about using the keyboard? That would focus the element. > > the point is as long as you cannot click on shadow DOM element then > drawFocusIfNeeded always draws focus ring around focused element, so that > part is not testable (perhaps this is ok for now). Would there be a way to "click" on it using a11y tools?

(In reply to Rik Cabanier from comment #15) > How about using the keyboard? That would focus the element. Yes, when you focus an element with the tab key it will be focused and draw a focus ring. If you had instead clicked on an element with the mouse on Windows, it will be focused, but should *not* draw the focus ring. > true at least until we implement event redirection (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvas-mouseevent-rerouting-steps) If that's the case, we can test this properly when that is implemented.

(In reply to Rik Cabanier from comment #17) > Would there be a way to "click" on it using a11y tools? you can use a11y API to "click" the element, I have doubts it works for canvas shadow DOM though. (In reply to Neil Deakin from comment #18) > > true at least until we implement event redirection (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvas-mouseevent-rerouting-steps) > > If that's the case, we can test this properly when that is implemented. are there any objections to not enable this feature by default?

> are there any objections to not enable this feature by default? I don't. I was trying to determine how to test it as asked above.

Comment on attachment 8415990 [details] [diff] [review] Change pref so drawFocusIfNeeded is enabled by default Review of attachment 8415990 [details] [diff] [review]: ----------------------------------------------------------------- so if there's nothing preventing the turning the feature on then let's proceed with it.

https://developer.mozilla.org/en-US/Firefox/Releases/32#Interfaces.2FAPIs.2FDOM https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.drawFocusIfNeeded#Compatibility_notes