Last Comment Bug 750612 - [Mac] Too much is exposed to VoiceOver that is actually not currently visible/interactable
: [Mac] Too much is exposed to VoiceOver that is actually not currently visible...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Mac OS X
: -- major (vote)
: mozilla16
Assigned To: Hubert Figuiere [:hub]
:
: alexander :surkov
Mentors:
Depends on: 758304
Blocks: osxa11y
  Show dependency treegraph
 
Reported: 2012-04-30 23:01 PDT by Marco Zehe (:MarcoZ)
Modified: 2012-07-09 07:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't expose invisible items. r= (1.21 KB, patch)
2012-05-02 17:44 PDT, Hubert Figuiere [:hub]
mzehe: feedback-
Details | Diff | Splinter Review
Don't expose invisible items. r= (2.91 KB, patch)
2012-05-09 15:19 PDT, Hubert Figuiere [:hub]
surkov.alexander: feedback-
Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. r= (5.31 KB, patch)
2012-05-15 16:21 PDT, Hubert Figuiere [:hub]
surkov.alexander: feedback-
Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. (6.71 KB, patch)
2012-05-23 13:23 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. (13.31 KB, patch)
2012-05-30 15:07 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. (13.67 KB, patch)
2012-06-11 18:22 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. (13.61 KB, patch)
2012-06-11 20:00 PDT, Hubert Figuiere [:hub]
dbolter: feedback+
Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. (13.60 KB, patch)
2012-06-27 18:45 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Don't expose invisible property pages in a deck frame. (13.35 KB, patch)
2012-07-03 11:44 PDT, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2012-04-30 23:01:37 PDT
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.
Comment 1 Marco Zehe (:MarcoZ) 2012-04-30 23:09:54 PDT
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.
Comment 2 Hubert Figuiere [:hub] 2012-05-01 12:54:48 PDT
One major thing is that we have all the AXWebArea that are exposed, while Safari always have the current one.
Comment 3 Hubert Figuiere [:hub] 2012-05-02 17:44:21 PDT
Created attachment 620540 [details] [diff] [review]
Don't expose invisible items. r=
Comment 4 alexander :surkov 2012-05-02 20:49:36 PDT
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
Comment 5 Marco Zehe (:MarcoZ) 2012-05-02 23:08:55 PDT
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.
Comment 6 alexander :surkov 2012-05-02 23:22:48 PDT
(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
Comment 7 Hubert Figuiere [:hub] 2012-05-03 13:45:00 PDT
Yeah I'm missing the update part.
Comment 8 Hubert Figuiere [:hub] 2012-05-09 15:19:01 PDT
Created attachment 622534 [details] [diff] [review]
Don't expose invisible items. r=
Comment 9 Hubert Figuiere [:hub] 2012-05-09 15:48:00 PDT
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.
Comment 10 alexander :surkov 2012-05-10 04:11:03 PDT
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?
Comment 11 Hubert Figuiere [:hub] 2012-05-15 16:21:00 PDT
Created attachment 624231 [details] [diff] [review]
Don't expose invisible property pages in a deck frame. r=
Comment 12 Hubert Figuiere [:hub] 2012-05-15 16:22:34 PDT
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.
Comment 13 alexander :surkov 2012-05-16 01:20:08 PDT
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
Comment 14 alexander :surkov 2012-05-16 01:28:44 PDT
(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
Comment 15 David Bolter [:davidb] 2012-05-22 06:41:04 PDT
Hub does Safari simply return null for children of inactive tab documents?
Comment 16 Hubert Figuiere [:hub] 2012-05-22 09:55:56 PDT
(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.
Comment 17 Hubert Figuiere [:hub] 2012-05-23 13:23:04 PDT
Created attachment 626578 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
Comment 18 Hubert Figuiere [:hub] 2012-05-23 13:25:22 PDT
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
Comment 19 Hubert Figuiere [:hub] 2012-05-23 15:04:00 PDT
> ::: 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.
Comment 20 Hubert Figuiere [:hub] 2012-05-23 23:39:35 PDT
Also this might crash in debug, but this is unrelated to this. I have a fix pending for the crash too.
Comment 21 alexander :surkov 2012-05-25 02:17:53 PDT
(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 alexander :surkov 2012-05-25 02:47:17 PDT
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)
Comment 23 alexander :surkov 2012-05-26 06:28:03 PDT
(In reply to alexander :surkov from comment #22)

> 1) rename XULTabpanelsAccessible to XULDeckAccessible

actually you can go with nsEnumRoleAccessible
Comment 24 Hubert Figuiere [:hub] 2012-05-30 15:07:21 PDT
Created attachment 628497 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
Comment 25 alexander :surkov 2012-05-30 18:41:46 PDT
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).
Comment 26 Hubert Figuiere [:hub] 2012-06-01 16:50:07 PDT
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 alexander :surkov 2012-06-01 19:22:09 PDT
(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?
Comment 28 Hubert Figuiere [:hub] 2012-06-11 18:21:33 PDT
(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.
Comment 29 Hubert Figuiere [:hub] 2012-06-11 18:22:14 PDT
Created attachment 632099 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
Comment 30 Hubert Figuiere [:hub] 2012-06-11 19:42:52 PDT
I need to rebase the patch, again.
Comment 31 Hubert Figuiere [:hub] 2012-06-11 19:59:52 PDT
Note that the whole nsXULTabAccessible has been renamed now :-)
Comment 32 Hubert Figuiere [:hub] 2012-06-11 20:00:04 PDT
Created attachment 632113 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
Comment 33 David Bolter [:davidb] 2012-06-15 10:46:55 PDT
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)
Comment 34 Trevor Saunders (:tbsaunde) 2012-06-26 14:50:30 PDT
(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 35 Trevor Saunders (:tbsaunde) 2012-06-26 15:08:27 PDT
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.
Comment 36 Hubert Figuiere [:hub] 2012-06-26 19:00:13 PDT
(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 alexander :surkov 2012-06-26 23:02:59 PDT
(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).
Comment 38 Hubert Figuiere [:hub] 2012-06-27 18:45:07 PDT
Created attachment 637339 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
Comment 39 Hubert Figuiere [:hub] 2012-06-27 18:47:03 PDT
Comment on attachment 637339 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.

Nits addressed.
Comment 40 Trevor Saunders (:tbsaunde) 2012-06-27 19:21:05 PDT
(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() ?
Comment 41 Trevor Saunders (:tbsaunde) 2012-06-27 19:23:00 PDT
(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.
Comment 42 Hubert Figuiere [:hub] 2012-06-27 19:36:42 PDT
(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.
 */
Comment 43 Hubert Figuiere [:hub] 2012-06-29 00:50:11 PDT
(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.
Comment 44 Trevor Saunders (:tbsaunde) 2012-07-01 12:26:03 PDT
(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?
Comment 45 Hubert Figuiere [:hub] 2012-07-03 11:39:58 PDT
(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.
Comment 46 Hubert Figuiere [:hub] 2012-07-03 11:44:12 PDT
Created attachment 638820 [details] [diff] [review]
Don't expose invisible property pages in a deck frame.
Comment 47 Trevor Saunders (:tbsaunde) 2012-07-03 21:02:46 PDT
> > > 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.
Comment 48 Hubert Figuiere [:hub] 2012-07-03 21:13:10 PDT
(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 49 Trevor Saunders (:tbsaunde) 2012-07-04 15:25:26 PDT
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.
Comment 50 Hubert Figuiere [:hub] 2012-07-04 15:30:49 PDT
(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.
Comment 53 Marco Zehe (:MarcoZ) 2012-07-09 07:59:02 PDT
Verified in the 2012-07-09 build of Firefox Nightly/16.0a1 for OS X.

Note You need to log in before you can comment on or make changes to this bug.