Last Comment Bug 395900 - Image frame changes via :before/:after causes recreation of accessible objects
: Image frame changes via :before/:after causes recreation of accessible objects
Status: NEW
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Linux
: -- major with 2 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 686400
Blocks: aria nicea11y
  Show dependency treegraph
 
Reported: 2007-09-12 07:10 PDT by Scott Haeger
Modified: 2011-11-20 22:24 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
If frame doesn't actually change, don't fire notification to invalidate a11y tree (950 bytes, patch)
2007-10-24 11:31 PDT, Aaron Leventhal
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
If frame *type* doesn't actually change, don't fire notification to invalidate a11y tree (1.34 KB, patch)
2007-10-25 07:24 PDT, Aaron Leventhal
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review

Description Scott Haeger 2007-09-12 07:10:38 PDT
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 2007-10-24 11:27:51 PDT
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 2007-10-24 11:31:36 PDT
Created attachment 286033 [details] [diff] [review]
If frame doesn't actually change, don't fire notification to invalidate a11y tree
Comment 3 Boris Zbarsky [:bz] 2007-10-24 12:57:12 PDT
I don't see how that test could possibly be true here.  What am I missing?
Comment 4 Aaron Leventhal 2007-10-24 13:05:09 PDT
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 [:bz] 2007-10-24 13:27:54 PDT
> 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 2007-10-24 20:42:51 PDT
aContent is for the <span>
Both frame and newFrame are the same inline frame for that span.

Comment 7 Boris Zbarsky [:bz] 2007-10-24 20:46:25 PDT
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 [:bz] 2007-10-24 21:01:43 PDT
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 [:bz] 2007-10-24 21:02:14 PDT
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 2007-10-25 07:24:43 PDT
Created attachment 286159 [details] [diff] [review]
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 [:bz] 2007-10-25 08:02:43 PDT
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 2007-10-25 13:08:41 PDT
Boris, how do I test that?
Comment 13 Boris Zbarsky [:bz] 2007-10-25 15:32:53 PDT
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 2007-10-25 16:07:27 PDT
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 [:bz] 2007-10-25 16:28:49 PDT
Assuming that changing that class also changed the :before content on the link, sure.
Comment 16 Aaron Leventhal 2007-10-26 06:39:30 PDT
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 [:bz] 2007-10-26 11:56:49 PDT
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] 2009-06-16 11:53:45 PDT
Mass un-assigning bugs assigned to Aaron.
Comment 19 Marco Zehe (:MarcoZ) 2011-11-01 06:27:01 PDT
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 2011-11-20 22:24:35 PST
(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

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