The system suffix is for system generated events only

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Joanmarie Diggs, Assigned: capella)

Tracking

(Blocks: 2 bugs, {access})

6 Branch
mozilla14
x86_64
Linux
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 560397 [details]
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]
(Reporter)

Comment 1

6 years ago
Created attachment 560398 [details]
test case
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

6 years ago
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?

Updated

6 years ago
Blocks: 472809
(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.
(Reporter)

Comment 5

6 years ago
(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

6 years ago
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

6 years ago
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

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

Comment 9

6 years ago
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.

Updated

6 years ago
(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.
(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

6 years ago
Trevor, can you check if it's true as well for node creation/deletion when editing? It shouldn't be iirc.
(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?
Created attachment 590175 [details] [diff] [review]
work around

this isn't the right fix, but it should make users lives much better.
Attachment #590175 - Flags: review?(surkov.alexander)

Comment 15

6 years ago
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.
(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

6 years ago
(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.
Created attachment 591994 [details]
ritch text editing test

it appears the new patch makes text changes in this text case not be system events.
Created attachment 591995 [details] [diff] [review]
patch

seems something was wrong with my build before, I went to investigate why the new approach was failing and it worked.
Attachment #590175 - Attachment is obsolete: true
Attachment #590175 - Flags: review?(surkov.alexander)
Attachment #591995 - Flags: review?(surkov.alexander)

Comment 20

6 years ago
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
Attachment #591995 - Flags: review?(surkov.alexander)
Created attachment 592850 [details] [diff] [review]
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.
Attachment #591995 - Attachment is obsolete: true
Attachment #592850 - Flags: review?(surkov.alexander)

Comment 22

6 years ago
Comment on attachment 592850 [details] [diff] [review]
patch

canceling review per irc discussion (waiting for new patch)
Attachment #592850 - Flags: review?(surkov.alexander)
Created attachment 592978 [details] [diff] [review]
wip tests

these tests are hopefully getting closer but I can't finish them tonight.
Attachment #592850 - Attachment is obsolete: true

Comment 24

6 years ago
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
Created attachment 592980 [details] [diff] [review]
wip

add the test file this time
Attachment #592978 - Attachment is obsolete: true

Comment 26

6 years ago
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.
(Assignee)

Comment 27

5 years ago
I've (IRQ) volunteered to finish this for Trevor.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 28

5 years ago
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...


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.
(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

5 years ago
(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."
(Assignee)

Comment 31

5 years ago
Created attachment 607911 [details] [diff] [review]
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
Attachment #607419 - Attachment is obsolete: true
Attachment #607911 - Flags: feedback?(surkov.alexander)

Updated

5 years ago
Attachment #607911 - Attachment is patch: true
(Assignee)

Comment 32

5 years ago
Created attachment 607979 [details] [diff] [review]
Re-worked patch (v3)

This expands the testing a little ... now includes HTML input and editable Text Nodes
Attachment #607911 - Attachment is obsolete: true
Attachment #607911 - Flags: feedback?(surkov.alexander)

Comment 33

5 years ago
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
(Assignee)

Comment 34

5 years ago
Created attachment 608260 [details] [diff] [review]
Re-worked patch (v4)

Nice clean-up Alex ... ! All items addressed, and it still works :)
Attachment #607979 - Attachment is obsolete: true
Attachment #608260 - Flags: feedback?(surkov.alexander)

Updated

5 years ago
Attachment #608260 - Attachment is patch: true

Comment 35

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

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

looks good, thank you
Attachment #608260 - Flags: feedback?(surkov.alexander) → feedback+

Comment 36

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

changing to r+
Attachment #608260 - Flags: feedback+ → review+

Comment 37

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5335ee086a50
https://hg.mozilla.org/mozilla-central/rev/5335ee086a50
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.