625652 - make sure accessible tree is correct when rendered text is changed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Rendered text may be changed due to various reasons, it may become empty or not empty what should lead to destruction or construction of text accessible. Keeping an accessible alive for empty rendered text will make portions of web page inaccessible.

I can't say at 100% it's regression since we didn't have protection in the past but I think situation in fx4 should be regressed at some point. I need to think of examples to find a case where we've got broken. I don't nominate it to blocking but it should be definitely fixed in fx4.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Robert, should I care about early returns in nsTextFrame::ReflowText()? In other words, can any of these cases occur when rendered text wasn't empty and nsCSSFrameConstructor::ContentRemoved wasn't called for this text node or its parents?

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6438

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6580

Comment 3

[alexander :surkov (:asurkov)]

fyi: test cases in mochitests don't work in FX3.6 as well, i.e. we don't fire events and don't update the tree. However I didn't test that with screen reader.

Comment 4

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

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

Aside:

I noticed this during my initial scan of the patch:
> #ifdef NOISY_REFLOW
What is this and friend REALLY_NOISY_REFLOW? Could we be checking these (not defined) when layout calls into us for perf possibilities?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> Could we be checking these (not
> defined) when layout calls into us for perf possibilities?

How are we going to improve a perf staying correct?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> Robert, should I care about early returns in nsTextFrame::ReflowText()? In
> other words, can any of these cases occur when rendered text wasn't empty and
> nsCSSFrameConstructor::ContentRemoved wasn't called for this text node or its
> parents?
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6438

This one can occur when the rendered text was previously non-empty.

> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6580

This one is out-of-memory only.

Comment 7

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

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

>+      document->ContentRemoved(containerElm, textNode);

Maybe we should have a ProcessContentRemoved now.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Maybe we should have a ProcessContentRemoved now.

please give me details what you keep in mind

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > Maybe we should have a ProcessContentRemoved now.
> 
> please give me details what you keep in mind

Just to match ProcessContentInserted.

Comment 10

[alexander :surkov (:asurkov)]

We have ContentInserted and ContentRemoved methods. ContentInserted calls ProcessContentInserted when layout is done. ContentRemoved process the notification immediately.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    nsCOMPtr<nsIAccessibilityService> accService =
+      do_GetService("@mozilla.org/accessibilityService;1");


Can we cache the accessibility service in nsContentUtils please?

+    if (mNotificationController)
+      mNotificationController->ScheduleTextUpdate(aTextNode);

{}

Otherwise looks good, except if textAcc already exists, and the renderedText is nonempty, shouldn't we still notify something somewhere that the text might have changed? I thought we were going to store the rendered text somewhere so we could compare the new text against the old text.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> +    nsCOMPtr<nsIAccessibilityService> accService =
> +      do_GetService("@mozilla.org/accessibilityService;1");
> 
> 
> Can we cache the accessibility service in nsContentUtils please?

that should be variation of bug 606085. Perhaps it makes fix it prior this bug. Ideally I would get accessibility service directly from a11y. Neither layout nor content don't start accessibility since they operate with the service when nsIPressShell()->IsAccessibilityActive() returns true, therefore we could try to ask a11y for the cached service.

> Otherwise looks good, except if textAcc already exists, and the renderedText is
> nonempty, shouldn't we still notify something somewhere that the text might
> have changed? I thought we were going to store the rendered text somewhere so
> we could compare the new text against the old text.

Right. This bug is a first step.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (In reply to comment #2)
> > Robert, should I care about early returns in nsTextFrame::ReflowText()? In
> > other words, can any of these cases occur when rendered text wasn't empty and
> > nsCSSFrameConstructor::ContentRemoved wasn't called for this text node or its
> > parents?
> > 
> > http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6438
> 
> This one can occur when the rendered text was previously non-empty.
> 
> > http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#6580
> 
> This one is out-of-memory only.

Perhaps then I should move a11y text update notification on top of TextReflow() method.

Comment 14

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

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

r=me

(In reply to comment #12)
> (In reply to comment #11)
> > +    nsCOMPtr<nsIAccessibilityService> accService =
> > +      do_GetService("@mozilla.org/accessibilityService;1");
> > 
> > 
> > Can we cache the accessibility service in nsContentUtils please?
> 
> that should be variation of bug 606085. Perhaps it makes fix it prior this bug.
> Ideally I would get accessibility service directly from a11y. Neither layout
> nor content don't start accessibility since they operate with the service when
> nsIPressShell()->IsAccessibilityActive() returns true, therefore we could try
> to ask a11y for the cached service.

Could we just call a setter in layout when we first instantiate accessibility?

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch2

based on bug 628922 (fixed Roc's comment). I keep UpdateText notification in the end of TextReflow because text leaf accessible under empty input element may randomly appear or dissapear (I guess at some point perhaps it depends whether input is initialized or not the text of text node under input element gets removed). I think it's worth to deal with that later.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do we have to do this for FF4? This is not a regression as I understand it.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 507118 [details] [diff] [review]
patch2

nsIAccessibilityService.h needs an IID change. That means it needs a _MOZILLA_2_0_BRANCH interface if you want it for Gecko 2.0.

+    NS_ASSERTION(!textAcc,
+                 "Text node was removed but accessible is kept alive!");
+    return PL_DHASH_NEXT;
+  }
+
+  nsIFrame* textFrame = textNode->GetPrimaryFrame();
+  if (!textFrame) {
+    NS_ASSERTION(!textAcc,
+                 "Text node isn't rendered but accessible is kept alive!");

Why are these assertions? Isn't it possible for the AT to keep accessibles alive?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Do we have to do this for FF4? This is not a regression as I understand it.

Technically it's not a regression but Firefox 4 is more sensible to wrong accessible tree, for example, hypertext interface will return wrong information for a wrong tree and as consequence that makes the content inaccessible, on the another hand the user may never hit this bug in Firefox 3.6 if the tree is created after the changes, Firefox 4 creates the tree in more defined times.

In short Firefox 4 has more chances to get broken comparing to Firefox 3.6. Thus I think it should be fixed in Firefox 4 timeframe.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Comment on attachment 507118 [details] [diff] [review]
> patch2
> 
> nsIAccessibilityService.h needs an IID change. That means it needs a
> _MOZILLA_2_0_BRANCH interface if you want it for Gecko 2.0.

Ok, I think I can put this method on nsAccessibleService directly if bug 628922 will make layout to deal with nsAccessibilityService.

> +    NS_ASSERTION(!textAcc,
> +                 "Text node was removed but accessible is kept alive!");
> +    return PL_DHASH_NEXT;
> +  }
> +
> +  nsIFrame* textFrame = textNode->GetPrimaryFrame();
> +  if (!textFrame) {
> +    NS_ASSERTION(!textAcc,
> +                 "Text node isn't rendered but accessible is kept alive!");
> 
> Why are these assertions? Isn't it possible for the AT to keep accessibles
> alive?

AT can keep them alive but they can't be in our cache so that if we can reach them then something is wrong.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 21

[alexander :surkov (:asurkov)]

Attachment: additional mochitest

mochitest for remove text data case, it appears this case is handled by nsCSSFrameConstructor::ContentRemoved, so that it shouldn't be handled by TextReflow().

Comment 22

[Marco Zehe (:MarcoZ)]

Comment on attachment 507405 [details] [diff] [review]
additional mochitest

Very good, thanks! r=me

Comment 23

[alexander :surkov (:asurkov)]

patch landed on 2.0 beta11 - http://hg.mozilla.org/mozilla-central/rev/95a9192221ea
mochitest landed - http://hg.mozilla.org/mozilla-central/rev/dc8a96be2009

Comment 24

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

Reopening until we sort out regression bug 629912.