Closed
Bug 391846
Opened 17 years ago
Closed 17 years ago
text-changed events incorrect for SHOW/HIDE events
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: aaronlev)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(3 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
sicking
:
superreview+
sicking
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
This bug, bug 390414 and bug 392203 all go together.
Surkov, I can work on them.
Summary: TEXT_INSERTED/REMOVED events incorrect for SHOW/HIDE events → text-changed events incorrect for SHOW/HIDE events
Comment 2•17 years ago
|
||
(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.
Assignee | ||
Comment 3•17 years ago
|
||
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
Assignee | ||
Comment 4•17 years ago
|
||
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
Blocks: 371279
Assignee | ||
Comment 5•17 years ago
|
||
Surkov/Ginn, if you want to look at the general approach I'm thinking about.
Comment 6•17 years ago
|
||
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•17 years ago
|
||
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?
Assignee | ||
Comment 8•17 years ago
|
||
Note: we need to make sure that the testcase in bug 392203 (phone #s shown on 411.com) works.
Assignee | ||
Comment 9•17 years ago
|
||
Filed offshoot bug 397297 for comment 6.
Filed offshoot bug 392796 for comment 7.
Both bugs have patches ready for review.
Comment 10•17 years ago
|
||
(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.
Assignee | ||
Comment 11•17 years ago
|
||
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().
Attachment #277127 -
Attachment is obsolete: true
Attachment #277482 -
Flags: review?(surkov.alexander)
Comment 12•17 years ago
|
||
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?
Assignee | ||
Comment 13•17 years ago
|
||
Surkov, I didn't understand the question. When could this happen?
Comment 14•17 years ago
|
||
(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•17 years ago
|
||
(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?
Assignee | ||
Comment 16•17 years ago
|
||
> 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•17 years ago
|
||
(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().
Assignee | ||
Comment 18•17 years ago
|
||
> 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.
Assignee | ||
Updated•17 years ago
|
Attachment #277528 -
Attachment is patch: false
Attachment #277528 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 19•17 years ago
|
||
You're right, we get dom node change notifications not text changes. Good catch.
Assignee | ||
Comment 20•17 years ago
|
||
Comment on attachment 277482 [details] [diff] [review]
Patch described below
Need to handle text nodes that are appended or removed.
Attachment #277482 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 21•17 years ago
|
||
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?
Attachment #277734 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 22•17 years ago
|
||
Attachment #277482 -
Attachment is obsolete: true
Attachment #277734 -
Attachment is obsolete: true
Attachment #277734 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 23•17 years ago
|
||
Bug 392987 is where we'll fix redundant events when a range of text is deleted.
Attachment #278007 -
Attachment is obsolete: true
Attachment #278177 -
Flags: review?(surkov.alexander)
Comment 24•17 years ago
|
||
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•17 years ago
|
||
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
Attachment #278177 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 26•17 years ago
|
||
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.
Attachment #278177 -
Attachment is obsolete: true
Assignee | ||
Comment 27•17 years ago
|
||
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).
Attachment #278356 -
Flags: superreview?(jonas)
Comment 28•17 years ago
|
||
(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.
Assignee | ||
Comment 29•17 years ago
|
||
Thanks, I'll fix the comment.
Why can't this be done using normal nsIMutationObserver events?
Assignee | ||
Comment 31•17 years ago
|
||
Basically because the frame is already destroyed by then. We need it.
Comment on attachment 278356 [details] [diff] [review]
Address Surkov's comments
ok. I hate that we have so many notification mechanisms though.
Attachment #278356 -
Flags: superreview?(jonas) → superreview+
Assignee | ||
Updated•17 years ago
|
Attachment #278356 -
Flags: approval1.9?
Comment on attachment 278356 [details] [diff] [review]
Address Surkov's comments
Oh, but please remove the nsNodeUtils.cpp change
Attachment #278356 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 34•17 years ago
|
||
Fixed.
Surkov, you will probably have some patch merge work to do for your text change event fix.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•