395900 - Image frame changes via :before/:after causes recreation of accessible objects

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scott Haeger]

This checkbox test case https://bugzilla.mozilla.org/attachment.cgi?id=280484 (you will need the three images) recreates the ATK object upon a state change.  Do the following to test.

1) Open up the test case and note that the checkboxes are visible.
2) Open up Accerciser and find one of the checkboxes.
3) Click on the checkbox to cause a state change.  You will notice that the checkbox goes defunct and that a new object is created.

Comment 1

[Aaron Leventhal]

Actually the root cause is that we are firing an extra internal invalidation event even though the frame didn't actually change -- it was just in a method where it could change.

Comment 2

[Aaron Leventhal]

Attachment: If frame doesn't actually change, don't fire notification to invalidate a11y tree

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't see how that test could possibly be true here.  What am I missing?

Comment 4

[Aaron Leventhal]

There's a <span> with a :before style rule. The :before child frame changes from checked.gif to unchecked.gif, but the frame for the span doesn't actually change.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> There's a <span> with a :before style rule. The :before child frame changes

It usually can't change without a frame reconstruct on the span's frame.  What are the two frames and aContent in RecreateFramesForContent in this case?

Comment 6

[Aaron Leventhal]

aContent is for the <span>
Both frame and newFrame are the same inline frame for that span.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That should not be happening.  If it is, something is seriously buggy.  Note that as far as the frame constructor is concerned, this function is exactly equivalent to aContent being removed from the document and then reinserted into the document...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, actually...  Are you _sure_ that they're "the same inline frame"?  Or is it that |frame| gets destroyed by the ContentRemoved() call, the memory gets placed on the recycling allocator freelist, and we allocate the new frame for the span off that freelist, which puts it at the same memory location as the old one used to occupy?

This chain of events I can absolutely see happening, in which case your equality test will sometimes randomly test true: you're comparing a pointer to deallocated memory with a pointer to newly-allocated memory, and sometimes you'll happen to get equality.

But make no mistake.  This is not a method in which the frame "could change".  This is a method in which the frame is deallocated, with the memory scribbled on with an 0xdddddddd pattern in a debug build, and another frame is possibly allocated.  If there is some circumstance where that does not happen, that's a serious bug.

Of course, using the |frame| pointer for anything other than maybe assigning some other frame pointer to it after the ContentRemoved() call is also a serious bug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 286033 [details] [diff] [review]
If frame doesn't actually change, don't fire notification to invalidate a11y tree

This patch is wrong.  If it doesn't break our existing a11y tests, that means we don't have the right tests.

Comment 10

[Aaron Leventhal]

Attachment: If frame *type* doesn't actually change, don't fire notification to invalidate a11y tree

Thanks Bz. The type of accessible object can sometimes be dependent on the frame type (not just the element's markup), so if the markup doesn't change and the frame type doesn't change, there is no need to fire the a11y invalidation.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 286159 [details] [diff] [review]
If frame *type* doesn't actually change, don't fire notification to invalidate a11y tree

So you don't care if some child frame of oldFrame/newFrame changed type?

Comment 12

[Aaron Leventhal]

Boris, how do I test that?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Simplest way is to enable a sheet that, in a single stylesheet:

1)  Changes the :before content for the span
2)  Changes the style for some child of the span (e.g sets it to display:none).

Comment 14

[Aaron Leventhal]

Something like this?
.table span { display: table; }

  <button onclick="document.getElementById('link13').className = (document.getElementById('link13').className == 'table') ? 'inline' : 'table';">Toggle inline/table for span child of link #13</button>
  <a id="link13" href="http://www.google.com" class="table"><span>Link #13</span></a>
</td>

Comment 15

[Boris Zbarsky [:bzbarsky]]

Assuming that changing that class also changed the :before content on the link, sure.

Comment 16

[Aaron Leventhal]

I don't think we need to get too stressed about getting that one fixed for ff3.
We're short on time, and i don't think real web pages will use :before for ARIA widgets as long as IE doesn't support it.

After chatting with Scott Haeger on IRC, I'm removing orca:high from this.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 286159 [details] [diff] [review]
If frame *type* doesn't actually change, don't fire notification to invalidate a11y tree

This looks wrong to me (it'll miss notifications).

Comment 18

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

Mass un-assigning bugs assigned to Aaron.

Comment 19

[Marco Zehe (:MarcoZ)]

Alexander, is this still valid after all the reorg happening for Firefox 4? I would suspect not, but would like you to take a look. Rich, can you still reproduce?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #19)
> Alexander, is this still valid after all the reorg happening for Firefox 4?
> I would suspect not, but would like you to take a look. Rich, can you still
> reproduce?

I bet it's still valid, we have more generic bug 686400 for problems like this

Comment 21

[James Teh [:Jamie]]

This test case no longer works, but I verified that we no longer re-create accessibles for this case after the fix for bug 686400. Test case:
data:text/html,<style>[role=checkbox][aria-checked=false]:before { content: url('https://via.placeholder.com/10x10?text=-'); } [role=checkbox][aria-checked=true]:before { content: url('https://via.placeholder.com/10x10?text=x'); }</style><div role="checkbox" aria-checked="false" aria-label="Test" tabindex="0" onclick="this.setAttribute('aria-checked', 'true');"></div>
Clicking the checkbox doesn't recreate the accessible after the fix, but does recreate it before the fix.