935383 - Focusrings are mispositioned

Status: RESOLVED WONTFIX

(Core :: Graphics: Canvas2D, defect)

Description

[Rik Cabanier]

If the element that is associated with a focusring, is not at position (0,0), the focusring is offset by that amount.

Also, a11y software was only notified of the focus area if the element was focused. This needs to happen even if the element is not focused.

Comment 1

[Rik Cabanier]

Attachment: Patch for misaligned focus areas

Comment 2

[Rik Cabanier]

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

Comment 3

[Rich Schwerdtfeger]

OK, email me when the nightly is updated and I will give it another go. 

I have not yet tried drawCustomFocusRing()

Comment 4

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

(In reply to Rik Cabanier from comment #0)
> Also, a11y software was only notified of the focus area if the element was
> focused. This needs to happen even if the element is not focused.
Why? That is not what the spec says.
The spec pretty clearly say in step 1
"If element is not focused or is not a descendant of the element with whose context the method is associated, then return false and abort these steps."

Comment 5

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

Comment on attachment 827836 [details] [diff] [review]
Patch for misaligned focus areas

And we need some test for this. I assume using drawSystemFocusRing for the
test would make testing easier.

Comment 6

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

(In reply to Rik Cabanier from comment #0)
> Also, a11y software was only notified of the focus area if the element was
> focused. This needs to happen even if the element is not focused.
If you think this should be the case, please get the spec changed.
(But I don't understand why that behavior is wanted)

Comment 7

[Rik Cabanier]

Attachment: Patch for misaligned focus areas (

Comment 8

[Rik Cabanier]

(In reply to Olli Pettay [:smaug] from comment #6)
> (In reply to Rik Cabanier from comment #0)
> > Also, a11y software was only notified of the focus area if the element was
> > focused. This needs to happen even if the element is not focused.
> If you think this should be the case, please get the spec changed.
> (But I don't understand why that behavior is wanted)

I updated the patch so it just addresses the failure case (and not always update the a11y regions).
How would I write a test for this? Can you point me to an example that verifies the focus area for a11y?

Comment 9

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

Hmm, actually, I'm not sure.

Alex, you might know if we could test this.

Comment 10

[alexander :surkov (:asurkov)]

I don't know much about how layout/visual testing works/implemented in Firefox but if a11y API is supposed to be used for testing then first of all a11y needs a fix to pick up focus drawn in canvas as accessible boundaries.

Comment 11

[Rik Cabanier]

Since we can't test this, could it be landed as-is?

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 829809 [details] [diff] [review]
Patch for misaligned focus areas (

the only way I can see I can grant f+ is I should try the patch. If that's what you look for then could you please provide me a try server build and testcase?

On the another hand I'm pretty sure that you don't really need to rely on a11y for testing here, Gecko core should have patterns to test visual stuff.

Olli, do you know anything about it?

Comment 13

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

The frame tree under canvas is there only because of a11y.

Comment 14

[Rik Cabanier]

(In reply to alexander :surkov from comment #12)
> Comment on attachment 829809 [details] [diff] [review]
> Patch for misaligned focus areas (
> 
> the only way I can see I can grant f+ is I should try the patch. If that's
> what you look for then could you please provide me a try server build and
> testcase?
> 
Yes, try server: https://tbpl.mozilla.org/?tree=Try&rev=9b296b80861c
I've attached canvaseditor.html that demonstrates the issue. To repro, turn on the focus rings (about:config -> canvas.focusring.enabled), open the doc and tab a couple of times to highlight the elements.
Now, the easiest way to see the hit area, is to right-click and inspect the canvas element. Now, descend to the 2 input elements that are children of the canvas elements and look at their bounding boxes. Without the patch, you will see that the second area is misaligned.

Comment 15

[Rik Cabanier]

Attachment: Test case for focus rings

Comment 16

[Rik Cabanier]

Sorry that I didn't post a try run; I did run one but forgot to attach it.
The test file can also be found here: http://www.w3.org/2013/08/canvas-editor.html

Comment 17

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

Comment on attachment 829809 [details] [diff] [review]
Patch for misaligned focus areas (

>-      // get the bounds of the current path
>+       // get the bounds of the current path
Extra space

>+        nsIFrame* parentFrame = frame->GetParent();
>+        while(parentFrame && (parentFrame != canvas->GetPrimaryFrame())) {
>+          nsRect parentRect = parentFrame->GetRect();
>+          rect.x -= parentRect.x;
>+          rect.y -= parentRect.y;
>+
>+          parentFrame = parentFrame->GetParent();
>+        }
>+
We should have some helper to do this 
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h?rev=04054b2172f3#525 or some such
And this is getting more and more layout stuff, so better to ask roc or someone to review.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 829809 [details] [diff] [review]
Patch for misaligned focus areas (

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

You should probably take account of CSS transforms and that sort of thing. So use nsLayoutUtils::GetTransformToAncestor and gfx3DMatrix::TransformBounds.

Comment 19

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 829809 [details] [diff] [review]
> Patch for misaligned focus areas (
> 
> Review of attachment 829809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should probably take account of CSS transforms and that sort of thing.
> So use nsLayoutUtils::GetTransformToAncestor and
> gfx3DMatrix::TransformBounds.

Are CSS transforms applied in the canvas shadow dom? I will experiment...

Comment 20

[Rik Cabanier]

Attachment: Patch for misaligned focus areas

integrated Roc's comments by using nsLayoutUtils::GetTransformToAncestor and gfx3DMatrix::TransformBounds. 
Test file is passing now (but I can't make it part of the patch since there's no way to test it)

Comment 21

[Rik Cabanier]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=847c31ec20ef

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8356959 [details] [diff] [review]
Patch for misaligned focus areas

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +1801,5 @@
> +
> +      // the bounds are relative to the containing element. Adjust if the fallback content is under other elements 
> +      if (frame && frame->GetParent() && (frame->GetParent()!= canvas->GetPrimaryFrame())) {
> +        gfx3DMatrix transform = nsLayoutUtils::GetTransformToAncestor(frame->GetParent(), canvas->GetPrimaryFrame());
> +        rect = transform.Inverse().TransformBounds(rect);      }

Move trailing }

@@ +1814,1 @@
>        }

Hmm, this looks nasty. We should only set frame rects during reflow, we shouldn't set them in response to DOM methods. Another problem is that we're not actually drawing anything to the canvas here!

This entire function seems wrong to me. I have no idea how it is supposed to work.

We don't have any code in Gecko that draws a native focus ring over an arbitrary path. We can draw some native widgets with a focused look. We probably should extend nsITheme.h with a new Draw method that draws a focus ring around a path to a given rendering context.

Comment 23

[Rik Cabanier]

Attachment: Patch for misaligned focus areas

Fixed misaligned '}'

Comment 24

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Hmm, this looks nasty. We should only set frame rects during reflow, we
> shouldn't set them in response to DOM methods. 
Yes, this is a bit wonky. Are you worried that if there's reflow, the focus area will be wrong?
For this API to work correctly, the author is supposed to call this function in a requestAnimationFrame callback.

> Another problem is that we're
> not actually drawing anything to the canvas here!

That is correct. For the drawCustomFocusRing API, the author is supposed to draw the ring.
drawSystemFocusRing (which calls drawCustomFocusRing) will draw a ring around the path using the focus style.

> 
> This entire function seems wrong to me. I have no idea how it is supposed to
> work.

Almost everyone gets confused when they first encounter this API :-)

> 
> We don't have any code in Gecko that draws a native focus ring over an
> arbitrary path. We can draw some native widgets with a focused look. We
> probably should extend nsITheme.h with a new Draw method that draws a focus
> ring around a path to a given rendering context.

The spec calls to draw a path using the focused style. This is what I implemented in drawSystemFocusRing.
This is also how blink is implemented. (They of course have a different focus style)

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Rik Cabanier from comment #24)
> That is correct. For the drawCustomFocusRing API, the author is supposed to
> draw the ring.

OK. So why don't we just check if the element is focused and return true if it is? Why do we need to move the frame?

> > This entire function seems wrong to me. I have no idea how it is supposed to
> > work.
> 
> Almost everyone gets confused when they first encounter this API :-)

That may indicate a problem with the API :-)

Comment 26

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> (In reply to Rik Cabanier from comment #24)
> > That is correct. For the drawCustomFocusRing API, the author is supposed to
> > draw the ring.
> 
> OK. So why don't we just check if the element is focused and return true if
> it is? Why do we need to move the frame?

That is for the a11y APIs that need to draw a rectangle around the focus region.
They need to know the area that the fallback content (with its ARIA rules) represents.

> 
> > > This entire function seems wrong to me. I have no idea how it is supposed to
> > > work.
> > 
> > Almost everyone gets confused when they first encounter this API :-)
> 
> That may indicate a problem with the API :-)

true, it's a complicated API but it is a complicated problem. It's seen a lot of discussion and it is still ongoing. (see current whatwg)
I wish there was a way to make it simpler and that it had a better name, but it didn't happen. 

The only thing the API has going for it, is that it is very simple to use which should lower the bar for authors to make their canvas applications accessible and it can fall back on the a11y support from HTML (so we don't have to invent something completely new for canvas such as the hitregions).

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)

> > Almost everyone gets confused when they first encounter this API :-)
> 
> That may indicate a problem with the API :-)

I have impression that everybody is not happy about this API. I hear opinions that having something is better than nothing so it could turn to be a temporary API (nothing more persistent than temporary though).

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Rik Cabanier from comment #26)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> > (In reply to Rik Cabanier from comment #24)
> > > That is correct. For the drawCustomFocusRing API, the author is supposed to
> > > draw the ring.
> > 
> > OK. So why don't we just check if the element is focused and return true if
> > it is? Why do we need to move the frame?
> 
> That is for the a11y APIs that need to draw a rectangle around the focus
> region.
> They need to know the area that the fallback content (with its ARIA rules)
> represents.

OK.

On the WHATWG list Ian is arguing that drawCustomFocusRing should not do this, and addHitRegion is for this. I'm not sure who's right, but it's definitely not a settled question.

If we want drawCustomFocusRing to change what accessibility returns, mashing the element's frame rect here is not the way to do it. A much better approach would be to attach a copy of the path to the frame as a frame property, and have the accessibility code check for that property and use it instead of the frame geometry when present. We can store the path in device pixels relative to the canvas and then accessibility can find the relevant canvas frame and do the necessary coordinate conversions itself.

Comment 29

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> On the WHATWG list Ian is arguing that drawCustomFocusRing should not do
> this, and addHitRegion is for this. I'm not sure who's right, but it's
> definitely not a settled question.

Yeah, Ian has a misunderstanding how the a11y software works.
I'm also not familiar with the matter but Rich (from IBM) and Dominic (from Google) tell me that this is how it is supposed to work.
It's been frustrating at times... 

> 
> If we want drawCustomFocusRing to change what accessibility returns, mashing
> the element's frame rect here is not the way to do it. A much better
> approach would be to attach a copy of the path to the frame as a frame
> property, and have the accessibility code check for that property and use it
> instead of the frame geometry when present. We can store the path in device
> pixels relative to the canvas and then accessibility can find the relevant
> canvas frame and do the necessary coordinate conversions itself.

How would that be different or better from the current solution? afaik the a11y code just wants to know the bounds, it is not set up to deal with paths.
Also, the path should not be associated with the canvas, it is supposed to be associated with the fallback/shadow content that lives inside the canvas element.

Comment 30

[Rik Cabanier]

(In reply to alexander :surkov from comment #27)
> 
> I have impression that everybody is not happy about this API. I hear
> opinions that having something is better than nothing so it could turn to be
> a temporary API (nothing more persistent than temporary though).

I think that this API is quite powerful and will persist for a long time.
The addHitRegion API is very cumbersome because it's complex and it requires that the author keeps another DOM up to date while he's changing the canvas.

Comment 31

[Rik Cabanier]

Roc, I could look into changing the code if someone can point me in the right direction.
However, that should probably happen in a different bug.

Can we submit this bug for now in case that end up being a dead end?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Rik Cabanier from comment #29)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> > If we want drawCustomFocusRing to change what accessibility returns, mashing
> > the element's frame rect here is not the way to do it. A much better
> > approach would be to attach a copy of the path to the frame as a frame
> > property, and have the accessibility code check for that property and use it
> > instead of the frame geometry when present. We can store the path in device
> > pixels relative to the canvas and then accessibility can find the relevant
> > canvas frame and do the necessary coordinate conversions itself.
> 
> How would that be different or better from the current solution?

The current solution calls SetRect on the frames of arbitrary elements. It violates a lot of layout invariants, such as (with a few very special exceptions) frame rects are only modified during Reflow. It does not update overflow areas on ancestor elements so breaks various invariants involving overflow areas. The next time a reflow happens, it could easily wipe out the changes you're making here (and a reflow could happen almost anytime for lots of reasons).

> afaik the a11y code just wants to know the bounds, it is not set up to deal with paths.

That's fine, it can compute the bounds of the path.

> Also, the path should not be associated with the canvas, it is supposed to
> be associated with the fallback/shadow content that lives inside the canvas
> element.

That's fine too; I suggested attaching the patch to the frame, which is associated with the fallback content.

(In reply to Rik Cabanier from comment #31)
> Roc, I could look into changing the code if someone can point me in the
> right direction.
> However, that should probably happen in a different bug.
> 
> Can we submit this bug for now in case that end up being a dead end?

Now that I've discovered it, the existing SetRect code is so bad it must be removed immediately, before we do anything else. If you prefer, I'll do it myself.

Comment 33

[Rik Cabanier]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Now that I've discovered it, the existing SetRect code is so bad it must be
> removed immediately, before we do anything else. If you prefer, I'll do it
> myself.

OK. I will remove it.
I will open bugs for removal and properly exposing the bounding rect to a11y.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks very much.