977170 - can't query IAccessible from HTML area element having ISimpleDOMNode pointer

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Was reported today this bug.

Jonathan, it's another side of the problem you approached recently in HTML area focus bug. Can you refresh pls why you went with local fix rather than DocAccessible::GetAccessible patch?

Comment 1

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #0)
> Was reported today this bug.
> 
> Jonathan, it's another side of the problem you approached recently in HTML
> area focus bug. Can you refresh pls why you went with local fix rather than
> DocAccessible::GetAccessible patch?

If I remember correctly, part of the reason was due to unwanted UpdateTree and TreeWalker behaviour (bug 961696 comment 4 and bug 961696 comment 6), which are used elsewhere.  The localised fix put into FocusManager was so area element accessibles would be focusable without affecting any other logic relying on those child accessibles being left out of the cache.

Comment 2

[alexander :surkov (:asurkov)]

Would it be a good idea to put localized fix in this case?

Comment 3

[Jonathan Wei [:jwei]]

I guess it depends how many potential issues might crop up in the future that depend on DocAccessible::GetAccessible returning the proper accessible when an accessible isn't cached.  While we still need TreeWalker to skip the uncached child accessibles, a localised fix makes sense.  However, applying all these localised fixes instead of modifying the central access point leads to a lot of code bloat.  Maybe we should get Trevor's opinion on this?

Comment 4

[Trevor Saunders (:tbsaunde)]

> Jonathan, it's another side of the problem you approached recently in HTML
> area focus bug. Can you refresh pls why you went with local fix rather than
> DocAccessible::GetAccessible patch?

So, you can't actually uniquely identify the accessible for a area node if the map is used for more than one image, that is what the focus manager does is broken if you have more than one image using a map.  So in the case of going from an accessible for an area to the ISimpleDOMNode you have a fundimentally lossy operation.  Pre ISimpleDOMNode being a tear off this worked because of what is more or less a bug, you could actually have more than one ISimpleDOMNode for the same DOMNode.  We could apply the same hack the focus manager uses here, but it seems pretty unexpected that acc->GetISimpleDOMNode()->GetAccessible() != acc which would happen if we did that and you incountered a map used by multiple images.

Comment 5

[alexander :surkov (:asurkov)]

perhaps it's the bug we can live with since we cannot support such AREA elements properly because of DOM focus restrictions.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

ugly but safe

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8383229 [details] [diff] [review]
patch

>+inline Accessible*
>+DocAccessible::GetAccessibleEvenIfNotInMapOrContainer(nsINode* aNode) const
>+{
>+  Accessible* acc = GetAccessibleEvenIfNotInMap(aNode);
>+  return acc ? acc : GetContainerAccessible(aNode);
>+}

I'm pretty sure this would be better out of line.  inlining it won't help the compiler know what the first call returns so it can't simplify the function at all, so so it will either inline it and just bloat code size or not inline but waste more time than if it was out of line.  Note it might actually be harmful because it might be better to inline the first function into it which will only happen if you out of line it.

Comment 8

[alexander :surkov (:asurkov)]

Trev, can you give me code snippet pls, I'm not sure I read you right.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> Trev, can you give me code snippet pls, I'm not sure I read you right.

for what?

Comment 10

[alexander :surkov (:asurkov)]

to demo your comment #7 of course

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> to demo your comment #7 of course

I still don't understand what you want to see.  Just move that function to DocAccessible.cpp

Comment 12

[alexander :surkov (:asurkov)]

ok, can you then pls rephrase your comment #7, why the extra function call is better than bloating size by two function calls.

Comment 13

[Trevor Saunders (:tbsaunde)]

I'm guessing its more useful to inline the functions that function calls into it than it is to inline it into its callers because if you do the former  the control flow and dupplicate function calls might actually allow you to do some useful optimizations.

Of course who really knows what compiler black magic will do without checking, but I don't see any good reason to define that function inline.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> I'm guessing its more useful to inline the functions that function calls
> into it than it is to inline it into its callers because if you do the
> former  the control flow and dupplicate function calls might actually allow
> you to do some useful optimizations.

I don't really follow it since not sure I can feel the difference between "inline the functions that function calls into it" and "inline the functions into its callers". Can you give me some examples pls?

> Of course who really knows what compiler black magic will do without
> checking, but I don't see any good reason to define that function inline.

It seems there's a number of inline methods in code that follows the pattern I used. I'm curious to know Neil opinion.

Comment 15

[[e-mail]]

Offhand I know of three potential ways in which inlining can benefit you.

If A is the sole call of B, then the call is simply extra overhead. However the code complexity of A and B may make the inlining inappropriate particularly on x86 where the number of registers is limited.

If, as here, B is very small, then even if there are many calls to B, the overhead of making the calls still exceeds the size of B.

If the calls to B often pass constant arguments, then inlining the call can result in strength reduction and other optimisations that may not normally be apparent. In this case B could be a larger function but there would still be some benefit from inlining it.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> > I'm guessing its more useful to inline the functions that function calls
> > into it than it is to inline it into its callers because if you do the
> > former  the control flow and dupplicate function calls might actually allow
> > you to do some useful optimizations.
> 
> I don't really follow it since not sure I can feel the difference between
> "inline the functions that function calls into it" and "inline the functions
> into its callers". Can you give me some examples pls?

ok, so lets have some functions
int
a()
{
 return 0;
}

int
b(int a)
{
  return a + 5;
}

int
c()
{
  return b(a());
}

int
d(int x)
{
  // bunch of fancy stuff
  x += c();
  // more fancy stuff
  return x;
}

now if I give you a choice either you you can inline c into d which gives you
int
d_and_c(int x)
{
  // fancy stuff
  x += b(a());
  /more fancy stuff
  return x;
}

or you can inline a and b into c to get
int
c_including_b_and_a()
{
  return 5;
}

the second option is better ;)

> > Of course who really knows what compiler black magic will do without
> > checking, but I don't see any good reason to define that function inline.
> 
> It seems there's a number of inline methods in code that follows the pattern
> I used. I'm curious to know Neil opinion.

knowing what is worth inlining is _really_ hard and best done with data about what actually matters so we're all sort of guessing :/

> If, as here, B is very small, then even if there are many calls to B, the
> overhead of making the calls still exceeds the size of B.

if your two choices are inline b or do no inlining I agree with you probably or atleast enough to make it the compilers problem.  However its not clear to me it wouldn't be more globaly optimal to copy and inline the functions b calls into it (though I'm less sure of that opinion than when I reviewed the patch)

> If the calls to B often pass constant arguments, then inlining the call can
> result in strength reduction and other optimisations that may not normally
> be apparent. In this case B could be a larger function but there would still
> be some benefit from inlining it.

this is the reason I want the compiler to inline the functions b calls into it, but its less clear to me now that it can actually get useful optimizations out of that.

Comment 17

[alexander :surkov (:asurkov)]

It seems like GetAccessibleEvenIfNotInMapOrContainer() from the patch is clearly Neil's case #2. Are there objections to not land the patch as is?

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> It seems like GetAccessibleEvenIfNotInMapOrContainer() from the patch is
> clearly Neil's case #2. Are there objections to not land the patch as is?

did you read my last comment? I'd still say make it out of line, but we're just guessing and wasting a lot of time doing it so do whatever you like.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #18)

> did you read my last comment?

yes, you said you're less sure of that opinion, so I wanted to double check

> I'd still say make it out of line, but we're
> just guessing and wasting a lot of time doing it so do whatever you like.

thanks

Comment 20

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a9e5e5c695a

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a9e5e5c695a