394493 - wrong offset in caret-moved event when text is inserted via javascript

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Joanmarie Diggs]

Attachment: test case/sample document

Steps to reproduce:

1. Launch Accerciser and load the attached test case.
2. Turn event monitoring on.
3. Move focus to the entry after "Link 1" then shift+tab to "Link 1"
4. Move focus to the entry after "Link 2" then shift+tab to "Link 2"

Expected results:  In both step 3 and step 4 a caret-moved event would NOT be generated for the entry which just lost focus.

Actual results:  In step 3 a caret-moved event is generated for the entry which just lost focus.  Seems to be the result of text being added to the entry via javascript.

I don't know if this is the same issue as bug 394318 or not.

Comment 1

[Aaron Leventhal]

I'm not sure if we'll get to this for Firefox 3 or not. Can we consider it a "nice to have" and see where we are with more important ones in 3-4 weeks?

Or do you see this on a lot of pages.

Comment 2

[Joanmarie Diggs]

Aaron, we are indeed seeing it more and more. :-(  It reminds me of the early days of HTML when color was first introduced and gaudy pages suddenly appeared everywhere.  (Along with blinking text.) Those were some ugly days.  Anyhoo....

If you absolutely can't get to it, -- which, for the record, is not the same as "nice to have" ;-) -- I believe I can hack around it.  Shall I sharpen my hatchet?

Comment 3

[Aaron Leventhal]

I'm going to consider it P3, where priorities are something like:

P1 is crashes & plain wrong API usage
P2 is missing features or big improvements across many pages
P3 is common enough to care about and we'd like to fix it if we have time

Comment 4

[Evan Yan]

Joanie, sorry I didn't get it why the caret-moved event shouldn't be fired. It seems to me that the caret in the entry is indeed changed when the text is inserted via javascript.

Comment 5

[Joanmarie Diggs]

When the entry first gets focus, it's at offset 0:

object:text-caret-moved(0, 0, None)
	source: [entry | bad]
	application: [application | Minefield]

When you press Shift+Tab, we're told it's at offset 1:

object:text-caret-moved(1, 0, None)
	source: [entry | bad]
	application: [application | Minefield]

Did the caret really move one character to the right?

Comment 6

[Evan Yan]

So the problem is the wrong offset, and we're fine with the caret-moved event being fired?

Comment 7

[Joanmarie Diggs]

Well... If the offset were correct (0 instead of 1), that means we were at offset 0 and we're still at offset 0.  Thus the caret hasn't moved, right?  I don't think an event should be fired in that case.

Ideally we'd only get caret-moved events fired when the caret has truly, and in a meaningful fashion, moved.

Comment 8

[Evan Yan]

I would expect the caret offset to be 11, at the end of "hello world".

"hello world" is inserted into the entry, so it seems to me that the caret moves to the end of "hello world" is the right thing. And if the caret-moved event for the entry is not needed for some AT, I think it can be easily identified by its source.

Comment 9

[Evan Yan]

Joanie, if the caret offset is fixed to the correct value, does that work for you?

The wrong caret offset is a regression. On an earlier build I tried, the caret offset was 11.

Comment 10

[Joanmarie Diggs]

Sure, that sounds like a plan.  Thanks!

Comment 11

[Evan Yan]

When shift+tab to the link, the entry still holds the old text accessible for the old text <br>. So we get the wrong offset.

Currently we do InvalidateChildren() in FlushPendingEvents(), which makes the delay in updating our accessible.

Comment 12

[Evan Yan]

Attachment: reverse part of the change in bug 397112

It's a regression of bug 397112.
I'm not sure whether this patch will break bug 397112 again. Because I can't  reproduce bug 397112 even after back out its patch. I'm testing on Linux.

Surkov, could you help make sure this patch won't break bug 397112?

Comment 13

[alexander :surkov (:asurkov)]

It should break the idea of delayed events. We should invalidate the tree after events are fired.

Comment 14

[alexander :surkov (:asurkov)]

So, probably we should fire text change events after delayed invalidation?

Comment 15

[Evan Yan]

Why we should invalidate the tree in a delayed way?

If we try to query accessible after the corresponding dom node changed but before the delayed invalidate happen, we would get wrong information.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Why we should invalidate the tree in a delayed way?

because I guess AT wants to be notified accessible was removed before it is removed and was shown after it is shown.(In reply to comment #15)

> If we try to query accessible after the corresponding dom node changed but
> before the delayed invalidate happen, we would get wrong information.
> 

patch of bug 398021 serves to guarentee this, there we freeze the accessible tree.

Comment 17

[Evan Yan]

Sounds we should not fire event against dom node in the stale list.

I tried the patch of bug 398021, because the text node is in the stale list, it failed to get offset for it. As a result, a caret-moved event with offset == 0 get fired. I'll deal with this after bug 398021 fixed.

I'm marking this bug depending on bug 398021.

Comment 18

[Evan Yan]

bug 398021 has been fixed, but didn't help this bug, because the delayed invalidation issue isn't addressed in that bug any more.

I think this bug and bug 401395 have the same root cause. We can work out this two bugs together.

Comment 19

[Aaron Leventhal]

Evan, I don't want to take Surkov's fix for bug 398021 in Firefox 3.0. I would need to bake that into the trunk after Firefox 3 branches, before being willing to take it.

So, either this bug needs to wait or you need to find a less risky way of dealing with it than waiting for bug 398021.

Comment 20

[Evan Yan]

Attachment: invalidate children for !isAsynch event

Comment 21

[Evan Yan]

this patch also fixes bug 401395

Comment 22

[Aaron Leventhal]

Comment on attachment 292392 [details] [diff] [review]
invalidate children for !isAsynch event

That's cool that this fixes both bugs.

I think it needs two things:
1) A comment describing why this code is here and only for this case
2) Prevention of an additional InvalidateChildren() in FlushPendingEvents(). That would happen later and is redundant, no?

Comment 23

[Evan Yan]

(In reply to comment #22)
> I think it needs two things:
> 1) A comment describing why this code is here and only for this case

For an un-asynch showing event, invalidate its children right now looks natural to me.

> 2) Prevention of an additional InvalidateChildren() in FlushPendingEvents().
> That would happen later and is redundant, no?
> 
I think it's ok even if redundant InvalidateChildren() get called. Because if we invalidated the old children again, it's fine; if we invalidated new generated children, the subtree will be rebuilt from cache.
I can add a condition of asynch event in FlushPendingEvents() if you like.

Actually, I'm not clear about two things.
1) If an event is not asynch, why we still call FireDelayedAccessibleEvent()? It will append the event to mEventsToFire anyway. Then the un-asynch event still get delayed.

2) We delayed InvalidateChildren() in the fix of bug 397112. If I back out the patch, bug 397112 will NOT be broken, while this bug and bug 401395 get fixed.

If we really need to dalay InvalidateChildren(), should we make it no delay for un-asynch events at least?

Comment 24

[Evan Yan]

the fix was checked in within bug 408997

Comment 25

[Willie Walker]

Woo hoo!  Thanks Evan!