1113389 - loading google creates accessibles without firing show events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

to reproduce just load google.com (with e10s on this will crash, otherwise there will just be missing events)

The problem is that CacheChildrenInSubtree() can create accessibles but it doesn't know if they're new or just being recached because of InvalidateChildren().  CacheChildrenInSubtree() can create child accessibles outside of those created in UpdateTree() which should be responsible for firing events.

related IRC discussion:
< tbsaunde> surkov: have a few minutes to see if you can reproduce something?
<@surkov> sure, what do I need for that?
< tbsaunde> surkov: I think just opening google with a debugger open
< tbsaunde> I'm seeing ProcessContentInserted() called with an array of one element which is a br in a div in a html input and when its called for that it creates an accessible from CacheChildrenInsubtree
<@surkov> ok, next?
-!- icaaq|afk is now known as icaaq
< tbsaunde> surkov: you see that too?
<@surkov> tbsaunde: I didn’t try but that sounds reasoable, what’s wrong do you see in that?
<@surkov> ProcessContentInserted() is supposed to call CacheChildrenInsubtree to create a subtree
< tbsaunde> surkov: well, it means accessibles are created and added to subtree, but there's no show event
<@surkov> UpdateTree should take care of it
<@surkov> is that CacheChildrenInsubtree called from UpdateTree()?
< tbsaunde> surkov: should all accessible creation happen through UpdateTree() ?
< tbsaunde> surkov: no, directly
<@surkov> oh, that one
<@surkov> and that UpdateTree() fails to handle that?
< tbsaunde> surkov: yeah, seems so
<@surkov> quite strange
< tbsaunde> the inserted content UpdateTree is called for is just the br, but some random other div gets a new accessible
< tbsaunde> surkov: can you into it please?
<~davidb> is there an editable involved?
<@surkov> I guess that div is a parent
<@surkov> I guess I can take a look, can you file a bug?
< tbsaunde> davidb: yeah, there's some inputs around

Comment 1

[alexander :surkov (:asurkov)]

can you think of test case? In general it sounds like layout issue since we should have insertion notification for that div before 'br' or even instead it.

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> can you think of test case? In general it sounds like layout issue since we
> should have insertion notification for that div before 'br' or even instead
> it.

google.com is the only thing I have, its probably easiest to get something smaller by seeing what happens in gdb.

Comment 3

[Trevor Saunders (:tbsaunde)]

Or maybe you could go back and find some of the other bugs where stuff like this has happened and try loading those test cases with e10s and see what happens.

Comment 4

[alexander :surkov (:asurkov)]

can you check this build out if it helps https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-349a29a77c03/

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> can you check this build out if it helps

not easily, just a patch would be much  easier.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > can you check this build out if it helps
> 
> not easily, just a patch would be much  easier.

actually I managed to dig up the patch from tbpl, and it seems to fix the issue.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> actually I managed to dig up the patch from tbpl, and it seems to fix the
> issue.

cool, thanks for checking it out. I'll finish it up then.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8542232 [details] [diff] [review]
patch

>+   * Cache children if necessary. Return false is no caching is required.
>    */
>   bool EnsureChildren();

nothing checks the return value right? so just make it return void.

>+  inline void SetSurvivingInUpdate(bool aIsSurviving)

nit, inline is redundant

> 
>   static const uint8_t kChildrenFlagsBits = 2;
>-  static const uint8_t kStateFlagsBits = 8;
>+  static const uint8_t kStateFlagsBits = 9;
>   static const uint8_t kContextFlagsBits = 1;
>   static const uint8_t kTypeBits = 6;
>   static const uint8_t kGenericTypesBits = 13;

looks like we're running low on space again :/

>+    // We have a DOM/layout change under the container accessible, and its tree
>+    // might need an update. Since DOM/layout change of the element may affect
>+    // on the accessibleness of adjacent elements (for example, insertion of
>+    // extra HTML:body make the old body accessible) then we have to recache
>+    // children of the container, and then fire show/hide events for a change.
>+    UpdateTreeOnInsertion(container);
>+    break;

that's rather strange loop.

>     var gQueue = null;
>-    //gA11yEventDumpToConsole = true; // debug stuff
>+    gA11yEventDumpToConsole = true; // debug stuff

remember to disable it

>-      testAccessibleTree("listbox", accTree);
>+  //    testAccessibleTree("listbox", accTree);

why do you disable it?

>                 ]
>-},
>+              },

extranious whitespace changes are really annoying.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> >   static const uint8_t kChildrenFlagsBits = 2;
> >-  static const uint8_t kStateFlagsBits = 8;
> >+  static const uint8_t kStateFlagsBits = 9;
> >   static const uint8_t kContextFlagsBits = 1;
> >   static const uint8_t kTypeBits = 6;
> >   static const uint8_t kGenericTypesBits = 13;
> 
> looks like we're running low on space again :/

yeah, unfortunately

> >+    // We have a DOM/layout change under the container accessible, and its tree
> >+    // might need an update. Since DOM/layout change of the element may affect
> >+    // on the accessibleness of adjacent elements (for example, insertion of
> >+    // extra HTML:body make the old body accessible) then we have to recache
> >+    // children of the container, and then fire show/hide events for a change.
> >+    UpdateTreeOnInsertion(container);
> >+    break;
> 
> that's rather strange loop.

agree, otherwise extra check after loop which is also not nice

> >     var gQueue = null;
> >-    //gA11yEventDumpToConsole = true; // debug stuff
> >+    gA11yEventDumpToConsole = true; // debug stuff
> 
> remember to disable it
> 
> >-      testAccessibleTree("listbox", accTree);
> >+  //    testAccessibleTree("listbox", accTree);
> 
> why do you disable it?

that was for testing, not part of landing

> >                 ]
> >-},
> >+              },
> 
> extranious whitespace changes are really annoying.

just some fixes as we go, dealing with them separately also not nice

Comment 11

[Trevor Saunders (:tbsaunde)]

> > >-      testAccessibleTree("listbox", accTree);
> > >+  //    testAccessibleTree("listbox", accTree);
> > 
> > why do you disable it?
> 
> that was for testing, not part of landing

if you revert it before committing fine.

> > >                 ]
> > >-},
> > >+              },
> > 
> > extranious whitespace changes are really annoying.
> 
> just some fixes as we go, dealing with them separately also not nice

what's wrong with doing them by themselves?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> > > extranious whitespace changes are really annoying.
> > 
> > just some fixes as we go, dealing with them separately also not nice
> 
> what's wrong with doing them by themselves?

that's ok but it's extra work and it may be avoided

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c69a53cb9a04