686909 - The system suffix is for system generated events only

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test script

Steps to reproduce:

1. Launch the attached test script in a terminal.
2. View the attached test page.
3. Type and use Backspace in the entry.

Expected results: The object:text-changed events would not have the 'system' suffix because the user is changing the text; the system (e.g. the web page itself, a live region, etc.) is not changing the text.

Actual results: The object:text-changed events have the 'system' prefix:

object:text-changed:insert:system(0, 1, t)
	source: [entry | ]
	host_application: [application | Firefox]
object:text-changed:insert:system(1, 1, h)
	source: [entry | ]
	host_application: [application | Firefox]
object:text-changed:insert:system(2, 1, i)
	source: [entry | ]
	host_application: [application | Firefox]
object:text-changed:insert:system(3, 1, s)
	source: [entry | ]
	host_application: [application | Firefox]
object:text-changed:delete:system(3, 1, )
	source: [entry | ]
	host_application: [application | Firefox]

Comment 1

[Joanmarie Diggs]

Attachment: test case

Comment 2

[Trevor Saunders (:tbsaunde)]

I haven't debugged this, but I think what is happening is that we are notified of the text change enqueue it as a text update we need to compute and allow the browser to go on its mary way.  Then when we go to generate the text change event and compute mIsFromUserInput we aren't actually in the event that caused the text change.  Which means that  our guess if  a text change is from user input is just nonsense.  I think we need to check where the event came from when we queue the new text for processing not when we generate the text change event.

Alex thoughts?

Comment 3

[alexander :surkov (:asurkov)]

Correct, isFromUserInput flag is valid for few cases only. Currently we ignore this information for mutation and text change processing because layout happens basically async. I realize we may have cases when layout allows to compute isFromUserInput correctly but we didn't care about this too much since I'm not aware of consumers.

All you should do is to check isFromUserInput value at nsDocAccessible ContentRangeInserted/ContentRemoved/UpdateText notifications. If there are cases where it's true (I assume editor may have since it's sync basically) then we can take it into account when processing these notifications.

Joanie, is this something that Orca relies on?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #3)
> Correct, isFromUserInput flag is valid for few cases only. Currently we
> ignore this information for mutation and text change processing because
> layout happens basically async. I realize we may have cases when layout
> allows to compute isFromUserInput correctly but we didn't care about this
> too much since I'm not aware of consumers.
> 
> All you should do is to check isFromUserInput value at nsDocAccessible
> ContentRangeInserted/ContentRemoved/UpdateText notifications. If there are
> cases where it's true (I assume editor may have since it's sync basically)
> then we can take it into account when processing these notifications.

ok, not sure I have the time to take this atm though :/

>> Joanie, is this something that Orca relies on?

I'd expect its used somewhere since it got noticed.  I seem to remeber from an old bug this was for telling user input for not user input such as js so that they could read clocks and other auto updating aria things differently.

Comment 5

[Joanmarie Diggs]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander surkov from comment #3)

 
> >> Joanie, is this something that Orca relies on?
> 
> I'd expect its used somewhere since it got noticed.  I seem to remeber from
> an old bug this was for telling user input for not user input such as js so
> that they could read clocks and other auto updating aria things differently.

Exactly.

Comment 6

[alexander :surkov (:asurkov)]

So we need to check whether text reflow for editing happens in sync. If not then we need to listen change events like DOM mutations (node insert, node remove, character data change) and put nodes into hash if the change is caused by user input. When event queue is processed then look into the hash and set isFromUserInput flag.

Comment 7

[Jason White]

The effect of this bug upon users is that characters typed into text fields are not echoed in speech or displayed in braille when Orca is used.
that is, the user can't read the text as it is being typed. This bug has been much discussed on the Orca mailing list.

Comment 8

[alexander :surkov (:asurkov)]

Thanks, Jason. A link to the mailing list would be great.

Comment 9

[Jason White]

The mailing list thread (started by me) is here:
https://mail.gnome.org/archives/orca-list/2011-October/msg00199.html
and eventually this bug is identified as the cause.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> So we need to check whether text reflow for editing happens in sync. If not
> then we need to listen change events like DOM mutations (node insert, node
> remove, character data change) and put nodes into hash if the change is
> caused by user input. When event queue is processed then look into the hash
> and set isFromUserInput flag.

If your suggesting adding a second hashmap  that feels a little heavy weight as well as possibly brittle, but I'm not sure if there's a better solution or what it might be.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to alexander :surkov from comment #6)
> > So we need to check whether text reflow for editing happens in sync. If not
> > then we need to listen change events like DOM mutations (node insert, node
> > remove, character data change) and put nodes into hash if the change is
> > caused by user input. When event queue is processed then look into the hash
> > and set isFromUserInput flag.
> 
> If your suggesting adding a second hashmap  that feels a little heavy weight
> as well as possibly brittle, but I'm not sure if there's a better solution
> or what it might be.


Since it appears the reflow is async from the editing it appears we need to do this or come up with another plan.

Comment 12

[alexander :surkov (:asurkov)]

Trevor, can you check if it's true as well for node creation/deletion when editing? It shouldn't be iirc.

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> Trevor, can you check if it's true as well for node creation/deletion when
> editing? It shouldn't be iirc.

Zi'M NOT SURE WHAT YOU MEAN,  cONTENTiNSERTED()?dO YOU MEAN THE cONTENTiNSERTED PROCCEdo you mean the ContentInsertion stuff?

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: work around

this isn't the right fix, but it should make users lives much better.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 590175 [details] [diff] [review]
work around

Review of attachment 590175 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +1365,5 @@
> +
> +    // XXX We should use IsFromUserInput here, but that isn't always correct
> +    // when the text change isn't related to content insertion or removal.
> +    bool isFromUserInput = GetAccessibleWrap(aObject)->State() &
> +        (states::FOCUSED | states::EDITABLE);

does states:FOCUSED do a trick for rich editing?

nit: wrong indent

Shouldn't we override AccEvent::IsFromUserInput instead? That'll be crossplatform solution.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #15)
> Comment on attachment 590175 [details] [diff] [review]
> work around
> 
> Review of attachment 590175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/atk/nsAccessibleWrap.cpp
> @@ +1365,5 @@
> > +
> > +    // XXX We should use IsFromUserInput here, but that isn't always correct
> > +    // when the text change isn't related to content insertion or removal.
> > +    bool isFromUserInput = GetAccessibleWrap(aObject)->State() &
> > +        (states::FOCUSED | states::EDITABLE);
> 
> does states:FOCUSED do a trick for rich editing?

I'm not sure off hand, ideas for testing?

> nit: wrong indent

I assume you mean the last line?  I'd have expected one tab in which currently is 4 spaces there, but I can make it only 2 if you like, or I guess reindent the whole thing.

> Shouldn't we override AccEvent::IsFromUserInput instead? That'll be
> crossplatform solution.

that'd mean making it virtual no?  I don't think we really want to do that.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #16)

> > does states:FOCUSED do a trick for rich editing?
> 
> I'm not sure off hand, ideas for testing?

<div contenteditable="true"><p>hello</p></div>

I don't think accessible for p element gets focused state.

> > nit: wrong indent
> 
> I assume you mean the last line?  I'd have expected one tab in which
> currently is 4 spaces there, but I can make it only 2 if you like, or I
> guess reindent the whole thing.

up to you which way to follow, just we shouldn't use 4 spaces indentation

> > Shouldn't we override AccEvent::IsFromUserInput instead? That'll be
> > crossplatform solution.
> 
> that'd mean making it virtual no?  I don't think we really want to do that.

yeah that's the one way. You could change the isFromUserInput flag in event constructor. I like to see crossplatform solution for consistence.

Comment 18

[Trevor Saunders (:tbsaunde)]

Attachment: ritch text editing test

it appears the new patch makes text changes in this text case not be system events.

Comment 19

[Trevor Saunders (:tbsaunde)]

Attachment: patch

seems something was wrong with my build before, I went to investigate why the new approach was failing and it worked.

Comment 20

[alexander :surkov (:asurkov)]

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

Review of attachment 591995 [details] [diff] [review]:
-----------------------------------------------------------------

you need mochitests, otherwise it looks good

::: accessible/src/base/AccEvent.cpp
@@ +259,5 @@
>  {
> +  // XXX We should use IsFromUserInput here, but that isn't always correct
> +  // when the text change isn't related to content insertion or removal.
> +  mIsFromUserInput = mAccessible->State() &
> +    (states::FOCUSED | states::EDITABLE);

comment about <div contenteditable="true"><p>hello</p></div> is not addressed

Comment 21

[Trevor Saunders (:tbsaunde)]

Attachment: patch

These tests are not comprehensive they don't cover the cases of adding text to editable nodes that are focused or not.  I'm not surre if we have a reasonable way to do that.  However they protect us from the worst case, and I'd like to get the fix in tomorrows merge since it appears to be a bad bug for users to deal with.

Comment 22

[alexander :surkov (:asurkov)]

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

canceling review per irc discussion (waiting for new patch)

Comment 23

[Trevor Saunders (:tbsaunde)]

Attachment: wip tests

these tests are hopefully getting closer but I can't finish them tonight.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 592978 [details] [diff] [review]
wip tests

Review of attachment 592978 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/events.js
@@ +1317,5 @@
>         "Text was " + changeInfo + " for " + prettyName(aID));
>      is(aEvent.modifiedText, modifiedText,
>         "Wrong " + changeInfo + " text for " + prettyName(aID));
> +    if (typeof aFromUser != "undefined")
> +      is(aEvent.isFromUserInput, aFromUser, "wrong value of isFromUserInput()");

it's better to add isFromUserInput field to checker object and do a check inside of eventQueue.checkEvent function.

Then you do
function textChangeChecker(aID, aStart, aEnd, aTextOrFunc, aIsInserted, aFromUser)
{
  this.target =
  this.type =
  this.isFromUserInput = aFromUser;
}

::: accessible/tests/mochitest/events/Makefile.in
@@ +78,5 @@
>  		test_focus_name.html \
>  		test_focus_selects.html \
>  		test_focus_tabbox.xul \
>  		test_focus_tree.xul \
> +		test_fromUserInput.html \

there's no this file in the patch

::: accessible/tests/mochitest/events/test_text.html
@@ +37,1 @@
>      }

some artifact I guess

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: wip

add the test file this time

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 592980 [details] [diff] [review]
wip

Review of attachment 592980 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/events.js
@@ +1296,5 @@
>  
>  /**
>   * Text inserted/removed events checker.
>   */
> +function textChangeChecker(aID, aStart, aEnd, aTextOrFunc, aIsInserted, aFromUser)

btw, I don't like boolean constants, can you use something like
const kNotFromUserInput = 0;
const kFromUserInput = 1;

::: accessible/tests/mochitest/events/test_fromUserInput.html
@@ +28,5 @@
> +    {
> +      this.DOMNode = getNode(aID);
> +      this.textNode = aTextNode ? aTextNode : this.DOMNode.firstChild;
> +      var atttr = aAttr ? aAttr : "data";
> +      this.textAttr = this.textNode.attr;

I'd say you invoker can detect how it should change a value so no need in these arguments

@@ +36,5 @@
> +      ];
> +
> +      this.invoke = function removeTextInvoker_invoke()
> +      {
> +        this.textNode.this.textAttr = "";

what's that?

@@ +56,5 @@
> +      ];
> +
> +      this.invoke = function insertTextInvoker_invoke()
> +      {
> +        this.textNode.data = aText;

aStart and aEnd aren't used here so you don't need them in arguments

@@ +77,5 @@
> +      gQueue = new eventQueue();
> +
> +      // insertion and removal from non-editable node
> +      gQueue.push(new removeTextInvoker("div", 0, 5, "hello", false));
> +      gQueue.push(new insertTextInvoker("div", 0, 5, "hello", false));

taking into account invokers don't do a real insert or remove then you can combine them into one changeText

@@ +98,5 @@
> +      gQueue.push(new removeTextInvoker("input", 0, 1, "v", true));
> +      gQueue.push(new insertTextInvoker("input", 0, 1, "v", true));
> +      gQueue.push(new synthFocus("editable"));
> +gQueue.push(new removeTextInvoker("editable", 0, 1, "v", true));
> +gQueue.push(new insertTextInvoker("editable", 0, 1, "v", true));

wrong indentation

at least you should add a comment that what you test here is wrong behaviour because DOM changes shouldn't result into isFromUserInput flag.

Comment 27

[Mark Capella [:capella]]

I've (IRQ) volunteered to finish this for Trevor.

Comment 28

[Mark Capella [:capella]]

Attachment: Re-worked patch (v1)

Ok, I created a version of the patch, addressed a few nits, (changed the WIP: "var atttr" to "var attr", etc.) ...

Trevor, the patch said WIP... this code:

   // insertion and removal from focused editable node
   gQueue.push(new synthFocus("input"));
   gQueue.push(new removeTextInvoker("input", 0, 1, "v", true));

   Fails with: failed | an unexpected uncaught JS exception reported through window.onerror - this.textNode is null at chrome://mochitests/content/a11y/accessible/events/test_fromUserInput.html:32

   did you drop code? or was this a known issue and I need to push ahead and fix...


Alex, can you briefly re-state comments concerning:

>  I'd say you invoker can detect how it should change a value so no need in these arguments
>  aStart and aEnd aren't used here so you don't need them in arguments
>  taking into account invokers don't do a real insert or remove then you can combine them into one changeText
>  at least you should add a comment that what you test here is wrong behaviour because DOM changes shouldn't result into isFromUserInput flag.

Comment 29

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #28)
> Created attachment 607419 [details] [diff] [review]
> Re-worked patch (v1)
> 
> Ok, I created a version of the patch, addressed a few nits, (changed the
> WIP: "var atttr" to "var attr", etc.) ...
> 
> Trevor, the patch said WIP... this code:
> 
>    // insertion and removal from focused editable node
>    gQueue.push(new synthFocus("input"));
>    gQueue.push(new removeTextInvoker("input", 0, 1, "v", true));
> 
>    Fails with: failed | an unexpected uncaught JS exception reported through
> window.onerror - this.textNode is null at
> chrome://mochitests/content/a11y/accessible/events/test_fromUserInput.html:32
> 
>    did you drop code? or was this a known issue and I need to push ahead and
> fix...

no idea, you'll need to debug :)

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #28)

> Alex, can you briefly re-state comments concerning:
> 
> >  I'd say you invoker can detect how it should change a value so no need in these arguments

i meant that invoker don't need to have aAttr argument. For example, if this is input node then use 'value' attribute, if this is text node then use 'data' property.

> >  aStart and aEnd aren't used here so you don't need them in arguments

ignore that, since they are used in textChangeChecker

> >  taking into account invokers don't do a real insert or remove then you can combine them into one changeText

well, you can have changeText and change the text like

node.data = aText

so removeText is equivalent to changeText with empty string, insertText is equivalent to changeText with new string.

> >  at least you should add a comment that what you test here is wrong behaviour because DOM changes shouldn't result into isFromUserInput flag.

add a comment like "isFromUserInput flag is set to true regardless these changes aren't made by the user. That's a bug."

Comment 31

[Mark Capella [:capella]]

Attachment: Re-worked patch (v2)

Ok, the last version of test_fromuserinput.html was too busy for me ... wasn't sure of the point of some of it ... this is stripped down ... modifys one char in the input field ... the patch and the test passes as is.

If the changes to AccEvent.cpp aren't applied, then the test fails. I wasn't involved in the original discussions so I don't know how much of the reported problem this addresses, and what is left that could be expanded upon.

Let me know -- mark

Comment 32

[Mark Capella [:capella]]

Attachment: Re-worked patch (v3)

This expands the testing a little ... now includes HTML input and editable Text Nodes

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 607979 [details] [diff] [review]
Re-worked patch (v3)

Review of attachment 607979 [details] [diff] [review]:
-----------------------------------------------------------------

otherwise looks good

::: accessible/tests/mochitest/events/Makefile.in
@@ +77,5 @@
>  		test_focus_name.html \
>  		test_focus_selects.html \
>  		test_focus_tabbox.xul \
>  		test_focus_tree.xul \
> +  	test_fromUserInput.html \

wrong indentation

::: accessible/tests/mochitest/events/test_fromUserInput.html
@@ +20,5 @@
> +
> +  <script type="application/javascript">
> +
> +    /**
> +     * Base invoker and checker

dot in the end

@@ +22,5 @@
> +
> +    /**
> +     * Base invoker and checker
> +     */
> +    function textRemoveInvoker(aID, aStart, aEnd, aText, aFromUser)

it doesn't sound like we really need a base class for these trivial code

@@ +32,5 @@
> +      ];
> +    }
> +
> +    /**
> +     * Remove text data from HTML input

dot in the end

@@ +38,5 @@
> +    function removeTextFromInput(aID, aStart, aEnd, aText, aFromUser)
> +    {
> +      this.__proto__ = new textRemoveInvoker(aID, aStart, aEnd, aText, aFromUser);
> +
> +      this.eventSeq.push(new invokerChecker(EVENT_VALUE_CHANGE, this.DOMNode));

why do you need to test value change event?

@@ +46,5 @@
> +        const nsIDOMNSEditableElement =
> +          Components.interfaces.nsIDOMNSEditableElement;
> +
> +        // this.DOMNode.focus();
> +        this.DOMNode.setSelectionRange(aStart, aEnd);

it makes sense to focus it here, doesn't it work properly?

@@ +61,5 @@
> +
> +    /**
> +     * Remove text data from text node
> +     */
> +    function removeTextFromNode(aID, aStart, aEnd, aText, aFromUser)

-> removeTextFromContentEditable

Comment 34

[Mark Capella [:capella]]

Attachment: Re-worked patch (v4)

Nice clean-up Alex ... ! All items addressed, and it still works :)

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 608260 [details] [diff] [review]
Re-worked patch (v4)

Review of attachment 608260 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thank you

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 608260 [details] [diff] [review]
Re-worked patch (v4)

changing to r+

Comment 37

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5335ee086a50

Comment 38

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/5335ee086a50