Closed Bug 625652 Opened 9 years ago Closed 9 years ago

make sure accessible tree is correct when rendered text is changed

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b11

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 625653
Attached patch patch (obsolete) — Splinter Review
Attachment #504714 - Flags: review?(bolterbugz)
Status: NEW → ASSIGNED
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
Attachment #504714 - Flags: superreview?(roc)
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 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?
(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?
(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 on attachment 504714 [details] [diff] [review]
patch

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

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

please give me details what you keep in mind
(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.
We have ContentInserted and ContentRemoved methods. ContentInserted calls ProcessContentInserted when layout is done. ContentRemoved process the notification immediately.
+    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.
(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.
(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 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?
Attachment #504714 - Flags: review?(bolterbugz) → review+
Attached patch patch2 (obsolete) — Splinter Review
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.
Attachment #504714 - Attachment is obsolete: true
Attachment #507118 - Flags: superreview?(roc)
Attachment #507118 - Flags: review?(bolterbugz)
Attachment #504714 - Flags: superreview?(roc)
Do we have to do this for FF4? This is not a regression as I understand it.
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?
Attachment #507118 - Flags: superreview?(roc) → superreview-
(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.
(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.
Attached patch patch3Splinter Review
Attachment #507118 - Attachment is obsolete: true
Attachment #507394 - Flags: superreview?(roc)
Attachment #507394 - Flags: approval2.0?
Attachment #507118 - Flags: review?(bolterbugz)
Attachment #507394 - Flags: superreview?(roc)
Attachment #507394 - Flags: superreview+
Attachment #507394 - Flags: approval2.0?
Attachment #507394 - Flags: approval2.0+
mochitest for remove text data case, it appears this case is handled by nsCSSFrameConstructor::ContentRemoved, so that it shouldn't be handled by TextReflow().
Attachment #507405 - Flags: review?(marco.zehe)
Comment on attachment 507405 [details] [diff] [review]
additional mochitest

Very good, thanks! r=me
Attachment #507405 - Flags: review?(marco.zehe) → review+
patch landed on 2.0 beta11 - http://hg.mozilla.org/mozilla-central/rev/95a9192221ea
mochitest landed - http://hg.mozilla.org/mozilla-central/rev/dc8a96be2009
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b11
Reopening until we sort out regression bug 629912.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.