[Mac] Too much is exposed to VoiceOver that is actually not currently visible/interactable

VERIFIED FIXED in mozilla16

Status

()

Core
Disability Access APIs
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: hub)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

5 years ago
VoiceOver sees contents that is actually currently not visible on the screen. Those elements should not be navigable with VoiceOver when they are not intractable with.

Example 1:
1. Launch Firefox nightly on the Mac.
2. Open two tabs and load any pages into them.
3. Switch to the second tab.
4. Find the HTML content for this second tab.

Expected: There should be only one HTML content.
Actual: There are two HTML contents intractable with, both showing the documents for the respective tabs. In Safari, when two tabs are open, only the contents of the foreground tab can be interacted with.

This has all sorts of implications: If one tries to navigate to the first heading within the second tab, by using Ctrl+Option+Cmd+H, the first heading in the *first* tab is found instead. This is repeatable with any element search.

Example 2:
1. Launch Firefox on the Mac.
2. Open Preferences.

Expected: Only the contents of the "General" tab should be visible to VoiceOver.
Actual: All tabs and pages are visible to VoiceOver, making it impossible for the user to know what he is currently really interacting with. Moreover, the fact that the user gets the whole blurp of stuff exposed means a huge amount of controls to try and navigate.
(Reporter)

Updated

5 years ago
Blocks: 342989
(Reporter)

Comment 1

5 years ago
Additional thoughts: I think these elements are exposed to VoiceOver in other applications, too, they just have some form of attribute that tells VoiceOver to ignore them for the time being until they become visible. Would be worth looking at attributes exposed by Safari for background tab content etc.
(Assignee)

Comment 2

5 years ago
One major thing is that we have all the AXWebArea that are exposed, while Safari always have the current one.
(Assignee)

Comment 3

5 years ago
Created attachment 620540 [details] [diff] [review]
Don't expose invisible items. r=
(Assignee)

Updated

5 years ago
Attachment #620540 - Flags: review?(surkov.alexander)

Comment 4

5 years ago
Comment on attachment 620540 [details] [diff] [review]
Don't expose invisible items. r=

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

::: accessible/src/mac/mozAccessible.mm
@@ +395,5 @@
>    // now iterate through the children array, and get each native accessible.
>    PRUint32 totalCount = childrenArray.Length();
>    for (PRUint32 idx = 0; idx < totalCount; idx++) {
>      nsAccessible* curAccessible = childrenArray.ElementAt(idx);
> +    if (curAccessible && !(curAccessible->NativeState() & states::INVISIBLE)) {

I don't see how it helps to example1. It should help to example2 for a while until bug 722417 is fixed
(Reporter)

Comment 5

5 years ago
Comment on attachment 620540 [details] [diff] [review]
Don't expose invisible items. r=

I built with this patch locally, and it does not show the desired effect.
1. When opening a second tab, the first tab's content disappears, but switching back to the first tab does not bring it back.
2. In Preferences, one is never able to switch away from the first tab. The others are hidden initially, but remain hidden even when switching dialog pages.
Attachment #620540 - Flags: feedback-

Comment 6

5 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #5)
> Comment on attachment 620540 [details] [diff] [review]
> 2. In Preferences, one is never able to switch away from the first tab. The
> others are hidden initially, but remain hidden even when switching dialog
> pages.

of course because the children cache is not invalidated when invisible state is changed

Updated

5 years ago
Attachment #620540 - Flags: review?(surkov.alexander)
(Assignee)

Comment 7

5 years ago
Yeah I'm missing the update part.
(Assignee)

Comment 8

5 years ago
Created attachment 622534 [details] [diff] [review]
Don't expose invisible items. r=
(Assignee)

Updated

5 years ago
Attachment #620540 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Comment on attachment 622534 [details] [diff] [review]
Don't expose invisible items. r=

If we have a XULTabpanels, make sure we get up to date children.

Since I can't get event for changes in the INVISIBLE state, this seems to be the easiest solution, to discard the children cache.

A small optimisation is missing is to memorize and check what the selectedPanel is.
Attachment #622534 - Flags: feedback?(surkov.alexander)
Attachment #622534 - Flags: feedback?(marco.zehe)

Comment 10

5 years ago
Comment on attachment 622534 [details] [diff] [review]
Don't expose invisible items. r=

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

logic is not consistent, invisible states presence is not equivalent to hosting an accessible in not current tab tab

btw, could you please address comment #4?
Attachment #622534 - Flags: feedback?(surkov.alexander) → feedback-
(Assignee)

Comment 11

5 years ago
Created attachment 624231 [details] [diff] [review]
Don't expose invisible property pages in a deck frame. r=
(Assignee)

Updated

5 years ago
Attachment #622534 - Attachment is obsolete: true
Attachment #622534 - Flags: feedback?(marco.zehe)
(Assignee)

Comment 12

5 years ago
Comment on attachment 624231 [details] [diff] [review]
Don't expose invisible property pages in a deck frame. r=

This should address comment #4 as well.
Attachment #624231 - Flags: feedback?(surkov.alexander)

Comment 13

5 years ago
Comment on attachment 624231 [details] [diff] [review]
Don't expose invisible property pages in a deck frame. r=

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

::: accessible/src/mac/mozAccessible.h
@@ +61,5 @@
> +{
> +  mozAccessible *native = nil;
> +  anAccessible->GetNativeInterface ((void**)&native);
> +  return native;
> +}

please correct style of the whole method

::: accessible/src/mac/mozActionElements.h
@@ +62,5 @@
>  }
>  -(id)tabs;
>  @end
> +
> +/* Accessible for a PANE (ie the container of the tab panels */

can you use the style please
/**
 * Comment.
 */

::: accessible/src/mac/mozActionElements.mm
@@ +42,5 @@
>  #include "Accessible-inl.h"
>  #include "nsXULTabAccessible.h"
>  
> +#include "nsDeckFrame.h"
> +#include "nsIFrame.h"

nsDeckFrame should include nsIFrame

@@ +373,5 @@
> +
> +- (NSArray*)children
> +{
> +  // TODO only if the visible pane has changed.
> +  // 

you don't need // on new line and mark it as XXX rather than TODO

@@ +384,5 @@
> +  nsAutoTArray<nsAccessible*, 10> childrenArray;
> +  mGeckoAccessible->GetUnignoredChildren(&childrenArray);
> +
> +  nsIFrame* deckFrame = mGeckoAccessible->GetContent()->GetPrimaryFrame();
> +  nsDeckFrame* deck = do_QueryFrame(deckFrame);

deck name means deck accessible, use deckFrame instead, and either get rid that deckFrame above or rename it

@@ +391,5 @@
> +  // now iterate through the children array, and get each native accessible.
> +  PRUint32 totalCount = childrenArray.Length();
> +  for (PRUint32 idx = 0; idx < totalCount; idx++) {
> +    nsAccessible* curAccessible = childrenArray.ElementAt(idx);
> +    if (curAccessible &&  curAccessible->GetContent()->GetPrimaryFrame() == selectedFrame) {

get an accessible for selectedFrame instead of array traversal?

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +115,5 @@
>      case roles::PAGETABLIST:
>        return [mozTabsAccessible class];
> +
> +    case roles::PANE:
> +      return [mozTabPaneAccessible class];

basically this is wrong, if that's something else than tab pane then mozTabPaneAccessible::children does unwanted things. If you are class dependent then use downcasting approach instead
Attachment #624231 - Flags: feedback?(surkov.alexander) → feedback-

Comment 14

5 years ago
(In reply to Hub Figuiere [:hub] from comment #12)
> Comment on attachment 624231 [details] [diff] [review]
> Don't expose invisible property pages in a deck frame. r=
> 
> This should address comment #4 as well.

btw, it doesn't seem to address example #2
Hub does Safari simply return null for children of inactive tab documents?
(Assignee)

Comment 16

5 years ago
(In reply to David Bolter [:davidb] from comment #15)
> Hub does Safari simply return null for children of inactive tab documents?

It does not return the children of the non visible tabs - which is indeed what we expect. However, and that is already working, it does return the number of tabs button to switch amongst tabs.
(Assignee)

Comment 17

5 years ago
Created attachment 626578 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
(Assignee)

Updated

5 years ago
Attachment #624231 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Comment on attachment 626578 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

Comment 14 is addressed. 

I haven't run mochitest or try build as Try is closed
Attachment #626578 - Flags: feedback?(surkov.alexander)
(Assignee)

Updated

5 years ago
Assignee: nobody → hub
(Assignee)

Comment 19

5 years ago
> ::: accessible/src/mac/nsAccessibleWrap.mm
> @@ +115,5 @@
> >      case roles::PAGETABLIST:
> >        return [mozTabsAccessible class];
> > +
> > +    case roles::PANE:
> > +      return [mozTabPaneAccessible class];
> 
> basically this is wrong, if that's something else than tab pane then
> mozTabPaneAccessible::children does unwanted things. If you are class
> dependent then use downcasting approach instead

How is it wrong? All the instance of PANE I have seen are like "Deck".
I renamed the class to not mention tabs at all though.
(Assignee)

Comment 20

5 years ago
Also this might crash in debug, but this is unrelated to this. I have a fix pending for the crash too.
(Assignee)

Updated

5 years ago
Depends on: 758304

Comment 21

5 years ago
(In reply to Hub Figuiere [:hub] from comment #19)

> > > +    case roles::PANE:
> > > +      return [mozTabPaneAccessible class];
> > 
> > basically this is wrong, if that's something else than tab pane then
> > mozTabPaneAccessible::children does unwanted things. If you are class
> > dependent then use downcasting approach instead
> 
> How is it wrong? All the instance of PANE I have seen are like "Deck".
> I renamed the class to not mention tabs at all though.

yes, but document accessible for example can expose PANE role as well. That means PANE is not necessary tabPanel.

Comment 22

5 years ago
Comment on attachment 626578 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

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

I don't see anything bad in accessible creation for xul deck. So to keep the code nice:

1) rename XULTabpanelsAccessible to XULDeckAccessible
2) override nsIFrame::CreateAccessible on deck frame to return XULDeckAccessible
3) remove nsIAccessibleProvider implementation on tabpanels binding (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml#596). Don't forget to fix nsIAccessibleProvider.
4) instead of role PANE check please check the accessible type (add nsAccessible::IsDeck())

(I didn't check (NSArray*)children code)
Attachment #626578 - Flags: feedback?(surkov.alexander)

Comment 23

5 years ago
(In reply to alexander :surkov from comment #22)

> 1) rename XULTabpanelsAccessible to XULDeckAccessible

actually you can go with nsEnumRoleAccessible
(Assignee)

Comment 24

5 years ago
Created attachment 628497 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
(Assignee)

Updated

5 years ago
Attachment #626578 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #628497 - Flags: review?(surkov.alexander)

Comment 25

5 years ago
Comment on attachment 628497 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

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

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1200,5 @@
> +    // xul:deck does not have XBL.
> +    nsIAtom* tag = content->Tag();
> +    if ((tag == nsGkAtoms::deck) || (tag == nsGkAtoms::tabpanels)) {
> +      newAcc = new XULDeckAccessible(content, docAcc);
> +    }

why don't you follow 2nd suggestion:
> 2) override nsIFrame::CreateAccessible on deck frame to return XULDeckAccessible

::: accessible/src/generic/Accessible.h
@@ +725,5 @@
>      eMenuPopupAccessible = 1 << 15,
>      eRootAccessible = 1 << 16,
>      eTextLeafAccessible = 1 << 17,
> +    eXULTreeAccessible = 1 << 18,
> +    eXULDeckAccessible = 1 << 19

they are in alphabet order

::: accessible/src/mac/mozActionElements.mm
@@ +338,5 @@
> +@implementation mozPaneAccessible
> +
> +- (NSArray*)children
> +{
> +  nsIFrame* primaryFrame = mGeckoAccessible->GetContent()->GetPrimaryFrame();

you can do mGeckoAcc->GetFrame()

@@ +342,5 @@
> +  nsIFrame* primaryFrame = mGeckoAccessible->GetContent()->GetPrimaryFrame();
> +  nsDeckFrame* deckFrame = do_QueryFrame(primaryFrame);
> +  nsIFrame* selectedFrame = deckFrame ? deckFrame->GetSelectedBox() : nsnull;
> +
> +  if((mSelectedFrame == selectedFrame) && mChildren && 

space after 'if', space in the end of line (could configure your editor to avoid that?)

@@ +343,5 @@
> +  nsDeckFrame* deckFrame = do_QueryFrame(primaryFrame);
> +  nsIFrame* selectedFrame = deckFrame ? deckFrame->GetSelectedBox() : nsnull;
> +
> +  if((mSelectedFrame == selectedFrame) && mChildren && 
> +     mGeckoAccessible->AreChildrenCached())

how does it happen? do you compute children during tree construction?

@@ +357,5 @@
> +  // get the array of children.
> +  nsAutoTArray<Accessible*, 10> childrenArray;
> +  mGeckoAccessible->GetUnignoredChildren(&childrenArray);
> +
> +  // now iterate through the children array, and get each native accessible.

// now iterate -> // Iterate
comma is not needed?

@@ +366,5 @@
> +      mozAccessible *curNative = GetNativeFromGeckoAccessible(curAccessible);
> +      if (curNative)
> +        [mChildren addObject:GetObjectOrRepresentedView(curNative)];
> +    }
> +  }

why wouldn't you simply call

if (selectedAcc->IsIgnored())
  return GetNativeObjct(selectedAcc)->children();

[mChildren addObject:GetObjOrRepView(GetNativeObj(selectAcc))]

::: accessible/src/xul/nsXULTabAccessible.h
@@ +58,2 @@
>   */
> +class XULDeckAccessible : public AccessibleWrap

please don't keep it inside nsXULTabAccessible (maybe nsXULTextAccessible is a proper place, see bug 759310).
Attachment #628497 - Flags: review?(surkov.alexander)
(Assignee)

Comment 26

5 years ago
Comment on attachment 628497 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

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

::: accessible/src/mac/mozActionElements.mm
@@ +343,5 @@
> +  nsDeckFrame* deckFrame = do_QueryFrame(primaryFrame);
> +  nsIFrame* selectedFrame = deckFrame ? deckFrame->GetSelectedBox() : nsnull;
> +
> +  if((mSelectedFrame == selectedFrame) && mChildren && 
> +     mGeckoAccessible->AreChildrenCached())

There is no "tree construction" per see. The AX client (Voice Over) request it.

This is the same code as in mozAccessible with the exception of dealing with the selectedFrame, ie discarding the cache if the selected frame changed.

@@ +366,5 @@
> +      mozAccessible *curNative = GetNativeFromGeckoAccessible(curAccessible);
> +      if (curNative)
> +        [mChildren addObject:GetObjectOrRepresentedView(curNative)];
> +    }
> +  }

I just do like in mozAccessible, except that I also check for the selected frame.
I could indeed exit the loop when I found it as there can only be one (to rule them all)

Comment 27

5 years ago
(In reply to Hub Figuiere [:hub] from comment #26)
> > +     mGeckoAccessible->AreChildrenCached())
> 
> There is no "tree construction" per see. The AX client (Voice Over) request
> it.

AX client doesn't control Gecko children creation

> This is the same code as in mozAccessible with the exception of dealing with
> the selectedFrame, ie discarding the cache if the selected frame changed.

I'm not sure why this code is in mozAccessible. Is it rudiment perhaps?

> @@ +366,5 @@
> > +      mozAccessible *curNative = GetNativeFromGeckoAccessible(curAccessible);
> > +      if (curNative)
> > +        [mChildren addObject:GetObjectOrRepresentedView(curNative)];
> > +    }
> > +  }
> 
> I just do like in mozAccessible, except that I also check for the selected
> frame.

Right, but why do you need to do that, can't you do as I suggested?
(Assignee)

Comment 28

5 years ago
(In reply to alexander :surkov from comment #27)
> (In reply to Hub Figuiere [:hub] from comment #26)
> > > +     mGeckoAccessible->AreChildrenCached())
> > 
> > There is no "tree construction" per see. The AX client (Voice Over) request
> > it.
> 
> AX client doesn't control Gecko children creation

No it doesn't. I just make sure to not cache invalid info.

> Right, but why do you need to do that, can't you do as I suggested?

I did in my current patch. There was some confusion originally when I read your comment 25, but it is crystal clear now. My bad.

>::: accessible/src/xul/nsXULTabAccessible.h
>@@ +58,2 @@
>>   */
>> +class XULDeckAccessible : public AccessibleWrap

> please don't keep it inside nsXULTabAccessible (maybe nsXULTextAccessible is a 
> proper place, see bug 759310).

I don't see how nsXULTextAccessible would be a better place. Actually I don't see why this is a bad place.
(Assignee)

Comment 29

5 years ago
Created attachment 632099 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
(Assignee)

Updated

5 years ago
Attachment #628497 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #632099 - Flags: review?(surkov.alexander)
(Assignee)

Comment 30

5 years ago
I need to rebase the patch, again.
(Assignee)

Comment 31

5 years ago
Note that the whole nsXULTabAccessible has been renamed now :-)
(Assignee)

Comment 32

5 years ago
Created attachment 632113 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
(Assignee)

Updated

5 years ago
Attachment #632099 - Attachment is obsolete: true
Attachment #632099 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #632113 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #632113 - Flags: review?(surkov.alexander) → review?(dbolter)
Comment on attachment 632113 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

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

f=me. This looks really close! I understand you didn't get rid of the provider stuff because it didn't work out (?). And you don't use an enum accessible because presumably because you want to override getchildren. I think it prudent to keep Alexander as reviewer since I'm not sure about all his concerns.

(Alexander should be available for reviews as per his email to me - he'll just be less consistent/quick)
Attachment #632113 - Flags: review?(dbolter) → feedback+
(Assignee)

Updated

5 years ago
Attachment #632113 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #632113 - Flags: review?(surkov.alexander) → review?(trev.saunders)
(In reply to David Bolter [:davidb] from comment #33)
> Comment on attachment 632113 [details] [diff] [review]
> Don't expose invisible property pages in a deck frame.
> 
> Review of attachment 632113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f=me. This looks really close! I understand you didn't get rid of the
> provider stuff because it didn't work out (?). And you don't use an enum

I think we talked about this at some point, but we could we get into this bug why creating it by frame type with nsIFrame::CreateAccessible() doesn't work?

> accessible because presumably because you want to override getchildren. I

its to set the bit in mFlags.
Comment on attachment 632113 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

>+  /** Used for XUL deck frame */
>+  const long XULDeck = 0x00001019;

its weird that its used to designate the type of the widget is a tabpanels not something that has to do with a deck.

>+    nsIAtom* tag = content->Tag();
>+    if ((tag == nsGkAtoms::deck) || (tag == nsGkAtoms::tabpanels)) {
>+      newAcc = new XULDeckAccessible(content, docAcc);
>+    }
>     // Create generic accessibles for SVG and MathML nodes.
>-    if (content->IsSVG(nsGkAtoms::svg)) {
>+    else if (content->IsSVG(nsGkAtoms::svg)) {

putting a comment between the } and else is crazy.

>-    eXULTreeAccessible = 1 << 18
>+    eXULDeckAccessible = 1 << 18,
>+    eXULTreeAccessible = 1 << 19

nit, keep in order

>+@interface mozPaneAccessible : mozAccessible
>+{
>+  nsIFrame* mSelectedFrame;

that scares me a bit since while it doesn't ucrrently matter the frame might well go away before the accessible changes mSelectedFrames value.  I'd suggest saving it as a uintptr_t or void*.  Though actually I'm not convinced this is even correct, since you could have a new frame replace the previous one.


>+
>+  if ((mSelectedFrame == selectedFrame) && mChildren &&
>+      mGeckoAccessible->AreChildrenCached())

I still don't see a reason you need AreChildrenCached()

>+  if (selectedFrame) {
>+    nsINode* node = selectedFrame->GetContent();
>+    DocAccessible* document = node ? GetAccService()->GetDocAccessible(node->OwnerDoc()) : nsnull;
>+    selectedAcc = document ? document->GetAccessible(node) : nsnull;

just do mAccessible->Document()->GetAccessible(node);

>+  if (selectedAcc) {
>+    mozAccessible *curNative = GetNativeFromGeckoAccessible(selectedAcc);
>+    if (curNative)
>+      [mChildren addObject:GetObjectOrRepresentedView(curNative)];

I don't think I see an anser to surkov's question about why this is needed.

>-XULTabpanelsAccessible::
>-  XULTabpanelsAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>+XULDeckAccessible::
>+  XULDeckAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   AccessibleWrap(aContent, aDoc)
> {
>+  mFlags |= eXULDeckAccessible;
> }

it probably makes sense to make this inline while your here.
(Assignee)

Comment 36

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #35)
> Comment on attachment 632113 [details] [diff] [review]
> Don't expose invisible property pages in a deck frame.
> 
> >+  /** Used for XUL deck frame */
> >+  const long XULDeck = 0x00001019;
> 
> its weird that its used to designate the type of the widget is a tabpanels
> not something that has to do with a deck.

A "tabpanels" is a deck.


> 
> >+    nsIAtom* tag = content->Tag();
> >+    if ((tag == nsGkAtoms::deck) || (tag == nsGkAtoms::tabpanels)) {
> >+      newAcc = new XULDeckAccessible(content, docAcc);
> >+    }
> >     // Create generic accessibles for SVG and MathML nodes.
> >-    if (content->IsSVG(nsGkAtoms::svg)) {
> >+    else if (content->IsSVG(nsGkAtoms::svg)) {
> 
> putting a comment between the } and else is crazy.

where else?

> >-    eXULTreeAccessible = 1 << 18
> >+    eXULDeckAccessible = 1 << 18,
> >+    eXULTreeAccessible = 1 << 19
> 
> nit, keep in order

It is. Deck comes before Tree. And the enum order is also

> >+@interface mozPaneAccessible : mozAccessible
> >+{
> >+  nsIFrame* mSelectedFrame;
> 
> that scares me a bit since while it doesn't ucrrently matter the frame might
> well go away before the accessible changes mSelectedFrames value.  I'd
> suggest saving it as a uintptr_t or void*.  Though actually I'm not
> convinced this is even correct, since you could have a new frame replace the
> previous one.

if it is new frame replacing this one, the pointer will be different. I only check the pointer value. I can make it void* if you really want.


> 
> 
> >+
> >+  if ((mSelectedFrame == selectedFrame) && mChildren &&
> >+      mGeckoAccessible->AreChildrenCached())
> 
> I still don't see a reason you need AreChildrenCached()


Because otherwise there is not reason I attempt to get the children since they haven't been created yet.


> 
> >+  if (selectedAcc) {
> >+    mozAccessible *curNative = GetNativeFromGeckoAccessible(selectedAcc);
> >+    if (curNative)
> >+      [mChildren addObject:GetObjectOrRepresentedView(curNative)];
> 
> I don't think I see an anser to surkov's question about why this is needed.

Well, I need the children for the accessible. Actually just one so I have to add it.

Comment 37

5 years ago
(In reply to David Bolter [:davidb] from comment #33)
> I
> think it prudent to keep Alexander as reviewer since I'm not sure about all
> his concerns.
> 
> (Alexander should be available for reviews as per his email to me - he'll
> just be less consistent/quick)

I'm fine if Trevor finishes it (I see he concerns the issues I concerned before).
(Assignee)

Comment 38

5 years ago
Created attachment 637339 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
(Assignee)

Updated

5 years ago
Attachment #632113 - Attachment is obsolete: true
Attachment #632113 - Flags: review?(trev.saunders)
(Assignee)

Comment 39

5 years ago
Comment on attachment 637339 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

Nits addressed.
Attachment #637339 - Flags: review?(trev.saunders)
(In reply to Hub Figuiere [:hub] from comment #36)
> (In reply to Trevor Saunders (:tbsaunde) from comment #35)
> > Comment on attachment 632113 [details] [diff] [review]
> > Don't expose invisible property pages in a deck frame.
> > 
> > >+  /** Used for XUL deck frame */
> > >+  const long XULDeck = 0x00001019;
> > 
> > its weird that its used to designate the type of the widget is a tabpanels
> > not something that has to do with a deck.
> 
> A "tabpanels" is a deck.

in the implementation sense yes, still seems slightly weird to me but whatever.

> > >+    nsIAtom* tag = content->Tag();
> > >+    if ((tag == nsGkAtoms::deck) || (tag == nsGkAtoms::tabpanels)) {
> > >+      newAcc = new XULDeckAccessible(content, docAcc);
> > >+    }
> > >     // Create generic accessibles for SVG and MathML nodes.
> > >-    if (content->IsSVG(nsGkAtoms::svg)) {
> > >+    else if (content->IsSVG(nsGkAtoms::svg)) {
> > 
> > putting a comment between the } and else is crazy.
> 
> where else?

probably in the else, or actually that comment is pretty useless just remove it.

> > >+@interface mozPaneAccessible : mozAccessible
> > >+{
> > >+  nsIFrame* mSelectedFrame;
> > 
> > that scares me a bit since while it doesn't ucrrently matter the frame might
> > well go away before the accessible changes mSelectedFrames value.  I'd
> > suggest saving it as a uintptr_t or void*.  Though actually I'm not
> > convinced this is even correct, since you could have a new frame replace the
> > previous one.
> 
> if it is new frame replacing this one, the pointer will be different. I only
> check the pointer value. I can make it void* if you really want.

no, it could be a new frame at that address if someone removes the selected one and causes a reflow and then adds one all before someone calls this method.

> > >+
> > >+  if ((mSelectedFrame == selectedFrame) && mChildren &&
> > >+      mGeckoAccessible->AreChildrenCached())
> > 
> > I still don't see a reason you need AreChildrenCached()
> 
> 
> Because otherwise there is not reason I attempt to get the children since
> they haven't been created yet.

but that method should *never* return false at a time other people can call into us.

> > >+  if (selectedAcc) {
> > >+    mozAccessible *curNative = GetNativeFromGeckoAccessible(selectedAcc);
> > >+    if (curNative)
> > >+      [mChildren addObject:GetObjectOrRepresentedView(curNative)];
> > 
> > I don't think I see an anser to surkov's question about why this is needed.
> 
> Well, I need the children for the accessible. Actually just one so I have to
> add it.

sure, but why the GetObjectOrRepresentedview() ?
(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> (In reply to David Bolter [:davidb] from comment #33)
> > Comment on attachment 632113 [details] [diff] [review]
> > Don't expose invisible property pages in a deck frame.
> > 
> > Review of attachment 632113 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > f=me. This looks really close! I understand you didn't get rid of the
> > provider stuff because it didn't work out (?). And you don't use an enum
> 
> I think we talked about this at some point, but we could we get into this
> bug why creating it by frame type with nsIFrame::CreateAccessible() doesn't

I also really want the answer to this documented somewhere.
(Assignee)

Comment 42

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #40)

> > > >+@interface mozPaneAccessible : mozAccessible
> > > >+{
> > > >+  nsIFrame* mSelectedFrame;
> > > 
> > > that scares me a bit since while it doesn't ucrrently matter the frame might
> > > well go away before the accessible changes mSelectedFrames value.  I'd
> > > suggest saving it as a uintptr_t or void*.  Though actually I'm not
> > > convinced this is even correct, since you could have a new frame replace the
> > > previous one.
> > 
> > if it is new frame replacing this one, the pointer will be different. I only
> > check the pointer value. I can make it void* if you really want.
> 
> no, it could be a new frame at that address if someone removes the selected
> one and causes a reflow and then adds one all before someone calls this
> method.

So what would be the alternative beside not checking that the selected frame stayed the same?
 
> > > >+
> > > >+  if ((mSelectedFrame == selectedFrame) && mChildren &&
> > > >+      mGeckoAccessible->AreChildrenCached())
> > > 
> > > I still don't see a reason you need AreChildrenCached()
> > 
> > 
> > Because otherwise there is not reason I attempt to get the children since
> > they haven't been created yet.
> 
> but that method should *never* return false at a time other people can call
> into us.

Yet is does. It was part of an early bug: we can get called before all the objects are instanciated.

> > Well, I need the children for the accessible. Actually just one so I have to
> > add it.
> 
> sure, but why the GetObjectOrRepresentedview() ?

Here is the comment from mozAccessible.h for that function.
/**
 * All mozAccessibles are either abstract objects (that correspond to XUL
 * widgets, HTML frames, etc) or are attached to a certain view; for example
 * a document view. When we hand an object off to an AT, we always want
 * to give it the represented view, in the latter case.
 */
(Assignee)

Comment 43

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #41)
> (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > (In reply to David Bolter [:davidb] from comment #33)
> > > Comment on attachment 632113 [details] [diff] [review]
> > > Don't expose invisible property pages in a deck frame.
> > > 
> > > Review of attachment 632113 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > f=me. This looks really close! I understand you didn't get rid of the
> > > provider stuff because it didn't work out (?). And you don't use an enum
> > 
> > I think we talked about this at some point, but we could we get into this
> > bug why creating it by frame type with nsIFrame::CreateAccessible() doesn't
> 
> I also really want the answer to this documented somewhere.

In nsAccessibilityService, it only calls nsIFrame::CreateAccessible() on HTML elements. Not on XUL like in that case.
(In reply to Hub Figuiere [:hub] from comment #43)
> (In reply to Trevor Saunders (:tbsaunde) from comment #41)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > > (In reply to David Bolter [:davidb] from comment #33)
> > > > Comment on attachment 632113 [details] [diff] [review]
> > > > Don't expose invisible property pages in a deck frame.
> > > > 
> > > > Review of attachment 632113 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > f=me. This looks really close! I understand you didn't get rid of the
> > > > provider stuff because it didn't work out (?). And you don't use an enum
> > > 
> > > I think we talked about this at some point, but we could we get into this
> > > bug why creating it by frame type with nsIFrame::CreateAccessible() doesn't
> > 
> > I also really want the answer to this documented somewhere.
> 
> In nsAccessibilityService, it only calls nsIFrame::CreateAccessible() on
> HTML elements. Not on XUL like in that case.

ok, I'm not sure what the effect on perf will be, but we might well want to change that.

(In reply to Hub Figuiere [:hub] from comment #42)
> (In reply to Trevor Saunders (:tbsaunde) from comment #40)
> 
> > > > >+@interface mozPaneAccessible : mozAccessible
> > > > >+{
> > > > >+  nsIFrame* mSelectedFrame;
> > > > 
> > > > that scares me a bit since while it doesn't ucrrently matter the frame might
> > > > well go away before the accessible changes mSelectedFrames value.  I'd
> > > > suggest saving it as a uintptr_t or void*.  Though actually I'm not
> > > > convinced this is even correct, since you could have a new frame replace the
> > > > previous one.
> > > 
> > > if it is new frame replacing this one, the pointer will be different. I only
> > > check the pointer value. I can make it void* if you really want.
> > 
> > no, it could be a new frame at that address if someone removes the selected
> > one and causes a reflow and then adds one all before someone calls this
> > method.
> 
> So what would be the alternative beside not checking that the selected frame
> stayed the same?

well, unless you have data showing its to slow to just get the selected one each time I'd do that.  I'd expect caching it isn't that useful, other than the objective C stuff there its just a hash table lookup and virtual call to get the node for the frame.

If we do need to cache somehow the easiest thing to do would be to use a nsWeakFrame, but that slows down layout so I'd rather not do it.

> > > > >+
> > > > >+  if ((mSelectedFrame == selectedFrame) && mChildren &&
> > > > >+      mGeckoAccessible->AreChildrenCached())
> > > > 
> > > > I still don't see a reason you need AreChildrenCached()
> > > 
> > > 
> > > Because otherwise there is not reason I attempt to get the children since
> > > they haven't been created yet.
> > 
> > but that method should *never* return false at a time other people can call
> > into us.
> 
> Yet is does. It was part of an early bug: we can get called before all the
> objects are instanciated.

that sounds kind of scary, any idea how?

> > > add it.
> > 
> > sure, but why the GetObjectOrRepresentedview() ?
> 
> Here is the comment from mozAccessible.h for that function.
> /**
>  * All mozAccessibles are either abstract objects (that correspond to XUL
>  * widgets, HTML frames, etc) or are attached to a certain view; for example
>  * a document view. When we hand an object off to an AT, we always want
>  * to give it the represented view, in the latter case.
>  */

ok, I forget how weird all that code to return mac objects is.  Is it a bug that we don't check this in mozAccessible.mm:208?
(Assignee)

Comment 45

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #44)

> well, unless you have data showing its to slow to just get the selected one
> each time I'd do that.  I'd expect caching it isn't that useful, other than
> the objective C stuff there its just a hash table lookup and virtual call to
> get the node for the frame.
> 
> If we do need to cache somehow the easiest thing to do would be to use a
> nsWeakFrame, but that slows down layout so I'd rather not do it.

Got rid of the caching there.


> > Yet is does. It was part of an early bug: we can get called before all the
> > objects are instanciated.
> 
> that sounds kind of scary, any idea how?

yep. Accessibles are created by the even processing, but then it is possible we call the Accessibility stuff from the the Mac AX client before they are done. Since we cache the children, we'd get an empty cache.

> > > > add it.
> > > 
> > > sure, but why the GetObjectOrRepresentedview() ?
> > 
> > Here is the comment from mozAccessible.h for that function.
> > /**
> >  * All mozAccessibles are either abstract objects (that correspond to XUL
> >  * widgets, HTML frames, etc) or are attached to a certain view; for example
> >  * a document view. When we hand an object off to an AT, we always want
> >  * to give it the represented view, in the latter case.
> >  */
> 
> ok, I forget how weird all that code to return mac objects is.  Is it a bug
> that we don't check this in mozAccessible.mm:208?

Possibly. I'd have to investigate.
(Assignee)

Comment 46

5 years ago
Created attachment 638820 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
(Assignee)

Updated

5 years ago
Attachment #637339 - Attachment is obsolete: true
Attachment #637339 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #638820 - Flags: review?(trev.saunders)
> > > Yet is does. It was part of an early bug: we can get called before all the
> > > objects are instanciated.
> > 
> > that sounds kind of scary, any idea how?
> 
> yep. Accessibles are created by the even processing, but then it is possible
> we call the Accessibility stuff from the the Mac AX client before they are
> done. Since we cache the children, we'd get an empty cache.

ok, I'm confused.  I know the platform objects may not exist, but AreChildrenCached() deals with internal ones.  The internal objects are not created lazily, and aren't created by events.  It seems like it should be force the creation of the platform object here if it doesn't already exist.
(Assignee)

Comment 48

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #47)

> ok, I'm confused.  I know the platform objects may not exist, but
> AreChildrenCached() deals with internal ones.  The internal objects are not
> created lazily, and aren't created by events.  It seems like it should be
> force the creation of the platform object here if it doesn't already exist.

That code is now gone from that patch anyway following your other suggestions.

But you can look at bug 689105 if you are curious.
Comment on attachment 638820 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

>+    // xul:deck does not have XBL.
>+    nsIAtom* tag = content->Tag();
>+    if ((tag == nsGkAtoms::deck) || (tag == nsGkAtoms::tabpanels)) {
>+      newAcc = new XULDeckAccessible(content, docAcc);

imo it would be more useful to say nsIFrame::CreateAccessible() is only called on html elements.

>+    }
>+    else if (content->IsSVG(nsGkAtoms::svg)) {

nit, } else if (blah) {

>+- (NSArray*)children
>+{
>+  nsDeckFrame* deckFrame = do_QueryFrame(mGeckoAccessible->GetFrame());

we require callers to check the accessible isn't defunct right?

>+  nsIFrame* selectedFrame = deckFrame ? deckFrame->GetSelectedBox() : nsnull;
>+
>+  Accessible* selectedAcc = nsnull;
>+  if (selectedFrame) {
>+    nsINode* node = selectedFrame->GetContent();
>+    selectedAcc = mGeckoAccessible->Document()->GetAccessible(node);
>+  }

we should have a follow up to move this to XULDeckAccessible::CurrentItem()

>-class XULTabpanelsAccessible : public AccessibleWrap
>+class XULDeckAccessible : public AccessibleWrap
> {
> public:
>-  XULTabpanelsAccessible(nsIContent* aContent, DocAccessible* aDoc);
>+  XULDeckAccessible(nsIContent* aContent, DocAccessible* aDoc)
>+    : AccessibleWrap(aContent, aDoc)
>+    { mFlags |= eXULDeckAccessible; }

usual style is : on first line.
Attachment #638820 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 50

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #49)
> >+- (NSArray*)children
> >+{
> >+  nsDeckFrame* deckFrame = do_QueryFrame(mGeckoAccessible->GetFrame());
> 
> we require callers to check the accessible isn't defunct right?

Good catch. Adding a check as it is not defunct checked.

Will address the rest.
(Assignee)

Comment 51

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca70d915a67

Comment 52

5 years ago
https://hg.mozilla.org/mozilla-central/rev/5ca70d915a67
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Reporter)

Comment 53

5 years ago
Verified in the 2012-07-09 build of Firefox Nightly/16.0a1 for OS X.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.