542039 - nsIFrame should return accessible type

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: wip

Now nsIFrame returns nsIAccessible object which results in unnecessary computations. In general it looks like

1) nsIAccessibilityService::GetAccessible calls nsIFrame::GetAccessible
2) nsIFrame::GetAccessible gets nsIAccessibilityService to create an accessible, calls methods like CreateHTML...
3) method CreateHTML... gets DOM node and pres shell from nsIFrame however they are obtained inside of nsIAccessibilityService::GetAccessible

We could keep all of this inside nsIAccessibilityService::GetAccessible if nsIFrame would return accessible type like nsIAccessibleProvider does. It should be more clean and quick.

We need to fix the bug 521730 because bullet frame creation requires bullet text. If bullet text can be accessed from a11y module then we'll be able fix this bug.

Comment 1

[alexander :surkov (:asurkov)]

Also this bug is necessary to switch to nsAccessible (instead of nsIAccessible) usage in nsIAccessibilityService

Comment 2

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

I'm not sure but what if we can get nsIContent to give us what we need (bug 534527 maybe). I was thinking it would be great if nsIContent could have a GetAccessible. For bullets I think Roc was willing to create an extra content node if we need it.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> I'm not sure but what if we can get nsIContent to give us what we need (bug
> 534527 maybe). I was thinking it would be great if nsIContent could have a
> GetAccessible. For bullets I think Roc was willing to create an extra content
> node if we need it.

It sounds like a bit different thing. If nsIContent could encapsulate nsIFrame's accessible creation logic then it will be great. And it will make this bug unnecessary but this bug's idea is we don't need to jump to layout from a11y and to a11y from layout and get back to ally and don't need unnecessary computations of DOM node and pres shell to create an accessible for frame.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> For bullets I think Roc was willing to create an extra content
> node if we need it.

This would be great. It might be part of bug 534527 as you said or bug 521730 (if we want to do smaller steps).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> I'm not sure but what if we can get nsIContent to give us what we need (bug
> 534527 maybe). I was thinking it would be great if nsIContent could have a
> GetAccessible. 

Ah, I see. This might make sense. But this would require very close relashionships between content/layout and a11y than we have now. Probably accessible types is something that is in the middle.

Comment 6

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

Yeah, nsIContent::GetAccessible is a longer range goal.

Also, I just realized you are talking about type here as a integer mapping

.(In reply to comment #0)
> Now nsIFrame returns nsIAccessible object which results in unnecessary
> computations. In general it looks like
> 
> 1) nsIAccessibilityService::GetAccessible calls nsIFrame::GetAccessible
> 2) nsIFrame::GetAccessible gets nsIAccessibilityService to create an
> accessible, calls methods like CreateHTML...
> 3) method CreateHTML... gets DOM node and pres shell from nsIFrame however they
> are obtained inside of nsIAccessibilityService::GetAccessible

How did it get this way?

Comment 7

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

Alexander, do you think we still need this bug?

Comment 8

[alexander :surkov (:asurkov)]

What would you suggest instead?

Comment 9

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

Actually I suggest finished the WIP; I like it so far :)  I think my comment 2 should be considered 'future'.

Comment 10

[alexander :surkov (:asurkov)]

Ok :) We need to put list bullet into content tree firstly.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #10)
> Ok :) We need to put list bullet into content tree firstly.

Not issue, anymore. Get the idea how to fix it from wip. It's plain and nice.

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: patch

I actually started working on this a little while ago to try and make GetOrCreateAccessible() faster, but just rebased this.

try: https://tbpl.mozilla.org/?tree=Try&rev=3ad54e8f5e78

Comment 13

[Trevor Saunders (:tbsaunde)]

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

looks like that doesn't quiet work on windows :(

I can see two reasonable fixes
1 forward declare struct HWND (I expect that works, but don't actually know)
2 make nsObjectFrame it self a QueryFrame target, and change back to using nsObjectFrame in nsAccessibilityService

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 666248 [details] [diff] [review]
> patch
> 
> looks like that doesn't quiet work on windows :(
> 
> I can see two reasonable fixes
> 1 forward declare struct HWND (I expect that works, but don't actually know)
> 2 make nsObjectFrame it self a QueryFrame target, and change back to using
> nsObjectFrame in nsAccessibilityService

I think I'd go for 2, but could do either.

Comment 15

[alexander :surkov (:asurkov)]

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

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

a bunch of comments

::: accessible/src/base/Makefile.in
@@ +57,5 @@
>  EXPORTS_NAMESPACES = mozilla/a11y
>  
>  EXPORTS_mozilla/a11y = \
>    FocusManager.h \
> +	ObjectTypes.h \

nit: tabs instead spaces?

::: accessible/src/base/ObjectTypes.h
@@ +12,5 @@
> +
> +/**
> + * Various types of accessible object we can be asked to create.
> + */
> +enum ObjectType {

I think I'd call them AccType, it's less generic than ObjectType

@@ +31,5 @@
> +  eHTMLRadioButtonAccessible,
> +  eHTMLTableAccessible,
> +    eHTMLTableCellAccessible,
> +    eHTMLTableRowAccessible,
> +    eHTMLTextFieldAccessible,

nit: bad offset for these three

@@ +37,5 @@
> +  eImageAccessible,
> +  eMediaAccessible,
> +  eObjectFrameAccessible,
> +  eOuterDocAccessible,
> +    eTextLeafAccessible

bad offset

::: layout/forms/nsFileControlFrame.cpp
@@ -639,2 @@
>  {
> -  // Accessible object exists just to hold onto its children, for later shutdown

it makes sense to keep this comment

::: layout/generic/nsBlockFrame.cpp
@@ +6380,1 @@
>    if (!HasBullet() || !presContext) {

nit: no local variable for pres context is needed

::: layout/generic/nsHTMLCanvasFrame.cpp
@@ +351,2 @@
>  {
> +  return a11y::eCanvasAccessible;

html prefix was lost

::: layout/generic/nsInlineFrame.cpp
@@ +907,5 @@
>    nsContainerFrame::DestroyFrom(aDestructRoot);
>  }
>  
>  #ifdef ACCESSIBILITY
> +  a11y::ObjectType

nit: wrong indent

@@ +916,5 @@
>    nsIAtom *tagAtom = mContent->Tag();
> +  if (tagAtom == nsGkAtoms::input)  // Broken <input type=image ... />
> +    return a11y::eHTMLButtonAccessible;
> +  else if (tagAtom == nsGkAtoms::img)  // Create accessible for broken <img>
> +      return a11y::eImageAccessible;

nit: bad offset

@@ +918,5 @@
> +    return a11y::eHTMLButtonAccessible;
> +  else if (tagAtom == nsGkAtoms::img)  // Create accessible for broken <img>
> +      return a11y::eImageAccessible;
> +  else if (tagAtom == nsGkAtoms::label)  // Creat accessible for <label>
> +      return a11y::eHTMLLabelAccessible;

nit: bad offset

::: layout/generic/nsObjectFrame.cpp
@@ +272,2 @@
>  {
> +  return a11y::eObjectFrameAccessible;

same as media question: html prefix was lost

::: layout/generic/nsTextFrameThebes.cpp
@@ +3900,1 @@
>      }

it seems we have doubled GetRenderedText check (see GetOrCreateAccessible, file a bug?)

::: layout/generic/nsVideoFrame.cpp
@@ +425,2 @@
>  {
> +  return a11y::eMediaAccessible;

media lost html prefix, reason?

::: layout/tables/nsTableCellFrame.h
@@ +54,5 @@
>                    nsIFrame*        aParent,
>                    nsIFrame*        aPrevInFlow);
>  
>  #ifdef ACCESSIBILITY
> +  virtual mozilla::a11y::ObjectType AccessibleType();

nit: MOZ_OVERRIDE? (here and other places)

::: layout/tables/nsTableOuterFrame.cpp
@@ +119,3 @@
>  {
>    if (!GetRect().IsEmpty()) {
> +    return a11y::eHTMLCaptionAccessible;

nit: having an opposite condition is nicer

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> > Comment on attachment 666248 [details] [diff] [review]
> > patch
> > 
> > looks like that doesn't quiet work on windows :(
> > 
> > I can see two reasonable fixes
> > 1 forward declare struct HWND (I expect that works, but don't actually know)
> > 2 make nsObjectFrame it self a QueryFrame target, and change back to using
> > nsObjectFrame in nsAccessibilityService
> 
> I think I'd go for 2, but could do either.

I think you can add QueryFrame for nObjectFrame, I did that in the past. You might need to ask Robert explicitly (I guess he missed your question).

Trev, it'd be nice if you finish this asap. I might need this bug to reorg accessible object creation (i.e. frame should never decide what accessible object is created for it but it should give a hint).

Comment 17

[Trevor Saunders (:tbsaunde)]

> ::: layout/forms/nsFileControlFrame.cpp
> @@ -639,2 @@
> >  {
> > -  // Accessible object exists just to hold onto its children, for later shutdown
> 
> it makes sense to keep this comment

from that bit of context it doesn't look like its true we use the input accessible to fire events on children and another thing or two now iirc.

> ::: layout/generic/nsObjectFrame.cpp
> @@ +272,2 @@
> >  {
> > +  return a11y::eObjectFrameAccessible;
> 
> same as media question: html prefix was lost

didn't really seem all that needed, and its shorter, but I changed back.

> ::: layout/generic/nsTextFrameThebes.cpp
> @@ +3900,1 @@
> >      }
> 
> it seems we have doubled GetRenderedText check (see GetOrCreateAccessible,
> file a bug?)

sure

Comment 18

[Trevor Saunders (:tbsaunde)]

Attachment: patch 2

roc for the layout bits which are pretty mechanical
surkov for the a11y stuff

Comment 19

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

Comment on attachment 669794 [details] [diff] [review]
patch 2

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

::: layout/generic/nsInlineFrame.cpp
@@ +918,5 @@
> +    return a11y::eHTMLButtonAccessible;
> +  else if (tagAtom == nsGkAtoms::img)  // Create accessible for broken <img>
> +    return a11y::eImageAccessible;
> +  else if (tagAtom == nsGkAtoms::label)  // Creat accessible for <label>
> +    return a11y::eHTMLLabelAccessible;

Remove "else"s here.

Comment 20

[alexander :surkov (:asurkov)]

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

> > > -  // Accessible object exists just to hold onto its children, for later shutdown
> > 
> > it makes sense to keep this comment
> 
> from that bit of context it doesn't look like its true we use the input
> accessible to fire events on children and another thing or two now iirc.

This object is really excess and serves for implementation purpose only. It's ok if we have a comment for it.

> > > +  return a11y::eObjectFrameAccessible;
> > 
> > same as media question: html prefix was lost
> 
> didn't really seem all that needed, and its shorter, but I changed back.

agree, just keeping the code consistent.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 669794 [details] [diff] [review]
patch 2

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

r=me with these fixed

::: accessible/src/base/AccTypes.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#pragma once

new style to replace guards? (some details please)

@@ +9,5 @@
> +namespace mozilla {
> +namespace a11y {
> +
> +/**
> + * Various types of accessible object we can be asked to create.

maybe: Accessible object types, used to create accessible objects by frame.

btw, one day it'd be nice to combine these with nsIAccessibleProvider constants and Accessible type constants.

@@ +25,5 @@
> +  eHTMLHRAccessible,
> +  eHTMLImageMapAccessible,
> +  eHTMLLabelAccessible,
> +  eHTMLLiAccessible,
> +  eHTMLListboxAccessible,

eHTMLSelectListAccessible to keep it closed to class names?

@@ +33,5 @@
> +  eHTMLTableAccessible,
> +  eHTMLTableCellAccessible,
> +  eHTMLTableRowAccessible,
> +  eHTMLTextFieldAccessible,
> +  eHypertextAccessible,

should be HyperText ('t' in uppercase) that's how the class is named

::: accessible/src/base/nsAccessibilityService.cpp
@@ +877,5 @@
>            return nullptr;
>          }
>  
>          // Try using frame to do it.
> +        newAcc = CreateAccessibleByFrameType(frame, frame->GetContent(), aDoc);

nit: use 'content' as well for consistence

@@ +1432,5 @@
> +                                                    nsIContent* aContent,
> +                                                    DocAccessible* aDoc)
> +{
> +  nsRefPtr<Accessible> newAcc;
> +  switch(aFrame->AccessibleType()) {

nit: space after switch

@@ +1434,5 @@
> +{
> +  nsRefPtr<Accessible> newAcc;
> +  switch(aFrame->AccessibleType()) {
> +    case eNoAccessible:
> +      return nullptr;

nit: do we really need it?

@@ +1478,5 @@
> +    case eHTMLMediaAccessible:
> +      newAcc = new EnumRoleAccessible(aContent, aDoc, roles::GROUPING);
> +      break;
> +    case eHTMLObjectFrameAccessible: {
> +                                   nsObjectFrame* objectFrame = do_QueryFrame(aFrame);

nit: a so wild indentation (two spaces relative 'case')

@@ +1481,5 @@
> +    case eHTMLObjectFrameAccessible: {
> +                                   nsObjectFrame* objectFrame = do_QueryFrame(aFrame);
> +      newAcc = CreateHTMLObjectFrameAccessible(objectFrame, aContent, aDoc);
> +      break;
> +                                 }

fix indent too

@@ +1502,5 @@
> +    case eHypertextAccessible:
> +      newAcc = new HyperTextAccessibleWrap(aContent, aDoc);
> +      break;
> +    case eImageAccessible:
> +      newAcc = new ImageAccessible(aContent, aDoc);

must be ImageAccessibleWrap

::: layout/forms/nsFileControlFrame.cpp
@@ -35,5 @@
>  #include "nsDisplayList.h"
>  #include "nsEventListenerManager.h"
> -#ifdef ACCESSIBILITY
> -#include "nsAccessibilityService.h"
> -#endif

it seems each frame file contains that include but you removed only this one. It'd be nice to get rid them all too.

::: layout/forms/nsImageControlFrame.cpp
@@ +133,4 @@
>  {
> +  if (mContent->Tag() == nsGkAtoms::button ||
> +      mContent->Tag() == nsGkAtoms::input) {
> +      return a11y::eHTMLButtonAccessible;

nit: indent it please properly (two spaces relative if)

::: layout/generic/nsInlineFrame.cpp
@@ -916,5 @@
>    // Broken image accessibles are created here, because layout
>    // replaces the image or image control frame with an inline frame
>    nsIAtom *tagAtom = mContent->Tag();
> -  if ((tagAtom == nsGkAtoms::img || tagAtom == nsGkAtoms::input || 
> -       tagAtom == nsGkAtoms::label) && mContent->IsHTML()) {

IsHTML() check was lost, are we guaranteed that nsInlineFrame is used for these tag names in HTML only?

::: layout/generic/nsObjectFrame.h
@@ +47,5 @@
>  
>    friend nsIFrame* NS_NewObjectFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
>  
>    NS_DECL_QUERYFRAME
> +    NS_DECL_QUERYFRAME_TARGET(nsObjectFrame)

nit: indent it properly

::: layout/generic/nsSubDocumentFrame.cpp
@@ +85,5 @@
>  {
>  }
>  
>  #ifdef ACCESSIBILITY
> +  a11y::AccType

nit: spaces at beginning new line

Comment 22

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/base/AccTypes.h
> @@ +3,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#pragma once
> 
> new style to replace guards? (some details please)

yes, just replaces gaurds but shorter and you don't have to remember what the define is supposed to be.

> @@ +9,5 @@
> > +namespace mozilla {
> > +namespace a11y {
> > +
> > +/**
> > + * Various types of accessible object we can be asked to create.
> 
> maybe: Accessible object types, used to create accessible objects by frame.
> 
> btw, one day it'd be nice to combine these with nsIAccessibleProvider
> constants and Accessible type constants.

not sure what you mean accessible type, but merging with nsIAccessibleProvider stuff seems nice if we can avoid those anoying long xpcomy names

> @@ +25,5 @@
> > +  eHTMLHRAccessible,
> > +  eHTMLImageMapAccessible,
> > +  eHTMLLabelAccessible,
> > +  eHTMLLiAccessible,
> > +  eHTMLListboxAccessible,
> 
> eHTMLSelectListAccessible to keep it closed to class names?

sure *shrug*

> @@ +1434,5 @@
> > +{
> > +  nsRefPtr<Accessible> newAcc;
> > +  switch(aFrame->AccessibleType()) {
> > +    case eNoAccessible:
> > +      return nullptr;
> 
> nit: do we really need it?

seems the easiest way to shut up the warning about not handling all enum cases

> ::: layout/forms/nsFileControlFrame.cpp
> @@ -35,5 @@
> >  #include "nsDisplayList.h"
> >  #include "nsEventListenerManager.h"
> > -#ifdef ACCESSIBILITY
> > -#include "nsAccessibilityService.h"
> > -#endif
> 
> it seems each frame file contains that include but you removed only this
> one. It'd be nice to get rid them all too.

we cann't get rid of all of them because some files also do other things with the service like UpdateText() RecreateAccessible() etc, but seems I missed a few that don't have any of that.

> ::: layout/generic/nsInlineFrame.cpp
> @@ -916,5 @@
> >    // Broken image accessibles are created here, because layout
> >    // replaces the image or image control frame with an inline frame
> >    nsIAtom *tagAtom = mContent->Tag();
> > -  if ((tagAtom == nsGkAtoms::img || tagAtom == nsGkAtoms::input || 
> > -       tagAtom == nsGkAtoms::label) && mContent->IsHTML()) {
> 
> IsHTML() check was lost, are we guaranteed that nsInlineFrame is used for
> these tag names in HTML only?

currently we only ever call this method when the content we got the frame for is html so it would be redundant, but if you plan to start calling this method in xul or something in the near future adding it back won't really hurt anything.

Comment 23

[alexander :surkov (:asurkov)]

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

> > new style to replace guards? (some details please)
> 
> yes, just replaces gaurds but shorter and you don't have to remember what
> the define is supposed to be.

i can see benefits, i meant whether this new mozilla code style or not

> > btw, one day it'd be nice to combine these with nsIAccessibleProvider
> > constants and Accessible type constants.
> 
> not sure what you mean accessible type, but merging with
> nsIAccessibleProvider stuff seems nice if we can avoid those anoying long
> xpcomy names

I meant Accessible::AccessibleTypes since they 90% are class names. Just thinking aloud.

> > it seems each frame file contains that include but you removed only this
> > one. It'd be nice to get rid them all too.
> 
> we cann't get rid of all of them because some files also do other things
> with the service like UpdateText() RecreateAccessible() etc, but seems I
> missed a few that don't have any of that.

sure, but I'd guess not few but most.

> > IsHTML() check was lost, are we guaranteed that nsInlineFrame is used for
> > these tag names in HTML only?
> 
> currently we only ever call this method when the content we got the frame
> for is html so it would be redundant, but if you plan to start calling this
> method in xul or something in the near future adding it back won't really
> hurt anything.

ok

Comment 24

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #23)
> (In reply to Trevor Saunders (:tbsaunde) from comment #22)
> 
> > > new style to replace guards? (some details please)
> > 
> > yes, just replaces gaurds but shorter and you don't have to remember what
> > the define is supposed to be.
> 
> i can see benefits, i meant whether this new mozilla code style or not

well, I'm certainly not the first person to be using it.

Comment 25

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e24562328a9

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 669794 [details] [diff] [review]
patch 2

>--- a/layout/forms/nsFileControlFrame.cpp
>+++ b/layout/forms/nsFileControlFrame.cpp
> 
>+namespace a11y = mozilla::a11y;
> namespace dom = mozilla::dom;

This causes this build error in --disable-accessibility builds:
 nsFileControlFrame.cpp:54:27: error: ‘a11y’ is not a namespace-name

After chatting with tbsaunde, I pushed a followup to fix this by swapping in "using namespace mozilla;" for both of the quoted lines above:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/e122361ed6de

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e24562328a9
https://hg.mozilla.org/mozilla-central/rev/e122361ed6de