613058 - nsIFrame::GetRenderedText may return wrong result on character change DOM mutations

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(no longer active)]

nsDocAccessible::CharacterDataWillChange calls FireTextChangeEventForText with PR_FALSE as the third param (aIsInserted), and nsDocAccessible::CharacterDataChanged calls that function with PR_TRUE as the third param.  This is completely wrong.  The former notification happens before the character data is changed on the content node, and the latter happens after it, and that's the same thing no matter whether text has been inserted or removed.

Comment 1

[(no longer active)]

See here <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericDOMDataNode.cpp#345> and further down for how it happens.

Comment 2

[(no longer active)]

Attachment: WIP

Upon further investigation, what I said in comment 0 is not really a problem, since this code seems to only look at the removed text in CharacterDataWillChange and the added text in CharacterDataChanged (although I believe that the way I do things in this patch makes it much more clear).

But there's still a serious problem which I have also addressed in this patch, and it's that the code tries to read the added/removed text from the frame, which is not safe, I don't think, since layout might not be fully updated at that point, specifically for added text.

I won't have time to actually take this, but I'm posting the patch here in the hopes that someone picks it up and works on it.  Feedback from a layout peer (roc or smontagu perhaps) could also be useful here.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> nsDocAccessible::CharacterDataWillChange calls FireTextChangeEventForText with
> PR_FALSE as the third param (aIsInserted), and
> nsDocAccessible::CharacterDataChanged calls that function with PR_TRUE as the
> third param.  This is completely wrong.  The former notification happens before
> the character data is changed on the content node, and the latter happens after
> it, and that's the same thing no matter whether text has been inserted or
> removed.

This is not wrong. Perhaps not clear. Changes in CharacterDataWillChange/CharacterDataChanged do nothing because you dupes the code from FireTextChangeEventForText.

(In reply to comment #2)
> Created attachment 491405 [details] [diff] [review]
> WIP
> 
> Upon further investigation, what I said in comment 0 is not really a problem,
> since this code seems to only look at the removed text in
> CharacterDataWillChange and the added text in CharacterDataChanged (although I
> believe that the way I do things in this patch makes it much more clear).

Ok, that should be clearer but you still have dupe code.

> But there's still a serious problem which I have also addressed in this patch,
> and it's that the code tries to read the added/removed text from the frame,
> which is not safe, I don't think, since layout might not be fully updated at
> that point, specifically for added text.

The frame is that what we want until nsIDOMCharacterData provides rendered text.
Since the frame is not safe here then we should notified from layout rather than DOM mutation listener.

Comment 4

[(no longer active)]

(In reply to comment #3)
> This is not wrong. Perhaps not clear. Changes in
> CharacterDataWillChange/CharacterDataChanged do nothing because you dupes the
> code from FireTextChangeEventForText.

Right.  That duplicate code should be removed, which I forgot to do in this patch.

> The frame is that what we want until nsIDOMCharacterData provides rendered
> text.

nsIDOMCharacterData is supposed to report raw DOM node contents, so you can't get rendered text from it either now or in the future.

> Since the frame is not safe here then we should notified from layout rather
> than DOM mutation listener.

Agreed.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #3)
> > This is not wrong. Perhaps not clear. Changes in
> > CharacterDataWillChange/CharacterDataChanged do nothing because you dupes the
> > code from FireTextChangeEventForText.
> 
> Right.  That duplicate code should be removed, which I forgot to do in this
> patch.

Actually all we should do is to change aIsInserted on something more clear like aProcessTextInsertion or aTextChangeFlag enum { eProcessInsertion, eProcessRemoval }

Comment 6

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

Comment on attachment 491405 [details] [diff] [review]
WIP

Thanks Ehsan. I'll cancel feedback on this one since it seems we need to get called from layout. Ehsan do you have time to drive this one?

Comment 7

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

Approving final since we should try to sort this out for FF4.

Comment 8

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

(Aside: this might relate to bug 498015?)

Comment 9

[(no longer active)]

(In reply to comment #6)
> Comment on attachment 491405 [details] [diff] [review]
> WIP
> 
> Thanks Ehsan. I'll cancel feedback on this one since it seems we need to get
> called from layout. Ehsan do you have time to drive this one?

I'm not sure how best to do this.  The only way that I know one can get notified after a reflow is implementing nsIReflowCallback.  Roc, is that a suitable way to make sure that we're dealing with valid textrun data inside text frames?

Comment 10

[(no longer active)]

(In reply to comment #8)
> (Aside: this might relate to bug 498015?)

Seems like a dupe to me.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (In reply to comment #8)
> > (Aside: this might relate to bug 498015?)
> 
> Seems like a dupe to me.

Right, that could be dupe what depends on the fix.

We had a chat with bz and the idea is to get notification from layout when text frame gains or looses rendered text. And when a11y handles these notifications then we create/destroy accessibles for text nodes and/or fire text change events. bz is not sure about best place to add notifications and suggested to ping Robert for that.

Comment 12

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

nsIReflowCallback would be a good way to do this. But we need to only do it when accessibility is enabled.

Comment 13

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

Hmm, actually it would be more efficient to trigger the notification in BuildTextRunsScanner::AssignTextRun. We would want to fire it asynchronously, though, or at least in a scriptrunner, and only when accessibility is in use.

Comment 14

[(no longer active)]

(In reply to comment #12)
> nsIReflowCallback would be a good way to do this. But we need to only do it
> when accessibility is enabled.

Hmm, I was thinking about registering an nsIReflowCallback from the nsDocAccessible code itself.  Why do we need to worry about only doing that if accessibility is enabled?  I mean, the entire thing shouldn't run if it's not, right?

Comment 15

[(no longer active)]

(In reply to comment #13)
> Hmm, actually it would be more efficient to trigger the notification in
> BuildTextRunsScanner::AssignTextRun. We would want to fire it asynchronously,
> though, or at least in a scriptrunner, and only when accessibility is in use.

I'm not sure which notification you're talking about here.  Are you talking about the accessibility event?

Comment 16

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

I guess, although we don't want to fire an event every time a textrun is built.

Maybe we should only fire the event if a textrun is changed for an element on which someone called GetRenderedText?

(In reply to comment #14)
> Hmm, I was thinking about registering an nsIReflowCallback from the
> nsDocAccessible code itself.  Why do we need to worry about only doing that if
> accessibility is enabled?  I mean, the entire thing shouldn't run if it's not,
> right?

Currently PostReflowCallback is only used during reflow, but I suppose it could be made to work to request a callback after the next reflow too. Memory management would have to be fixed; currently if the presshell goes away, post-reflow callbacks are not cleaned up. But I don't recommend going this way anyway.

Comment 17

[(no longer active)]

Hmm, I don't like the idea of firing the event from layout that much, because there's all sorts of accessibility stuff that we'd need to figure out in layout that way...

What if we flush layout from the accessibility code?  Would that be a terrible idea?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)

> What if we flush layout from the accessibility code?  Would that be a terrible
> idea?

Yes, because it means make layout on frame construction.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Hmm, actually it would be more efficient to trigger the notification in
> BuildTextRunsScanner::AssignTextRun.

When does it trigger?

> We would want to fire it asynchronously,
> though, or at least in a scriptrunner, and only when accessibility is in use.

I think we can notify a11y asynchronously when a11y is run.

(In reply to comment #16)
> I guess, although we don't want to fire an event every time a textrun is built.

We would want notifications when rendered text is gained or lost. We don't want them if the text stays the same.

> Maybe we should only fire the event if a textrun is changed for an element on
> which someone called GetRenderedText?

I assume it's not about GetRenderedText called from a11y, right?

Comment 20

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

(In reply to comment #19)
> (In reply to comment #13)
> > Hmm, actually it would be more efficient to trigger the notification in
> > BuildTextRunsScanner::AssignTextRun.
> 
> When does it trigger?

Usually during layout --- first layout, and after text has changed --- but also at other times.

> > We would want to fire it asynchronously,
> > though, or at least in a scriptrunner, and only when accessibility is in use.
> 
> I think we can notify a11y asynchronously when a11y is run.

Good.

> We would want notifications when rendered text is gained or lost. We don't want
> them if the text stays the same.

That is not easy to optimize for.

> > Maybe we should only fire the event if a textrun is changed for an element on
> > which someone called GetRenderedText?
> 
> I assume it's not about GetRenderedText called from a11y, right?

Yes. I mean, if you haven't called GetRenderedText on one or more frames of a text node, why do you need to get a notification that the result of GetRenderedText has changed?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #13)
> > > Hmm, actually it would be more efficient to trigger the notification in
> > > BuildTextRunsScanner::AssignTextRun.
> > 
> > When does it trigger?
> 
> Usually during layout --- first layout, and after text has changed --- but also
> at other times.

We need perhaps a tricky thing. We need to distinguish things when it's first layout as a part of its container frame construction and when it's result of text changes. Text changes should result of a11y text change events. But we don't need to fire a11y text change events if rendered text appears if this text frame is somewhere inside of accessible subtree that was created when corresponding frame subree was constructed (nsCSSFrameConstructor::ContentRangeInserted). The same time we need to fire a11y text change events when we rendered text appears if nsCSSFrameConstructor::ContentRangeInserted was called for this text frame earlier.

Does it clear enough? Should I give more details?

> > I assume it's not about GetRenderedText called from a11y, right?
> 
> Yes. I mean, if you haven't called GetRenderedText on one or more frames of a
> text node, why do you need to get a notification that the result of
> GetRenderedText has changed?

I'm not sure who and when calls GetRenderedText but we must be notified, perhaps with some delay but notified. It's similar to how we fire a11y events, we wait for layout and then fire them: here we are sure layout will happen soon or later and doesn't require user input for that.

Comment 22

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

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #13)
> > > > Hmm, actually it would be more efficient to trigger the notification in
> > > > BuildTextRunsScanner::AssignTextRun.
> > > 
> > > When does it trigger?
> > 
> > Usually during layout --- first layout, and after text has changed --- but also
> > at other times.
> 
> We need perhaps a tricky thing. We need to distinguish things when it's first
> layout as a part of its container frame construction and when it's result of
> text changes. Text changes should result of a11y text change events. But we
> don't need to fire a11y text change events if rendered text appears if this
> text frame is somewhere inside of accessible subtree that was created when
> corresponding frame subree was constructed
> (nsCSSFrameConstructor::ContentRangeInserted). The same time we need to fire
> a11y text change events when we rendered text appears if
> nsCSSFrameConstructor::ContentRangeInserted was called for this text frame
> earlier.
> 
> Does it clear enough? Should I give more details?

It's not clear to me yet.

> I'm not sure who and when calls GetRenderedText but we must be notified,
> perhaps with some delay but notified. It's similar to how we fire a11y events,
> we wait for layout and then fire them: here we are sure layout will happen soon
> or later and doesn't require user input for that.

Accessibility calls GetRenderedText.

http://mxr.mozilla.org/mozilla-central/search?string=getrenderedtext

I understood this comment:
(In reply to comment #11)
> We had a chat with bz and the idea is to get notification from layout when text
> frame gains or looses rendered text. And when a11y handles these notifications
> then we create/destroy accessibles for text nodes and/or fire text change
> events.

But do you really want to get these notifications from layout for every text frame when a page loads? If not, when exactly do you want to get them?

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
 
> Accessibility calls GetRenderedText.
> 
> http://mxr.mozilla.org/mozilla-central/search?string=getrenderedtext

Right. But we do this at wrong time. We need to call it when text was rendered already.

> I understood this comment:
> (In reply to comment #11)
> > We had a chat with bz and the idea is to get notification from layout when text
> > frame gains or looses rendered text. And when a11y handles these notifications
> > then we create/destroy accessibles for text nodes and/or fire text change
> > events.
> 
> But do you really want to get these notifications from layout for every text
> frame when a page loads? If not, when exactly do you want to get them?

Not now, at least while we don't build complete accessible tree when new content appears, now we just create accessibles for top of new inserted content subtrees. But we lean towards to start accessible complete tree construction.

So
1) get notification about rendered text changes if it doesn't related with new content insertion or content removal.
2) if new content is inserted then
2.1) (while we don't create complete accessible tree) get notifications for text frames that aren't contained by any accessible that was inserted within the same subtree
2.2) (if we create complete accessible tree) get notifications for any text frame in subtree but we need to know if this frame is contained by any accessible inserted within the same subtree so that we can decide whether we need to fire a11y text change event or not.
3) We don't need notifications if content is removed (nsCSSFrameConstructor::ContentRemoved is enough for a11y)

The item 2) is more tricky because it might be hard to detect when this text frame that gets rendered text is contained by some accessible that was inserted as a part of frame subtree construction.

Alternative is if we collect ContentRangeInserted calls and process them after layout when we can get rendered text safely and when we can decide if this text frame requires an accessible or not. But we need to have a good way to know if collected info from ContentRangeInserted is still valid.

Comment 24

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

Item #1 is tricky too.

Note that changes to a textnode inside an element can affect the "rendered text" for textnodes inside other elements.

I'm not sure exactly what you're trying to solve or what the requirements are, but this seems hard.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> I'm not sure exactly what you're trying to solve or what the requirements are,

Basically we update accessible tree on nsCSSFrameConstructor::ContentRangeInserted. If we see text frame then we get rendered text to check if it's not empty (based on that we create on not create an accessible for this text frame). As well we fire a11y text change events that expose the rendered text to AT. Since we may not get right rendered text then we may create accessible for text nodes with empty rendered text what breaks screen reader in some way and on another hand we may fire a11y text change events with wrong information.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Item #1 is tricky too.

Doesn't first reflow frame flag help here (to filter content inserted notifications)? Does it trigger on content removal?

Comment 27

[Marco Zehe (:MarcoZ)]

Surkov, you asked me yesterday to test whehter the try-server build for bug 498015 fixes this bug, too. I don't find STR anywhere where I could verify this. How do I reproduce this bug? Do you or Ehsan have a testcase somewhere I could try?

Comment 28

[alexander :surkov (:asurkov)]

It should be mistake. We need to deal with this bug after bug 498015.

Comment 29

[alexander :surkov (:asurkov)]

Robert, when BuildTextRunsScanner::AssignTextRun triggers then it's not necessary text frame has old textrun even if it existed, for example, it looks like CharacterDataChanged clears text run and new text run will be generated somewhere later. Is that correct?

I need to compare old text and new text to report changes to AT. If I'm not guaranteed to get old text inside an AssignTextRun before AssignTextRun generates new text run then it sounds all I can is to cache rendered text on a11y side.

Comment 30

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

(In reply to comment #29)
> Robert, when BuildTextRunsScanner::AssignTextRun triggers then it's not
> necessary text frame has old textrun even if it existed, for example, it looks
> like CharacterDataChanged clears text run and new text run will be generated
> somewhere later. Is that correct?

Yes.
 
> I need to compare old text and new text to report changes to AT. If I'm not
> guaranteed to get old text inside an AssignTextRun before AssignTextRun
> generates new text run then it sounds all I can is to cache rendered text on
> a11y side.

Yes.

Comment 31

[alexander :surkov (:asurkov)]

Robert, can you give me an example multiflow text run when one text run is shared between different contents?

Comment 32

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

Hello <span>Kitty</span>

Comment 33

[alexander :surkov (:asurkov)]

How can I get rendered text from text run that corresponds to a content (it should be a content of mappedFlow->mStartFrame, outer loop in BuildTextRunsScanner::AssignTextRun).

Comment 34

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

I don't understand the question. What do you want to do that GetRenderedText doesn't already do?

Comment 35

[(no longer active)]

I found out this problem by code inspection.  I don't think that we have any actual evidence that this is a real problem in practice given how a11y clients interact with us, so I'm not sure if we need to block on this.

Comment 36

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

Jamie does NVDA currently trust our text remove events?

Comment 37

[James Teh [:Jamie]]

(In reply to comment #36)
> Jamie does NVDA currently trust our text remove events?
Define trust. In virtual buffers, we use textRemoved to notify us that a change has occurred and we re-render the node in question. We don't use it for anything else at this stage.

Comment 38

[alexander :surkov (:asurkov)]

fixed by bug 626660