Last Comment Bug 749812 - [AccessFu] Display text editing changes
: [AccessFu] Display text editing changes
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-27 15:21 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-22 02:11 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add AccessFu text editing feedback. (3.57 KB, patch)
2012-04-27 15:22 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Add AccessFu text editing feedback. (3.74 KB, patch)
2012-04-30 14:08 PDT, Eitan Isaacson [:eeejay]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-04-27 15:21:12 PDT
Provide feedback for text editing.
Comment 1 Eitan Isaacson [:eeejay] 2012-04-27 15:22:44 PDT
Created attachment 619194 [details] [diff] [review]
Add AccessFu text editing feedback.
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-27 22:45:48 PDT
Comment on attachment 619194 [details] [diff] [review]
Add AccessFu text editing feedback.

># HG changeset patch
># User Eitan Isaacson <eitan@monotonous.org>
>
>Add AccessFu text editing feedback.
>
>diff --git a/accessible/src/jsat/AccessFu.jsm b/accessible/src/jsat/AccessFu.jsm
>index 795ad40..0ecf9a7 100644
>--- a/accessible/src/jsat/AccessFu.jsm
>+++ b/accessible/src/jsat/AccessFu.jsm
>@@ -252,16 +252,36 @@ var AccessFu = {
>       case Ci.nsIAccessibleEvent.EVENT_FOCUS:
>       {
>         if (this.isBrowserDoc(aEvent.DOMNode)) {
>           // The document recieved focus, call tabSelected to present current tab.
>           this.presenters.forEach(
>             function(p) {p.tabSelected(aEvent.accessible);});
>         }
>       }
>+      break;

nit, I guess we have some c++ that uses this style, but putting the break outside the block always feels funny to me.

>+      case Ci.nsIAccessibleEvent.EVENT_TEXT_INSERTED:
>+      case Ci.nsIAccessibleEvent.EVENT_TEXT_REMOVED:
>+      {
>+        if (aEvent.isFromUserInput) {
>+          // TODO support live regions as well.

I'm not sure what you mean by that, I believe currently you'll fire an event regrdless if the source is a live object or typing or pasting.

>+          let event = aEvent.QueryInterface(Ci.nsIAccessibleTextChangeEvent);
>+          let isInserted = event.isInserted();
>+          let textIface = aEvent.accessible.QueryInterface(Ci.nsIAccessibleText);
>+          // Bug 749810 doesn't allow us to simply do getText and get an empty string.
>+          let charCount = textIface.characterCount;
>+          let text = (charCount) ? textIface.getText(0, charCount) : '';

perhaps try / catch?

>+  if (aIsInserted) {
>+    androidEvent.addedCount = aLength;
>+    androidEvent.beforeText =
>+      aText.substring(0, aStart) + aText.substring(aStart + aLength);

I wonder if there is a race here if you do two two text changes very quickly where the first is an insert.  I sort of think not because either they will take place in the same reflow window in which we'll fire only one event, or they'll take place in different reflow windows in which case I think this code will run before the second reflow but I'm not sure of that off hand.

I think using aModifiedText would solve this issue if it in factually exists.

>+  } else {
>+    androidEvent.removedCount = aLength;
>+    androidEvent.beforeText =
>+      aText.substring(0, aStart) + aModifiedText + aText.substring(aStart);

so, what happens in the cse of replacement where we fire a delete and insert something liike
myElm.value = "xyz";
myElm.value = "xabz";

I think you'll say that y was deleted from the string xyabz.

btw what about text changes where the text contains eocs? I'd thik we probably either do or should fire text changes when you insert html which could clearly have the effect of adding a link which will get an EOC.
Comment 3 alexander :surkov 2012-04-30 03:43:48 PDT
Comment on attachment 619194 [details] [diff] [review]
Add AccessFu text editing feedback.

redirecting the request to Trevor since he started to look at the patch
Comment 4 Eitan Isaacson [:eeejay] 2012-04-30 13:57:23 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 619194 [details] [diff] [review]
> Add AccessFu text editing feedback.
> 
> ># HG changeset patch
> ># User Eitan Isaacson <eitan@monotonous.org>
> >
> >Add AccessFu text editing feedback.
> >
> >diff --git a/accessible/src/jsat/AccessFu.jsm b/accessible/src/jsat/AccessFu.jsm
> >index 795ad40..0ecf9a7 100644
> >--- a/accessible/src/jsat/AccessFu.jsm
> >+++ b/accessible/src/jsat/AccessFu.jsm
> >@@ -252,16 +252,36 @@ var AccessFu = {
> >       case Ci.nsIAccessibleEvent.EVENT_FOCUS:
> >       {
> >         if (this.isBrowserDoc(aEvent.DOMNode)) {
> >           // The document recieved focus, call tabSelected to present current tab.
> >           this.presenters.forEach(
> >             function(p) {p.tabSelected(aEvent.accessible);});
> >         }
> >       }
> >+      break;
> 
> nit, I guess we have some c++ that uses this style, but putting the break
> outside the block always feels funny to me.
> 

I agree. That was an oversight. Fixed.

> >+      case Ci.nsIAccessibleEvent.EVENT_TEXT_INSERTED:
> >+      case Ci.nsIAccessibleEvent.EVENT_TEXT_REMOVED:
> >+      {
> >+        if (aEvent.isFromUserInput) {
> >+          // TODO support live regions as well.
> 
> I'm not sure what you mean by that, I believe currently you'll fire an event
> regrdless if the source is a live object or typing or pasting.
> 

AFAIK live region events would still have isFromUserInput as false. So the TODO there is
a reminder to handle events with isFromUserInput being false from live regions.

> >+          let event = aEvent.QueryInterface(Ci.nsIAccessibleTextChangeEvent);
> >+          let isInserted = event.isInserted();
> >+          let textIface = aEvent.accessible.QueryInterface(Ci.nsIAccessibleText);
> >+          // Bug 749810 doesn't allow us to simply do getText and get an empty string.
> >+          let charCount = textIface.characterCount;
> >+          let text = (charCount) ? textIface.getText(0, charCount) : '';
> 
> perhaps try / catch?

Technically, you could not be certain that an exception is thrown because of a zero length string. And if it is not, you will get a lot of wacky calculations later on that would break things, and it would be harder to debug and discover what went wrong. So I will check the length in the catch block to be certain.

> 
> >+  if (aIsInserted) {
> >+    androidEvent.addedCount = aLength;
> >+    androidEvent.beforeText =
> >+      aText.substring(0, aStart) + aText.substring(aStart + aLength);
> 
> I wonder if there is a race here if you do two two text changes very quickly
> where the first is an insert.  I sort of think not because either they will
> take place in the same reflow window in which we'll fire only one event, or
> they'll take place in different reflow windows in which case I think this
> code will run before the second reflow but I'm not sure of that off hand.
> 
> I think using aModifiedText would solve this issue if it in factually exists.
> 

As we discussed on IRC. The modified text field does not really help here.

> >+  } else {
> >+    androidEvent.removedCount = aLength;
> >+    androidEvent.beforeText =
> >+      aText.substring(0, aStart) + aModifiedText + aText.substring(aStart);
> 
> so, what happens in the cse of replacement where we fire a delete and insert
> something liike
> myElm.value = "xyz";
> myElm.value = "xabz";
> 
> I think you'll say that y was deleted from the string xyabz.
> 

This is just for user edits. Not all DOM mutations. At this point it is for text entered manually on a mobile device. So besides copy/paste this is a slowish sequence of one character inserts or deletes.

I realize this isn't perfect, and that the text we retrieve from the object might be the result of edit events that we have not processed yet. But for simple usage I think it would do for now. I would like to have this land and see where the stress-points are and fix it there.

> btw what about text changes where the text contains eocs? I'd thik we
> probably either do or should fire text changes when you insert html which
> could clearly have the effect of adding a link which will get an EOC.

That is a good question for rich text editing. Besides flattening the whole deal, I don't know what is best. I think a future patch might handle that. I opened bug 750479 for that.
Comment 5 Eitan Isaacson [:eeejay] 2012-04-30 14:08:33 PDT
Created attachment 619680 [details] [diff] [review]
Add AccessFu text editing feedback.
Comment 6 Trevor Saunders (:tbsaunde) 2012-04-30 23:47:17 PDT
Comment on attachment 619680 [details] [diff] [review]
Add AccessFu text editing feedback.

>+          } catch (x) {
>+            // Because of bug #749810, we might have gotten an exception
>+            // with of a zero-length text. If we did, ignore it.

nit, // XXX blah blah is the usual style for this sort of comment

>+            if (textIface.characterCount)
>+              throw x;

can't you check what the nsresult error code was too?
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-30 23:50:41 PDT
> Technically, you could not be certain that an exception is thrown because of
> a zero length string. And if it is not, you will get a lot of wacky
> calculations later on that would break things, and it would be harder to
> debug and discover what went wrong. So I will check the length in the catch
> block to be certain.

yeah, I was assuming you could only catch exceptions where the error code was invalid arg.

> > >+  if (aIsInserted) {
> > >+    androidEvent.addedCount = aLength;
> > >+    androidEvent.beforeText =
> > >+      aText.substring(0, aStart) + aText.substring(aStart + aLength);
> > 
> > I wonder if there is a race here if you do two two text changes very quickly
> > where the first is an insert.  I sort of think not because either they will
> > take place in the same reflow window in which we'll fire only one event, or
> > they'll take place in different reflow windows in which case I think this
> > code will run before the second reflow but I'm not sure of that off hand.
> > 
> > I think using aModifiedText would solve this issue if it in factually exists.
> > 
> 
> As we discussed on IRC. The modified text field does not really help here.
> 
> > >+  } else {
> > >+    androidEvent.removedCount = aLength;
> > >+    androidEvent.beforeText =
> > >+      aText.substring(0, aStart) + aModifiedText + aText.substring(aStart);
> > 
> > so, what happens in the cse of replacement where we fire a delete and insert
> > something liike
> > myElm.value = "xyz";
> > myElm.value = "xabz";
> > 
> > I think you'll say that y was deleted from the string xyabz.
> > 
> 
> This is just for user edits. Not all DOM mutations. At this point it is for
> text entered manually on a mobile device. So besides copy/paste this is a
> slowish sequence of one character inserts or deletes.

ok, it makes me really unhappy, but since I can't think of a good way to fix these issues and I don't think this makes things worse or harder to fix its ok.
Comment 8 Eitan Isaacson [:eeejay] 2012-05-01 01:49:20 PDT
Thanks!

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > Technically, you could not be certain that an exception is thrown because of
> > a zero length string. And if it is not, you will get a lot of wacky
> > calculations later on that would break things, and it would be harder to
> > debug and discover what went wrong. So I will check the length in the catch
> > block to be certain.
> 
> yeah, I was assuming you could only catch exceptions where the error code
> was invalid arg.
> 

The GetText method seems to return the same code on many conditions.

> > > >+  } else {
> > > >+    androidEvent.removedCount = aLength;
> > > >+    androidEvent.beforeText =
> > > >+      aText.substring(0, aStart) + aModifiedText + aText.substring(aStart);
> > > 
> > > so, what happens in the cse of replacement where we fire a delete and insert
> > > something liike
> > > myElm.value = "xyz";
> > > myElm.value = "xabz";
> > > 
> > > I think you'll say that y was deleted from the string xyabz.
> > > 
> > 
> > This is just for user edits. Not all DOM mutations. At this point it is for
> > text entered manually on a mobile device. So besides copy/paste this is a
> > slowish sequence of one character inserts or deletes.
> 
> ok, it makes me really unhappy, but since I can't think of a good way to fix
> these issues and I don't think this makes things worse or harder to fix its
> ok.

Agreed. I'll put in a comment about that in the code. But also, since we check the IsFromUserInput attribute on the event object, we should not run in to the case above where the edit happens via js, right?
Comment 9 Trevor Saunders (:tbsaunde) 2012-05-01 02:09:06 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #8)
> Thanks!
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > Technically, you could not be certain that an exception is thrown because of
> > > a zero length string. And if it is not, you will get a lot of wacky
> > > calculations later on that would break things, and it would be harder to
> > > debug and discover what went wrong. So I will check the length in the catch
> > > block to be certain.
> > 
> > yeah, I was assuming you could only catch exceptions where the error code
> > was invalid arg.
> > 
> 
> The GetText method seems to return the same code on many conditions.

I'd assume it returns NS_ERROR_INVALID_ARG for other sorts of invalid arguments, and I don't know when else, I'm not terribly familiar with that code and its kind of confusing.

> > > > >+  } else {
> > > > >+    androidEvent.removedCount = aLength;
> > > > >+    androidEvent.beforeText =
> > > > >+      aText.substring(0, aStart) + aModifiedText + aText.substring(aStart);
> > > > 
> > > > so, what happens in the cse of replacement where we fire a delete and insert
> > > > something liike
> > > > myElm.value = "xyz";
> > > > myElm.value = "xabz";
> > > > 
> > > > I think you'll say that y was deleted from the string xyabz.
> > > > 
> > > 
> > > This is just for user edits. Not all DOM mutations. At this point it is for
> > > text entered manually on a mobile device. So besides copy/paste this is a
> > > slowish sequence of one character inserts or deletes.
> > 
> > ok, it makes me really unhappy, but since I can't think of a good way to fix
> > these issues and I don't think this makes things worse or harder to fix its
> > ok.
> 
> Agreed. I'll put in a comment about that in the code. But also, since we
> check the IsFromUserInput attribute on the event object, we should not run
> in to the case above where the edit happens via js, right?

well, if someday we really make isFromUserInput correct for text change events, see bug 686909 for details on this issue.
Comment 10 Eitan Isaacson [:eeejay] 2012-05-02 13:21:10 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> Comment on attachment 619680 [details] [diff] [review]
> >+            if (textIface.characterCount)
> >+              throw x;
> 
> can't you check what the nsresult error code was too?

I could, but it would be of little use. I see two places where the returned nsresult is NS_ERROR_INVALID_ARG, and they conditionally return that result on the opaque failure of two chained utility functions (-1 meaning error).

http://dxr.lanedo.com/mozilla-central/accessible/src/html/nsHyperTextAccessible.cpp.html#l450
Comment 12 Ed Morley [:emorley] 2012-05-04 02:30:12 PDT
https://hg.mozilla.org/mozilla-central/rev/b918ae1dd5ef
Comment 13 Trevor Saunders (:tbsaunde) 2012-05-07 02:24:00 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > Comment on attachment 619680 [details] [diff] [review]
> > >+            if (textIface.characterCount)
> > >+              throw x;
> > 
> > can't you check what the nsresult error code was too?
> 
> I could, but it would be of little use. I see two places where the returned
> nsresult is NS_ERROR_INVALID_ARG, and they conditionally return that result
> on the opaque failure of two chained utility functions (-1 meaning error).

iirc your talking about the functions that get the actual offsets to get text for.  But really I'd have just said fix the real bug here if I didn't think it'd make you want to kill me.
Comment 14 Marco Zehe (:MarcoZ) 2012-05-22 02:11:20 PDT
Verified fixed in Fennec/15.0a1 2012-05-21

Note You need to log in before you can comment on or make changes to this bug.