391846 - text-changed events incorrect for SHOW/HIDE events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

See also bug 390414.

Two problems:
1) We're not firing text_inserted/remove when accessibles are shown/hidden because of style changes. This was an error on my part, for not realizing we needed that. Right now we only do it for content_inserted
2) There are 2 scenarios
a) ABC?GHI (where ? is an embedded object char)
     \_DEF   (needed separate object, e.g. a link or label)
   If the "DEF" node is removed or inserted there should be an event for the single embedded object char in the parent text
b) abcDEFghi (where DEF is from a separate node, but does is exposed in the parent text itself as an attribute rnage).
   If the "DEF" node is removed or inserted ther eshould be event for the 3 characters in the hypertext

Right now we never get it right.

Comment 1

[Aaron Leventhal]

This bug, bug 390414 and bug 392203 all go together.

Surkov, I can work on them.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> This bug, bug 390414 and bug 392203 all go together.
> 
> Surkov, I can work on them.
> 

I don't see how they are related with bug 390414 because if I get right it suppose to add CharacterDataWillChange() method only. Though I didn't start to work yet. So please feel free to go ahead.

Comment 3

[Aaron Leventhal]

The way we calculate the offset and length of a text change is not exactly right:
- We need to use rendered length
- There are some situations where a node w/o an accessible could be removed, but the subtree has accessible objects which reflect in the hypertext container
- We need to figure this out for any kind of node removal or addition, including style changes that change their visibility
- When nodes are inserted in the doc, the event needs to be calculated late, after the frames have been created, otherwise we won't know the text length
- Finally (i may file a follow-up bug for it), we should aggregate text change events and remove duplicates, so if we get a text removal from offset 3 to 5, and one from offset 5 to 9 at the same time

Comment 4

[Aaron Leventhal]

The way we calculate the offset and length of a text change is not exactly right:
- We need to use rendered length
- There are some situations where a node w/o an accessible could be removed, but the subtree has accessible objects which reflect in the hypertext container
- We need to figure this out for any kind of node removal or addition, including style changes that change their visibility
- When nodes are inserted in the doc, the event needs to be calculated late, after the frames have been created, otherwise we won't know the text length
- Finally (i may file a follow-up bug for it), we should aggregate text change events and remove duplicates, so if we get a text removal from offset 3 to 5, and one from offset 5 to 9 at the same time

Comment 5

[Aaron Leventhal]

Attachment: Work in progress

Surkov/Ginn, if you want to look at the general approach I'm thinking about.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 277127 [details] [diff] [review]
Work in progress


> 
>   nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>-  while (content) {
>-    nsIFrame* frame = shell->GetPrimaryFrameFor(content);
>-    if (frame) {
>-      return frame;
>-    }
>-    nsCOMPtr<nsIContent> tempContent = content->GetParent();
>-    content = tempContent;
>-  }
>-
>-  return nsnull;
>+  return shell->GetPrimaryFrameFor(content);
> }

I would suggested to put it in separate bug to see a possible regressions.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 277127 [details] [diff] [review]
Work in progress


>   static PRBool IsCorrectFrameType(nsIFrame* aFrame, nsIAtom* aAtom);
>   static PRUint32 State(nsIAccessible *aAcc) { PRUint32 state; aAcc->GetFinalState(&state, nsnull); return state; }
>-  static PRUint32 Role(nsIAccessible *aAcc) { PRUint32 role; aAcc->GetFinalRole(&role); return role; }
>+  static PRUint32 Role(nsIAccessible *aAcc) { PRUint32 role = nsIAccessibleRole::ROLE_NOTHING; aAcc->GetFinalRole(&role); return role; }

I guess it's better to initialize role inside GetFinalRole() because we should have unique behavior of role related methods, no?

Comment 8

[Aaron Leventhal]

Note: we need to make sure that the testcase in bug 392203 (phone #s shown on 411.com) works.

Comment 9

[Aaron Leventhal]

Filed offshoot bug 397297 for comment 6.
Filed offshoot bug 392796 for comment 7.
Both bugs have patches ready for review.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Filed offshoot bug 397297 for comment 6.
> Filed offshoot bug 392796 for comment 7.
> Both bugs have patches ready for review.
> 

Ok, thank you. I'll try to look today one more time at your patch.

Comment 11

[Aaron Leventhal]

Attachment: Patch described below

1. Do not fire text change event for show/hide of a text node. Use CharacterDataModified etc. for that.
2. Build CreateTextChangeEventForNode()
3. Call CreateTextChangeEventForNode() immediately for removals
4. Call CreateTextChangeEventForNode() after a delay for insertions (wait until frames and thus accessible objects are updated). 
5. Change HyperText text getters so they can deal with embedded objects or <br>'s with no frame, which is unfortunately the case for our deleted nodes.
6. Change DOMPointToHypertextOffset() so it can take a magic node offset of -1 which means, just use aNode, don't grab a child.

Also, I have filed bug 392897 to deal with trying to simplify the events we fire, including removing the extra events from when the editor calls JoinNodes() and SplitNodes().

Comment 12

[alexander :surkov (:asurkov)]

3. Call CreateTextChangeEventForNode() immediately for removals
4. Call CreateTextChangeEventForNode() after a delay for insertions (wait until
frames and thus accessible objects are updated). 

I didn't look at patch yet but might it be possible that you fire delete event before delayed insert for the same node when that node was inserted and removed at once?

Comment 13

[Aaron Leventhal]

Surkov, I didn't understand the question. When could this happen?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Surkov, I didn't understand the question. When could this happen?
> 

What's event order will I get?

var img = document.createElement("img");
document.documentElement.appendChild(img);
document.documentElement.removeChild(img);

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Created an attachment (id=277482) [details]
> Patch described below
> 
> 1. Do not fire text change event for show/hide of a text node. Use
> CharacterDataModified etc. for that.

CharacterDataModified won't be called for show/hide of nodes, right?

Comment 16

[Aaron Leventhal]

> CharacterDataModified won't be called for show/hide of nodes, right?

There are 2 cases:
* the text node is deleted/inserted -- CharacterDataModified will be called
* style changes: that will happen on the parent element of the DOM node

I don't think you can cause a style change that only affects the text frame. It needs to affect the parent element of the text frame.

So we should be okay.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> > CharacterDataModified won't be called for show/hide of nodes, right?
> 
> There are 2 cases:
> * the text node is deleted/inserted -- CharacterDataModified will be called

Are you 100% sure? I always thought in this case I should get ContentInserted/ContentAppended().

Comment 18

[Aaron Leventhal]

Attachment: Insert and remove image quickly

> var img = document.createElement("img");
> document.documentElement.appendChild(img);
> document.documentElement.removeChild(img);

In this case only a HIDE event is fired for the ROLE_GRAPHIC. No text change events or show event are fired. I think that's probably okay.

Most likely they will all be fired on a delay at some point so that we can do a better job intelligently collating events.

Comment 19

[Aaron Leventhal]

Attachment: Testcase for dynamically inserting and removing text node

You're right, we get dom node change notifications not text changes. Good catch.

Comment 20

[Aaron Leventhal]

Comment on attachment 277482 [details] [diff] [review]
Patch described below

Need to handle text nodes that are appended or removed.

Comment 21

[Aaron Leventhal]

Attachment: Invalidate before frames are destroyed

Surkov, I couldn't get it to actually work with the test cases. For events I keep getting this:
TEXT_REMOVED	 Name=[Error getting string:80070057 - The parameter is incorrect.] Role=[Error getting role:80070057 - The parameter is incorrect.] State=[Error getting state:80070057 - The parameter is incorrect.] Attribs@@= OldText=

I'll be gone for the next week, but will be trying to check emails. If you have a chance, maybe you can take a look?

Comment 22

[Aaron Leventhal]

Attachment: Getting closer -- remaining issue is that in the remove text node test case, 2 removals in a row doesn't work

Comment 23

[Aaron Leventhal]

Attachment: Works, ready for review

Bug 392987 is where we'll fix redundant events when a range of text is deleted.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 278177 [details] [diff] [review]
Works, ready for review

>+  /**
>+   * The inserted or removed text
>+   */
>+  readonly attribute DOMString modifiedText; 

nit: extra space at the end.

>+  NS_ENSURE_TRUE(aShell, NS_ERROR_FAILURE);

it's an argument, why not NS_ENSURE_ARG?

>+             aAccessible, nsnull, aIsAsynch),
>   mStart(aStart), mLength(aLength), mIsInserted(aIsInserted)
> {
>+  nsCOMPtr<nsIAccessibleText> textAccessible = do_QueryInterface(aAccessible);
>+  if (textAccessible) {
>+    textAccessible->GetText(aStart, aStart + aLength, mModifiedText);
>+  }

text changed event should be fired for text accessible only, istn't it? Should you have rather assertion instead 'if' clause?

I'll continue review tomorrow.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 278177 [details] [diff] [review]
Works, ready for review


>+  else {
>+    if (changeAccessible != aAccessibleForNode) {
>+      NS_WARNING("Hypertext is reporting a different accessible for this node");

Should it be an assertion instead?


>+      nsCOMPtr<nsIDOMNode> newNode;
>+      accessibleEvent->GetDOMNode(getter_AddRefs(newNode));
>+      if (newNode && newNode != mDOMNode) {

'newNode' name confuses me, possible just DOMNode or node?

>+          // FireDelayedAccessibleEvent(textChangeEvent, eRemoveDupes, PR_TRUE);

Why this comment?

>+     * Create a text change event for a changed node
>+     * @param aContainerAccessible, the first accessible in the container
>+     * @param aChangeNode, the node that is being inserted or removed, or shown/hidden
>+     * @param aAccessibleForNode, the accessible for that node, or nsnull if none exists

Please rename arguments 'aChangeNode' and 'aAccessibleForNode' to see they are related.

> NS_IMETHODIMP
> nsTextAccessible::AppendTextTo(nsAString& aText, PRUint32 aStartOffset, PRUint32 aLength)
> {
>   nsIFrame *frame = GetFrame();
>-  NS_ENSURE_TRUE(frame, NS_ERROR_FAILURE);
>-
>-  return frame->GetRenderedText(&aText, nsnull, nsnull, aStartOffset, aLength);
>+  return frame ? frame->GetRenderedText(&aText, nsnull, nsnull, aStartOffset, aLength) : NS_OK;
> }

The method do not fail anymore? Is it ok if text accessible hasn't frame?

the same is for nsHTMLTextAccessible.

r=me with answered/fixed

Comment 26

[Aaron Leventhal]

Attachment: Address Surkov's comments

Surkov, I removed the change to nsTextAccessible::AppendTextTo()  -- you're right we don't need that.

But I kept the change nsHTMLTextAccessible:GetRole() so it succeeds even if there is no frame. That way if the event listener for the delayed EVENT_HIDE asks for the role it can still get it.

Comment 27

[Aaron Leventhal]

Comment on attachment 278356 [details] [diff] [review]
Address Surkov's comments

Only need sr= for nsGenericElement.cpp part.

(The nsNodeUtils whitespace change is a mistake I won't check that in).

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> But I kept the change nsHTMLTextAccessible:GetRole() so it succeeds even if
> there is no frame. That way if the event listener for the delayed EVENT_HIDE
> asks for the role it can still get it.
> 

It's good issue. Do we want to fail if accessible is out of date? Ususally we always fail. For now I would suggested to add a comment inside GetRole() that we don't fail here wittingly.

Comment 29

[Aaron Leventhal]

Thanks, I'll fix the comment.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why can't this be done using normal nsIMutationObserver events?

Comment 31

[Aaron Leventhal]

Basically because the frame is already destroyed by then. We need it.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 278356 [details] [diff] [review]
Address Surkov's comments

ok. I hate that we have so many notification mechanisms though.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 278356 [details] [diff] [review]
Address Surkov's comments

Oh, but please remove the nsNodeUtils.cpp change

Comment 34

[Aaron Leventhal]

Fixed.

Surkov, you will probably have some patch merge work to do for your text change event fix.