Closed Bug 452564 Opened 12 years ago Closed 11 years ago

can't create accessibles for table children when visibility style of table is changed

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

Attached file testcase
When visibility style is changed and we fail to create accessible for table 

ASSERTION: No accessible for parent table and it didn't have role of presentation: 'tableRoleMapEntry && !nsCRT::strcmp(tableRoleMapEntry->roleString, "presentation")', file c:/mozilla/fx/accessible/src/base/nsAccessibilityService.cpp, line 1533

originally bug comes from dojo's dialog (http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Dialog.html)

testcase is included, just open DOMI and click 'show table' button.

this bug may be related with the bug 452479.
here's we fail http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1436

frame->GetStyleVisibility()->IsVisible() is false for table element
stack trace is

 >	accessibility.dll!nsAccessibilityService::GetAccessible(nsIDOMNode * aNode=0x04bb62dc, nsIPresShell * aPresShell=0x04b99f98, nsIWeakReference * aWeakShell=0x04b9efd8, nsIFrame * * aFrameHint=0x0012e870, int * aIsHidden=0x0012e874, nsIAccessible * * aAccessible=0x0012e9c4)  Line 1527	C++
 	accessibility.dll!nsAccessibilityService::GetAccessibleInWeakShell(nsIDOMNode * aNode=0x04bb62dc, nsIWeakReference * aWeakShell=0x04b9efd8, nsIAccessible * * aAccessible=0x0012e9c4)  Line 1246 + 0x2e	C++
 	accessibility.dll!nsDocAccessible::GetAccessibleInParentChain(nsIDOMNode * aNode=0x04bb6460, int aCanCreate=1, nsIAccessible * * aAccessible=0x0012e9c4)  Line 2074	C++
 	accessibility.dll!nsDocAccessible::InvalidateCacheSubtree(nsIContent * aChild=0x04bb6440, unsigned int aChangeEventType=4)  Line 1928 + 0x38	C++
 	accessibility.dll!nsAccessibilityService::InvalidateSubtreeFor(nsIPresShell * aShell=0x04b99f98, nsIContent * aChangeContent=0x04bb6440, unsigned int aEvent=4)  Line 2014 + 0x21	C++
 	gklayout.dll!nsFrameManager::ReResolveStyleContext(nsPresContext * aPresContext=0x04b94a00, nsIFrame * aFrame=0x04bb326c, nsIContent * aParentContent=0x04bb62c0, nsStyleChangeList * aChangeList=0x0012f044, nsChangeHint aMinChange=5)  Line 1462	C++

so we have in testcase:
<table>
  <tr><td>bla</td></tr>
</table>

so first of all we get notification the style is changed for text frame, then table cell, then table row and then table section and I assume then we should get it for table itself.

So I wonder is the a reason we get style notifications from the bottom to top?

cc'ing Robert.
I guess that's just what the style system does. There's nothing wrong with that.

Maybe you should respond to style changes by just invalidating your accessibles and then dispatch an asynchronous event to do whatever other processing you need? That way, the other processing will happen after all style changes have completed.
I don't know how you monitor style changes, can you point me to that code?
here's the accessible tree invalidation when we get frame or DOM tree changes (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsDocAccessible.cpp#1833). Here's we fail (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsDocAccessible.cpp#1928) because we can't get parent accessible (because we didn't get style notification). For frame tree changes we do most processing after delay (like you suggest) but some stuffs we prepare immediately. So if it the current behaviour is intended then I think we should rethink our invalidation code.
Should nsFrameManager::ReResolveStyleContext call InvalidateSubtreeFor only for the root of the frame subtree that's reresolving style? That would solve your problem and be more efficient too.
(In reply to comment #6)
> Should nsFrameManager::ReResolveStyleContext call InvalidateSubtreeFor only for
> the root of the frame subtree that's reresolving style? That would solve your
> problem and be more efficient too.
> 

I believe this should work if it concerns display:none/display:xxx and visibility:hidden/visibility:visible. When is ReResolveStyleContext called?
It's called whenever something changes that could affect the style of a frame subtree.

The main entry point is the call from nsFrameManager::ComputeStyleChangeFor. If you move the call to InvalidateSubtreeFor out to that method, you might win.

The main thing I'm concerned about is whether the accessiblity tree structure matches the frame structure traversed by ReResolveStyleContext, which is basically the DOM structure. Do parent-child relationships int he accessible tree exactly match DOM element parent-child relationships?
Sometimes we create accessibles for frames without DOM Node. For example, we create accessible for list bullet, its parent is listitem. Also we create accessible for treeitems/treecells for custom tree views. But as far as I know if accessible has own DOM node then its parent will be accessible with own DOM node too.

Aaron, is it correct, are there some exceptions?
That sounds OK. The main thing we want to avoid is that frame F has descendant frame G, but G's accessible object is not a descendant of F's accessible.
er, to be more precise, we want to avoid that element E has a descendant element D, but D's accessible is not a descendant of E's accessible.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #336432 - Flags: superreview?(roc)
Attached patch patch2 (obsolete) — Splinter Review
previous patch was wrong
Attachment #336432 - Attachment is obsolete: true
Attachment #336433 - Flags: superreview?(roc)
Attachment #336432 - Flags: superreview?(roc)
Comment on attachment 336433 [details] [diff] [review]
patch2

Marco, could you test the patch on possible regressions?
Attachment #336433 - Flags: review?(marco.zehe)
Attachment #336433 - Flags: review?(aaronleventhal)
Alex, I have trouble reproducing the original bug. This testcase works for me even without the patch, so i am not sure what to test for. In other places, like the test for bug 434464, I don't see a problem. Also normal surfing seems OK.
(In reply to comment #15)
> Alex, I have trouble reproducing the original bug. This testcase works for me
> even without the patch, so i am not sure what to test for. In other places,
> like the test for bug 434464, I don't see a problem. Also normal surfing seems
> OK.

You should run debug build on windows. Just open DOMI, ensure it shows accessible DOM nodes in 'DOM Nodes' view and click 'Show table' button in the testcase, you should see assertions (a debug dialogs).
Attachment #336433 - Flags: superreview?(roc) → superreview+
Attachment #336433 - Flags: review?(marco.zehe) → review+
Comment on attachment 336433 [details] [diff] [review]
patch2

r=me. Can't find anything wrong and ran with this patch for a day. Aaron should look over it, too.
(In reply to comment #10)
> That sounds OK. The main thing we want to avoid is that frame F has descendant
> frame G, but G's accessible object is not a descendant of F's accessible.

The only exception to this is for image maps. We create the <area> accessibles as children of the <img>.
Alex, can you give me an overview of how this patch works? It's been a while since I looked at this code.
(In reply to comment #19)
> Alex, can you give me an overview of how this patch works? It's been a while
> since I looked at this code.

Oops, I made progress to forget about this patch and bug :).

Ok.

ReResolveStyleContext is called by ComputeStyleChangeFor only. When style is changed then ComputeStyleChangeFor is called and then ReResolveStyleContext is called for descendants. Now we fire show/hide events from ReResolveStyleContext, this patch makes to fire them in ComputeStyleChangeFor. It means
1) we get lesser show/hide events, some performance win when we use hard detections if this event is descendants of another event.
2) we won't fire show/hide event for children before we fire show/hide event on parent (because ReResolveStyleContext is called on children firstly) because actually we won't fire show/hide events for children at all. And this means we fix the bug.
You introduced this behaviour in the bug 354745, I don't see there the reason why you used ReResolveStyleContext. Note, this behaviour introduced too many events, so you fixed this in bug 391847 by coalesce same events from subtree.
Aaron, does it look correct?
Aaron, ping?
(In reply to comment #21)
> You introduced this behaviour in the bug 354745, I don't see there the reason
> why you used ReResolveStyleContext. Note, this behaviour introduced too many
> events, so you fixed this in bug 391847 by coalesce same events from subtree.

The reason is in the description of bug 354745 (https://bugzilla.mozilla.org/show_bug.cgi?id=354745#c0)
> When frames change visibility or display indirectly, e.g. via an ancestor
> class/attribute change which causes computed style to change, the accessibility
> show/hide/reorder events are not getting fired.

Unfortunately, in that case, the frame passed in to ComputeStyleChangeFor() is not the frame which will change.

See the last two items in the testcase for that bug:
"Via parent display:none styling:"
and
"Via parent visibility:hidden styling"

I don't see how your patch would catch those changes.
Comment on attachment 336433 [details] [diff] [review]
patch2

Cancelling review unless I'm proven wrong somehow.
Attachment #336433 - Flags: review?(aaronleventhal)
I should note that those tests should be in our regression test suite.  Then there would be no issue of regressing them....

And yes, you have to get notified on all style changes, not just on the toplevel one.  That said, you _could_ try to get the same effect by walking the changelist and notifying based on that, right?

Alternately you could notify in ReResolveStyleContext before processing kids?
Depends on: 469985
Ok, I'll put mochitest in bug 469985 to make sure I won't break anything.
No longer depends on: 469985
Blocks: eventa11y
Blocks: tablea11y
Attached patch patch3 (obsolete) — Splinter Review
Fire accessible events on top frame that is changed its visibility (shown or hidden), do not fire them on child frames in this case. There is one thing I doubt in is it really safe to not fire events for child provider frames. I think it's ok for tables (nsOuterTableFrame and nsTableFrame) but I don't know about any other cases.
Attachment #336433 - Attachment is obsolete: true
Attachment #390993 - Flags: superreview?(roc)
Attachment #390993 - Flags: review?(ginn.chen)
Nit:

-                                      nsChangeHint       aMinChange)
+                                      nsChangeHint       aMinChange,
+                                      PRBool aFireA11yEvents)

                                     NS_SubtractHint(aMinChange,
-                                                    nsChangeHint_ReflowFrame));
+                                                    nsChangeHint_ReflowFrame),
+                                    fireA11yEvents);
 
-                          nsChangeHint       aMinChange);
+                          nsChangeHint       aMinChange,
+                          PRBool aDoFireA11yEvents);
 };
(In reply to comment #29)
> Nit:

>                                      NS_SubtractHint(aMinChange,
> -                                                   
> nsChangeHint_ReflowFrame));
> +                                                    nsChangeHint_ReflowFrame),
> +                                    fireA11yEvents);
> 

I aligned fireA11yEvents with NS_SubtractHint. Anything wrong?
(In reply to comment #30)
> I aligned fireA11yEvents with NS_SubtractHint. Anything wrong?

You're right, my bad.
Attachment #390993 - Flags: review?(ginn.chen) → review+
Comment on attachment 390993 [details] [diff] [review]
patch3

Rename aFireA11yEvents to aFireAccessibilityEvents please, not everyone knows the A11y contraction
Attachment #390993 - Flags: superreview?(roc) → superreview+
Attached patch patch4Splinter Review
with Ginn's and Roc's addressed comments
Attachment #390993 - Attachment is obsolete: true
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/f7ea35747ee3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.