[AccessFu] Display text editing changes

VERIFIED FIXED in mozilla15

Status

()

Core
Disability Access APIs
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Provide feedback for text editing.
(Assignee)

Comment 1

5 years ago
Created attachment 619194 [details] [diff] [review]
Add AccessFu text editing feedback.
Attachment #619194 - Flags: review?(surkov.alexander)
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

5 years ago
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
Attachment #619194 - Flags: review?(surkov.alexander) → review?(trev.saunders)
(Assignee)

Comment 4

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

Comment 5

5 years ago
Created attachment 619680 [details] [diff] [review]
Add AccessFu text editing feedback.
Attachment #619194 - Attachment is obsolete: true
Attachment #619194 - Flags: review?(trev.saunders)
Attachment #619680 - Flags: review?(trev.saunders)
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?
Attachment #619680 - Flags: review?(trev.saunders) → review+
> 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.
(Assignee)

Comment 8

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

Comment 10

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

Comment 11

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b918ae1dd5ef
Assignee: nobody → eitan
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b918ae1dd5ef
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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.
Verified fixed in Fennec/15.0a1 2012-05-21
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.