695623 - xul:browser is subject of focus event when tabbing from chrome to content process

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

XUL:browser shouldn't ever be focused and be a subject of focus event. When you tab into content process then you can see the DOM focus event on xul:browser what reports to be focused (presumingly nsFocusManager returns it).

blocker of bug 693644

Comment 1

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

(In reply to alexander surkov from comment #0)
> XUL:browser shouldn't ever be focused and be a subject of focus event.
Why not?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #1)
> (In reply to alexander surkov from comment #0)
> > XUL:browser shouldn't ever be focused and be a subject of focus event.
> Why not?

I'm not aware of any way how to make it focused (or make DOM focus event dispatched on it) in single process Firefox. We should be consistent in e10s builds.

For this particular example it doesn't make sense to focus it because focus goes into content process and only one element is allowed to have focus the same time.

Comment 3

[Neil Deakin]

If the page loaded in the browser is in a different process, then the <browser> should be focused and receive a focus event.

What are you expecting to be focused?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Neil Deakin from comment #3)
> If the page loaded in the browser is in a different process, then the
> <browser> should be focused and receive a focus event.

Why?

> What are you expecting to be focused?

Nothing. If focus goes into content process then nothing should be focused in chrome process because two elements can't be focused the same time (focus must be unique - that was my start point). Further if something in content process is going to be focused then it doesn't make focus/unfocus things like xul:browser in chrome process.

Comment 5

[Neil Deakin]

(In reply to alexander surkov from comment #4)
> Nothing.

If nothing is focused it means the top-level chrome window is focused with no specific element within it focused.

> If focus goes into content process then nothing should be focused
> in chrome process because two elements can't be focused the same time (focus
> must be unique - that was my start point).

That's not correct. Only one element, the <browser>, is focused in the chrome process. The child process has its own focus manager with whatever child window or element focused.

This is also the way embedding browsers and plugins work.

Comment 6

[alexander :surkov (:asurkov)]

Per UUAG we must ensure that the user can reliably identify the focus location. If we have two focuses then we break this rule. From user point of view focus is always unique (two applications can't have focus the same time or two controls within one application can't be focused). However having two focuses might be ok from point of view of DOM but it's still confusing but that could be terminology issue.

If chrome window stays focused then it's referred as active from the user point of view and that's fine. In accessibility we ignore focused window and work with focused elements or documents.

Maybe we shouldn't rely on the way plugins work sine they aren't generally accessible or have big accessibility problems.

Comment 7

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

Well, from OS point of view, and actually from user's point of view too, it is the chrome process
which has focus. content processes are something behind the scene.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Well, from OS point of view, and actually from user's point of view too, it
> is the chrome process
> which has focus. content processes are something behind the scene.

I don't understand why the focus is in the chrome process from user point of view when the focus is in content process. In single process Firefox focus goes into content document and xul:browser doesn't acquire the focus. I don't get a reason why it should be different in multi process Firefox.

Could the window of chrome process be active (nsIFocusManager::activeWindow) and neither document nor element from chrome is focused when focus is in content process? If this is not acceptable for DOM then I need a help to figure out how to ignore focus events and focused state in accessibility for this case.

Comment 9

[Neil Deakin]

(In reply to alexander surkov from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #7)
> > Well, from OS point of view, and actually from user's point of view too, it
> > is the chrome process
> > which has focus. content processes are something behind the scene.
> 
> I don't understand why the focus is in the chrome process from user point of
> view when the focus is in content process. In single process Firefox focus
> goes into content document and xul:browser doesn't acquire the focus. I
> don't get a reason why it should be different in multi process Firefox.
> 

The focus is only in the content process. Ideally, the os would just send messages/events to the content process when it is focused. I don't know anything about the multiprocess architecture to know what actually happens though.

Comment 10

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

I meant that user doesn't even know there is content process. User sees only chrome and whatever
chrome paints.

So key event handling works so that events are dispatched to the currently focused element.
This is not changed from non-e10s.
All the events go first to the chrome process since that is the one which communicates with OS.
If chrome then notices that it is <browser remote="true"> which has focus, it forwards the event
to the content process.

Comment 11

[alexander :surkov (:asurkov)]

How does the focus work in single process Firefox, why does xul:browser never gets focus?

However my concern was rather ideological than technical. As I said for the user the focus is unique and its uniqueness doesn't depend on amount of processes. That's axiom we follow in accessibility. If you say it's ok to have one DOM focus in chrome and one DOM focus in content then I need to workaround this on accessibility side. For example, always ignore focus events and focused state on xul:browser. That works if there's no any usecase when xul:browser may have user focus.

Comment 12

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

Currently:
In non-e10s focus stays all the time in chrome process (since there is only that and that is the 
one which interacts with OS), and key events are dispatched to the focused element. In e10s focus 
stays all the time in chrome process (since that is the one which interacts with OS)
and key events are dispatched to the focused element.

However since content process has also focusmanager (since focusmanager is a single process 
service), content process keeps track of focus inside itself.

Anyway, what are the requirements for a11y? In which way does it expect focus to work in e10s, and 
could we change that assumptions? Or do we need to change how focus works in E10s and if so, why?

xul:browser does work as an event forwarder. First key events are dispatched to it (if it is 
focused), and one can call event.preventDefault() to disable forwarding to content process.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #12)

It could be that we talk in different terms. I refer to focus term defined by sec 508 (http://www.ehealth.va.gov/508/terms/term_focus.html):
Focus refers to the place in an application where user interaction takes place. Properly maintained focus allows users to know where they are in an application and what the next keystroke will do.

> Anyway, what are the requirements for a11y?

the focus (in terms of DOM focused element or focused document) must be unique within the whole application

> In which way does it expect
> focus to work in e10s, and 
> could we change that assumptions?

the way the focus is reported shouldn't depend on whether this is single process build or e10s build.

> Or do we need to change how focus works in
> E10s and if so, why?

If current behavior is correct for DOM, i.e. if there's no any confusion that each process can have own focused element or document then we shouldn't change anything on DOM side and wrap this behavior on a11y side to make it fit to sec508 focus definition.

Comment 14

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

(In reply to alexander surkov from comment #13)
> (In reply to Olli Pettay [:smaug] from comment #12)
> 
> It could be that we talk in different terms.
Yeah, I guess.


> I refer to focus term defined
> by sec 508 (http://www.ehealth.va.gov/508/terms/term_focus.html):
Never seen that.

> Focus refers to the place in an application where user interaction takes
> place. Properly maintained focus allows users to know where they are in an
> application and what the next keystroke will do.
Well, this is all getting a bit vague in multiprocess setup, and heck, it is vague even in any DOM based application, 
since event can be handled somewhere nowhere near the event target.


> 
> > Anyway, what are the requirements for a11y?
> 
> the focus (in terms of DOM focused element or focused document) must be
> unique within the whole application
And application is chrome process + all the content processes, I assume.
Why does a11y have such assumption?




> 
> > In which way does it expect
> > focus to work in e10s, and 
> > could we change that assumptions?
> 
> the way the focus is reported shouldn't depend on whether this is single
> process build or e10s build.
Well, it kind of doesn't change in e10s vs. non-e10s. Focused element is what gets key events etc.
It just happens to be that there is default action to forward certain events to a different browser when
event is targeted to <xul:browser type="remote">.
ButI  do see what you mean, I'm just not quite sure the assumption makes sense from Gecko point of view.


> 
> > Or do we need to change how focus works in
> > E10s and if so, why?
> 
> If current behavior is correct for DOM, i.e. if there's no any confusion
> that each process can have own focused element or document then we shouldn't
> change anything on DOM side and wrap this behavior on a11y side to make it
> fit to sec508 focus definition..
Only one of the content processes can have focus at a time, because only one <xul:browser> can have focus at a time.
And if no <xul:browser> (well, to be exact <xul:browser> or <xul:iframe>) has focus, then no content processes have focus.

I would assume handling this in a11y code shouldn't be too hard. If a content process has focus, then it has. Otherwise
chrome process may have the focus.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #14)
> (In reply to alexander surkov from comment #13)
> > (In reply to Olli Pettay [:smaug] from comment #12)
> > 
> > It could be that we talk in different terms.
> Yeah, I guess.
> 
> 
> > I refer to focus term defined
> > by sec 508 (http://www.ehealth.va.gov/508/terms/term_focus.html):
> Never seen that.
> 
> > Focus refers to the place in an application where user interaction takes
> > place. Properly maintained focus allows users to know where they are in an
> > application and what the next keystroke will do.
> Well, this is all getting a bit vague in multiprocess setup, and heck, it is
> vague even in any DOM based application, 
> since event can be handled somewhere nowhere near the event target.

It might be pretty hard to give perfect definition for things like this. They try to describe things from user point of view. But in either way this definition doesn't allow to have two focuses so that for example if user press tab when xul:browser has focus in chrome process while say text field has focus in content process then on one hand focus should move to the next element to xul:browser element (btw, that's what happens in bug 695622) and on the other hand it should move to the next element to text field in content process.

> > > Anyway, what are the requirements for a11y?
> > 
> > the focus (in terms of DOM focused element or focused document) must be
> > unique within the whole application
> And application is chrome process + all the content processes, I assume.
> Why does a11y have such assumption?

a11y is all about to expose things from the screen, i.e. blind people should see the same what sighted users do, a11y APIs are user oriented (might be not good wording). For example, if sighted user sees the focus on text field (that is in content process) then we can't expose anything else as focused element via a11y apis.

> Only one of the content processes can have focus at a time, because only one
> <xul:browser> can have focus at a time.
> And if no <xul:browser> (well, to be exact <xul:browser> or <xul:iframe>)
> has focus, then no content processes have focus.
> 
> I would assume handling this in a11y code shouldn't be too hard. If a
> content process has focus, then it has. Otherwise
> chrome process may have the focus.

Yes. It looks like a11y should ignore DOM focus events for remove target content (sEventStateManager::IsRemoteTarget) and do not expose them as focused via a11y apis. Moving to a11y component then.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander surkov from comment #15)
> Yes. It looks like a11y should ignore DOM focus events for remove target
> content (sEventStateManager::IsRemoteTarget) and do not expose them as
> focused via a11y apis. Moving to a11y component then.

Could you expand on this? ( Patch coming? :) )

Comment 17

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #16)
> (In reply to alexander surkov from comment #15)
> > Yes. It looks like a11y should ignore DOM focus events for remove target
> > content (sEventStateManager::IsRemoteTarget) and do not expose them as
> > focused via a11y apis. Moving to a11y component then.
> 
> Could you expand on this? ( Patch coming? :) )

are there alternatives? :) ( yes )

Comment 18

[alexander :surkov (:asurkov)]

(In reply to alexander surkov from comment #15)
> 
> Yes. It looks like a11y should ignore DOM focus events for remove target
> content (sEventStateManager::IsRemoteTarget) and do not expose them as
> focused via a11y apis. Moving to a11y component then.

Olli, if this looks reasonable, what is the best way to reuse logic of nsEventStateManager::IsRemoteTarget (it's protected member)?

Comment 19

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

Make it a public static member?

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander surkov from comment #17)
> (In reply to David Bolter [:davidb] from comment #16)
> > (In reply to alexander surkov from comment #15)
> > > Yes. It looks like a11y should ignore DOM focus events for remove target
> > > content (sEventStateManager::IsRemoteTarget) and do not expose them as
> > > focused via a11y apis. Moving to a11y component then.
> > 
> > Could you expand on this? ( Patch coming? :) )
> 
> are there alternatives? :) ( yes )

Oh "remove target" was a typo?

I think I understand now.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #21)

> Oh "remove target" was a typo?

it is

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 568445 [details] [diff] [review]
patch

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

I don't completely understand the situation, and I have questions:

::: accessible/src/base/FocusManager.cpp
@@ +361,4 @@
>  
> +  // No focus on remote target elements like xul:browser having DOM focus and
> +  // residing in chrome process because it means an element in content process
> +  // keeps the focus.

I'm not sure about the comment. How about:
// If the dom focus manager gives us a remote node then bail; the remote instance of the focus manager can return that node when requested.

Does that make sense?

@@ +363,5 @@
> +  // residing in chrome process because it means an element in content process
> +  // keeps the focus.
> +  if (focusedElm) {
> +    if (nsEventStateManager::IsRemoteTarget(focusedElm))
> +      return nsnull;

What are all the cases? If this instance of our focus manager is in a content process, will a remote target always be either a xul:browser or xul:iframe? What if this instance of this focus manager is in the main process?

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 568445 [details] [diff] [review]
patch

So is IsRemoteTarget something specific to e10s, or could this potentially be executed in single-process Firefox, too? Just wanting to point out a potential risk when we take this to make sure it doesn't impact current behavior.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #23)

> > +  // No focus on remote target elements like xul:browser having DOM focus and
> > +  // residing in chrome process because it means an element in content process
> > +  // keeps the focus.
> 
> I'm not sure about the comment. How about:
> // If the dom focus manager gives us a remote node then bail; the remote
> instance of the focus manager can return that node when requested.
> 
> Does that make sense?

I'm not sure about remote node term, remove target I borrowed from content IsRemoteTarget method. So maybe:

If the DOM focus manager gives us a remote target element like xul:browser@remote then bail because the focus is in content process.

> > +  if (focusedElm) {
> > +    if (nsEventStateManager::IsRemoteTarget(focusedElm))
> > +      return nsnull;
> 
> What are all the cases? If this instance of our focus manager is in a
> content process, will a remote target always be either a xul:browser or
> xul:iframe? 

yes

> What if this instance of this focus manager is in the main
> process?

if it's in chrome process then it returns remote target element, if it's in content process then it returns focused node in content process

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #24)
> Comment on attachment 568445 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> So is IsRemoteTarget something specific to e10s, or could this potentially
> be executed in single-process Firefox, too? Just wanting to point out a
> potential risk when we take this to make sure it doesn't impact current
> behavior.

as far as I know yes, if xul:browser has @remote attribute (IsRemoteTarget is true) then it should load the content document in content process.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander surkov from comment #25)

I finally looked at the implementation nsEventStateManager::IsRemoteTarget. This now makes more sense to me.

> (In reply to David Bolter [:davidb] from comment #23)
> 
> > > +  // No focus on remote target elements like xul:browser having DOM focus and
> > > +  // residing in chrome process because it means an element in content process
> > > +  // keeps the focus.
> > 
> > I'm not sure about the comment. How about:
> > // If the dom focus manager gives us a remote node then bail; the remote
> > instance of the focus manager can return that node when requested.
> > 
> > Does that make sense?
> 
> I'm not sure about remote node term, remove target I borrowed from content
> IsRemoteTarget method. So maybe:
> 
> If the DOM focus manager gives us a remote target element like
> xul:browser@remote then bail because the focus is in content process.

Sounds good.

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 568445 [details] [diff] [review]
patch

r=me if this tests out okay.

Comment 29

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

Comment on attachment 568445 [details] [diff] [review]
patch

r+ for the content/ part

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9367d1dae799