959257 - google's search box may not emit caret move events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

was reported about bug:
1) load google.com
2) type into google's searchbox => no caret move events

if you move the focus and gets back to searcbox then events are fired

Comment 1

[Jonathan Wei [:jwei]]

It seems that the searchbox is firing off selection notifications as expected (SelectionManager::NotifySelectionChanged).  However, they are not being processed properly.  By the time the SelectionManager processes the change in selection (SelectionManager::ProcessSelectionChanged), the associated pres shell pointer is null.  The call stack for the removal of the pres shell from the selection includes the following:

nsTextControlFrame::DestroyFrom ->
HTMLInputElement::UnbindFromFrame ->
nsTextEditorState::UnbindFromFrame ->
nsTextInputSelectionImpl::SetScrollableFrame ->
nsFrameSelection::DisconnectFromPresShell

What might be happening such that the frame and pres shell are being torn down before the event can be processed?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #1)
> It seems that the searchbox is firing off selection notifications as
> expected (SelectionManager::NotifySelectionChanged).  However, they are not
> being processed properly.  By the time the SelectionManager processes the
> change in selection (SelectionManager::ProcessSelectionChanged), the
> associated pres shell pointer is null. 

for each typed character? is it also null in SelectionManager::NotifySelectionChanged?

Comment 3

[Jonathan Wei [:jwei]]

At the point of NotifySelectionChanged, the associated pres shell is not null yet.  I suspect it's for each typed character, and not just the first, though I'm currently not sure how to check for each typed character.  After windows are switched after the breakpoint for the first character, it seems that the caret position is reset (text caret move events are processed properly after that point).  Without breakpoints, none are processed as long as the caret isn't manually moved or refocused.

Comment 4

[alexander :surkov (:asurkov)]

you should be able to use conditional breakpointers to trigger for 2nd/3d/etc selection change only.

Comment 5

[Jonathan Wei [:jwei]]

Sorry, somehow completely forgot about conditional breakpoints.  The following behaviour is observed:

- Ignoring the 1st NotifySelectionChanged and ProcessSelectionChanged, no more selection notifications are fired/processed when directly typing in the searchbox.
- Ignoring the first 3 NotifySelectionChanged and ProcessSelectionChanged, when clicking a DIV on the page (2 notifications are processed) and back into the searchbox (1 notification is processed), then no more selection notifications are fired/processed after that.
- After having the page switch to instant results, notifications stop firing.  If the searchbox is refocused after switching to the instant results mode, text caret move notifications are fired and processed properly.
- Disabling instant results has the text caret move notifications firing and processing properly.

It seems it's the rearrangement of the home page to the instant results that has the text caret move notifications stop firing.  I'll do more investigation to find the actual cause.

Comment 6

[Jonathan Wei [:jwei]]

The DocAccessible that was used when creating the notification still exists at the point of processing the notification.  However, the PresShell pointer that links the Selection to the document has been cleared by this point, although the DocAccessible itself still has a reference to the PresShell.

Comment 7

[alexander :surkov (:asurkov)]

is PresShell object same for Selection and DocAccessible? So PresShell is alive, correct, just text field is disconnected from that presshell (per comment #1), is that correct? What initiates the nsFrameSelection::DisconnectFromPresShell?

Comment 8

[Jonathan Wei [:jwei]]

Yes, it's the same PresShell object that the DocAccessible and the Selection (should) point to.  Looking at the call stack, I think the point at which this starts is when get_offsetWidth is called from JS, which triggers a flush of notifications, in which restyles are processed.  This causes the current (old) frame to be destroyed when RecreateFramesForContent is called.

Comment 9

[alexander :surkov (:asurkov)]

which frame is destroyed? can you please attach the whole stack?

Comment 10

[Jonathan Wei [:jwei]]

Attachment: DisconnectFromPresShell Call Stack

Comment 11

[Jonathan Wei [:jwei]]

Attachment: DisconnectFromPresShell Call Stack

Comment 12

[alexander :surkov (:asurkov)]

So the problem is 
1) a11y gets notified about selection change in text fields (nsISelectionListener::NotifySelectionChanged) 2) the presshell gets disconnected from selection object (see stack trace)
3) nsARefreshObserver::WillRefresh calls into a11y to process selection change but we can't having no presshell associated with selection object.

Olli, do you have ideas how to proceed here? Is it really necessary to disconnect presshell from selection in this case?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

The code was added in Bug 421083 (and then later moved to current place).
Given the current setup where mShell is a raw value we must call DisconnectFromPresShell at some point
and the current place is the most logical one.

We could perhaps make mShell strong member variable and use cycle collector to break the edge.
(call DisconnectFromPresShell() during unlink)

Comment 14

[alexander :surkov (:asurkov)]

Jonathan please correct me if I'm wrong

1) a11y gets notified about selection change
2) after that (because of some JS script running) frame of the text field is recreated, and thus its accessible gets recreated too and its selection object is recreated as well
3) then after typing we don't get any selection notification because selection object we added listeners for is another one

Comment 15

[Jonathan Wei [:jwei]]

Yes, that appears to be the case.  The FrameSelection is different between the initial Selection (before the recreation of the frame) and after.

Comment 16

[alexander :surkov (:asurkov)]

Olli, is it something supposed to happen, i.e. when frame is destroyed then associated FrameSelection and Selection objects are destroyed as well?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

ehsan should know better the current setup for text editors.

But, I think selection lives for the native anonymous content and that is recreated when
creating frames.

Comment 18

[(no longer active)]

Yes, each plaintext editor instance (the object used for text inputs and textareas) has its own selection controller and frame selection objects.  These objects are created in nsTextEditorState::BindToFrame(), which is a method that gets called every time that the DOM element gets a new frame.

We have previously had other problems about the selection objects changing this way.  Specifically, we used to forget the selection offsets for the element when it got reframed.  A while ago we fixed this by restoring the selection after the reframing happens (see the code at the end of nsTextEditorState::PrepareEditor()).  We should probably let the a11y engine know about these changes, perhaps from the same place.

Comment 19

[alexander :surkov (:asurkov)]

a11y notified when frame is constructed/destroyed so we could update selection stuff at this point (or for this change). Are there any cases when selection object can be changed (but frame isn't reconstructed)?

Comment 20

[(no longer active)]

(In reply to alexander :surkov from comment #19)
> a11y notified when frame is constructed/destroyed so we could update
> selection stuff at this point (or for this change). Are there any cases when
> selection object can be changed (but frame isn't reconstructed)?

Sure, the selection is exposed to the DOM for example, which means that pages can modify it without causing frame reconstruction.  The right thing to do is to use selection listeners here.

Comment 21

[alexander :surkov (:asurkov)]

So we need to set up selection listener when we fire focus event for recreated accessible (see DocAccessible::UpdateTreeInternal)

Comment 22

[Jonathan Wei [:jwei]]

Attachment: Set control selection listener upon creation of new focused accessible

This makes it so that text caret events are fired after the switch to Google Instant.  However, the first event is not properly handled, as the event is created prior to the creation of a new accessible for the input.

Comment 23

[Jonathan Wei [:jwei]]

Attachment: Set control selection listener upon creation of new focused accessible

Added commit message.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd483927c59

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cd483927c59