847763 - Prevent virtual keyboard iframe from getting focus

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Evelyn Hung]

Before OOP, we did a preventDefault when the user is trying to interact with the keyboard app. This works fine but it's not a correct way after OOP keyboard apps, because they are under different context now, the preventDefault won't work anymore. 
We also can't rely on a (3rd party) keyboard app passing the focus. Need a mechanism on the platform to prevent keyboard apps getting focus from the input field.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The work require here has nothing to do with OOP.

What we need here is having Gecko never to send focus to the keyboard frame (thus taken the focus out of input field). We currently workaround this by ev.preventDefault() the received event in our built-in keyboard app, however FxOS should not rely on 3rd-party apps implementing this.

Alternatively, we might want to "simulate" a focus on the keyboard frame, but not to take the real focus away from the input field? Is that possible? If so, it will at least make CSS :target, :active, and :focus available in the keyboard app.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we currently have a way to prevent a frame from taking focus while still receiving events. I think this would require new API --- perhaps a new attribute or CSS property on <iframe> --- that prevents it from taking focus. Neil, is that right?

Comment 3

[Neil Deakin]

While still receiving which events? focus/blur, keyboard, something else?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mouse and touch events. Of course it couldn't receive keyboard events while it's not focused.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, maybe -moz-user-select:none is all we need here?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, it takes focus away from wherever the focus currently is.

Comment 7

[Neil Deakin]

Do you essentially want -moz-user-focus: ignore? That is, clicks on elements don't cause the focus to change? The actual sending of mouse events is unaffected by the focus.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

If I put this in keyboard/css/keyboard.css:

* {
  -moz-user-focus: ignore;
}

And remove all preventDefault() calls then this works fine in FF Nightly and B2G desktop but not on the device. So a bug somewhere with user-focus and touch?

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #8)
> If I put this in keyboard/css/keyboard.css:
> 
> * {
>   -moz-user-focus: ignore;
> }
> 
> And remove all preventDefault() calls then this works fine in FF Nightly and
> B2G desktop but not on the device. So a bug somewhere with user-focus and
> touch?

Note this won't fix this issue because we can't rely on the keyboard app to set this style. We have to make the containing iframe non-focusible.

Comment 10

[Evelyn Hung]

(In reply to Kan-Ru Chen [:kanru] from comment #9)
> (In reply to Jan Jongboom [:janjongboom] from comment #8)
> > If I put this in keyboard/css/keyboard.css:
> > 
> > * {
> >   -moz-user-focus: ignore;
> > }
> > 
> > And remove all preventDefault() calls then this works fine in FF Nightly and
> > B2G desktop but not on the device. So a bug somewhere with user-focus and
> > touch?
> 
> Note this won't fix this issue because we can't rely on the keyboard app to
> set this style. We have to make the containing iframe non-focusible.
Agree, especially the keyboard app is developed by 3rd-party.

Remove dependency because correct -moz-user-focus behavior won't solve the problem here.

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Evelyn Hung [:evelyn] from comment #10)
> (In reply to Kan-Ru Chen [:kanru] from comment #9)
> Agree, especially the keyboard app is developed by 3rd-party.
> 
> Remove dependency because correct -moz-user-focus behavior won't solve the
> problem here.

Yeah so my idea was to add a new property on iframe that then would add * { -moz-user-focus: ignore } as a default style to that iframe. Don't know if that makes sense?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #11)
> (In reply to Evelyn Hung [:evelyn] from comment #10)
> > (In reply to Kan-Ru Chen [:kanru] from comment #9)
> > Agree, especially the keyboard app is developed by 3rd-party.
> > 
> > Remove dependency because correct -moz-user-focus behavior won't solve the
> > problem here.
> 
> Yeah so my idea was to add a new property on iframe that then would add * {
> -moz-user-focus: ignore } as a default style to that iframe. Don't know if
> that makes sense?

Yeah that might work. I'm not sure if it's the best way to do this though. /me testing OOP case.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

BTW, I'm tracing how the focus changes between the parent window and iframe in normal and OOP case. As I test so far, -moz-user-focus did take effects but breaks correct keyboard behavior. Still trying to figure out the problem.

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

Reuse -moz-user-focus: ignore for mozbrowser.

Together with patch from bug 845169, if a remote mozbrowser has -moz-user-focus:ignore then it won't trigger blur event on original target.

We could discuss whether to reuse -moz-user-focus or create a new attribute for mozbrowser.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Doesn't this block all focusing on mozbrowser iframes or does the `-1 == *aTabIndex` cover that?

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #15)
> Doesn't this block all focusing on mozbrowser iframes or does the `-1 ==
> *aTabIndex` cover that?

Yes, the -1 == *aTabIndex check ensure the mozbrowser iframe has -moz-user-focus:ignore set.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> (In reply to Jan Jongboom [:janjongboom] from comment #15)
> > Doesn't this block all focusing on mozbrowser iframes or does the `-1 ==
> > *aTabIndex` cover that?
> 
> Yes, the -1 == *aTabIndex check ensure the mozbrowser iframe has
> -moz-user-focus:ignore set.

Alright, this is the CSS class we're talking about right? Is this ready for review?

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jan Jongboom [:janjongboom] from comment #17)
> (In reply to Kan-Ru Chen [:kanru] from comment #16)
> > (In reply to Jan Jongboom [:janjongboom] from comment #15)
> > > Doesn't this block all focusing on mozbrowser iframes or does the `-1 ==
> > > *aTabIndex` cover that?
> > 
> > Yes, the -1 == *aTabIndex check ensure the mozbrowser iframe has
> > -moz-user-focus:ignore set.
> 
> Alright, this is the CSS class we're talking about right? Is this ready for
> review?

I don't think so. This changes the semantic of -moz-user-focus (if there is a defined semantic, it should only apply to the matched element, not it's descendents). I'd like to add a new attribute "ignore-focus" to mozbrowser instead.

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

+1 for new argument.

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-a-mozignorefocus-property-to-mozbrows.patch

Comment 21

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

Comment on attachment 787976 [details] [diff] [review]
0001-Bug-847763-Add-a-mozignorefocus-property-to-mozbrows.patch

I think we should make -moz-user-focus work properly with iframes.
I we can't focus iframe itself, sure we shouldn't be able to
focus its contents. It isn't about descendants, since the contents of 
iframe aren't descendants in DOM tree or anything. It is about the iframe itself.

Comment 22

[Ivan Tsay (:ITsay)]

Add keyword in white board for tracking in https://wiki.mozilla.org/FirefoxOS/SprintStatus#Systems-Platform

Comment 23

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

kanru, so I think http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1409
should check the iframes in parent documents.
And remember that aTabIndex is odd inout parameter
http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#534

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML

It seems -moz-user-focus has never worked correctly in HTML.

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Check if our parent frame is focusable

This should work however in B2G the root html:iframe containing the system app gets -moz-user-focus:ignore. I'm sure where this was set, it makes every app in B2G non-focusable.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

(gdb) p *frame->mStyleContext->mRuleNode->mRule->List(0x40125b6c, 0)
 { overflow: hidden; -moz-box-flex: 1; border: medium none;}

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

So the system app html:iframe is loaded in b2g/chrome/content/shell.xul which got applied the toolkit/content/xul.css:

* {
  -moz-user-focus: ignore;
  -moz-user-select: none;
  display: -moz-box;
  -moz-box-sizing: border-box;
}

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Set system app iframe -moz-user-focus:normal

Comment 29

[Neil Deakin]

(In reply to Kan-Ru Chen [:kanru] from comment #24)
> Created attachment 790614 [details] [diff] [review]
> Fix -moz-user-focus in HTML
> 
> It seems -moz-user-focus has never worked correctly in HTML.

No, support for -moz-user-focus on html elements was intentionally removed. The remaining distinction that 'none' versus 'ignore' has an effect should also be removed on html elements. Or, we should decide to support -moz-user-focus fully again on them. But I'm not sure that's a decision that

Comment 30

[[:fabrice] Fabrice Desré]

Neil, can you finish your last sentence? Do you object using -moz-user-focus:normal ?

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #29)
> (In reply to Kan-Ru Chen [:kanru] from comment #24)
> > Created attachment 790614 [details] [diff] [review]
> > Fix -moz-user-focus in HTML
> > 
> > It seems -moz-user-focus has never worked correctly in HTML.
> 
> No, support for -moz-user-focus on html elements was intentionally removed.
> The remaining distinction that 'none' versus 'ignore' has an effect should
> also be removed on html elements. Or, we should decide to support
> -moz-user-focus fully again on them. But I'm not sure that's a decision that

I think for now supporting 'none', 'normal' and 'ignore' is enough. The other values aren't used even in XUL and they don't seem to be implemented.

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

Just found in bug 313088 and bug 474440 there was some effort trying to deprecate -moz-user-focus. So what should we do now? Either remove it completely or add full support for XUL and HTML back is better than keep a ghost in the code.

Comment 33

[Neil Deakin]

My last sentence was supposed to be "But I'm not sure that's a decision that I can make."

Comment 34

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

hmm, so support for -moz-user-focus on html elements was partially removed, but not for iframes.

Comment 35

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

Hmm, tabindex=-1 on iframe certainly isn't enough.

Comment 36

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

Comment on attachment 790616 [details] [diff] [review]
Check if our parent frame is focusable

You should null check the return value of GetWindow too.

Comment 37

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

Comment on attachment 790614 [details] [diff] [review]
Fix -moz-user-focus in HTML

>   }
>+  else if (tabIndex == -1) {
>+    // -moz-user-focus:ignore
>+    override = true;
>+  }
>   else {

} else if (...) {

...} else {
...

>+++ b/dom/base/nsFocusManager.cpp
>@@ -1457,21 +1457,21 @@ nsFocusManager::CheckIfFocusable(nsIContent* aContent, uint32_t aFlags)
>   }
> 
>   // if this is a child frame content node, check if it is visible and
>   // call the content node's IsFocusable method instead of the frame's
>   // IsFocusable method. This skips checking the style system and ensures that
>   // offscreen browsers can still be focused.
>   nsIDocument* subdoc = doc->GetSubDocumentFor(aContent);
>   if (subdoc && IsWindowVisible(subdoc->GetWindow())) {
>     const nsStyleUserInterface* ui = frame->StyleUserInterface();
>     int32_t tabIndex = (ui->mUserFocus == NS_STYLE_USER_FOCUS_IGNORE ||
>-                        ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE) ? -1 : 0;
>+                        (aContent->IsXUL() && ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE)) ? -1 : 0;
I don't understand the IsXUL check.


>@@ -2863,21 +2863,22 @@ nsFocusManager::GetNextTabbableMapArea(bool aForward,
> 
>   nsCOMPtr<nsIDocument> doc = aImageContent->GetDocument();
>   if (doc) {
>     nsCOMPtr<nsIContent> mapContent = doc->FindImageMap(useMap);
>     if (!mapContent)
>       return nullptr;
>     uint32_t count = mapContent->GetChildCount();
>     // First see if the the start content is in this map
> 
>     int32_t index = mapContent->IndexOf(aStartContent);
>-    int32_t tabIndex;
>+    // XXX must initialize this InOut parameter. But to what value?
>+    int32_t tabIndex = 0;
0 should be ok


>+++ b/layout/generic/nsFrame.cpp
>@@ -7300,22 +7300,23 @@ nsIFrame::IsFocusable(int32_t *aTabIndex, bool aWithMouse)
> {
>   int32_t tabIndex = -1;
>   if (aTabIndex) {
>     *aTabIndex = -1; // Default for early return is not focusable
>   }
>   bool isFocusable = false;
> 
>   if (mContent && mContent->IsElement() && IsVisibleConsideringAncestors()) {
>     const nsStyleUserInterface* ui = StyleUserInterface();
>     if (ui->mUserFocus != NS_STYLE_USER_FOCUS_IGNORE &&
>-        ui->mUserFocus != NS_STYLE_USER_FOCUS_NONE) {
>+        (mContent->IsHTML() || ui->mUserFocus != NS_STYLE_USER_FOCUS_NONE)) {
>       // Pass in default tabindex of -1 for nonfocusable and 0 for focusable
>+      // HTML element will use its default tabindex if we pass 0
>       tabIndex = 0;
IsHTML is odd here.

Could we make HTML to work like XUL. tabindex attribute always overrides the css.

And perhaps make ignore and none synonyms.

Comment 38

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

Neil, are you strongly against -moz-user-focus? Since we need something new anyway, I'd prefer fixing
existing feature rather than adding something totally new.

Comment 39

[Neil Deakin]

> Neil, are you strongly against -moz-user-focus?

I don't have anything against using it. Eventually it would be good to remove/replace it entirely though.

> And perhaps make ignore and none synonyms.

Note that ignore and none don't do the same thing.

Comment 40

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #37)
> >   // if this is a child frame content node, check if it is visible and
> >   // call the content node's IsFocusable method instead of the frame's
> >   // IsFocusable method. This skips checking the style system and ensures that
> >   // offscreen browsers can still be focused.
> >   nsIDocument* subdoc = doc->GetSubDocumentFor(aContent);
> >   if (subdoc && IsWindowVisible(subdoc->GetWindow())) {
> >     const nsStyleUserInterface* ui = frame->StyleUserInterface();
> >     int32_t tabIndex = (ui->mUserFocus == NS_STYLE_USER_FOCUS_IGNORE ||
> >-                        ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE) ? -1 : 0;
> >+                        (aContent->IsXUL() && ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE)) ? -1 : 0;
> I don't understand the IsXUL check.

Only XUL element need special treatment for NS_STYLE_USER_FOCUS_NONE as they do not have default tabIndex. But HTML elements has default tabIndex (-1 or 0) depending on the element type.

> >+++ b/layout/generic/nsFrame.cpp
> >@@ -7300,22 +7300,23 @@ nsIFrame::IsFocusable(int32_t *aTabIndex, bool aWithMouse)
> > {
> >   int32_t tabIndex = -1;
> >   if (aTabIndex) {
> >     *aTabIndex = -1; // Default for early return is not focusable
> >   }
> >   bool isFocusable = false;
> > 
> >   if (mContent && mContent->IsElement() && IsVisibleConsideringAncestors()) {
> >     const nsStyleUserInterface* ui = StyleUserInterface();
> >     if (ui->mUserFocus != NS_STYLE_USER_FOCUS_IGNORE &&
> >-        ui->mUserFocus != NS_STYLE_USER_FOCUS_NONE) {
> >+        (mContent->IsHTML() || ui->mUserFocus != NS_STYLE_USER_FOCUS_NONE)) {
> >       // Pass in default tabindex of -1 for nonfocusable and 0 for focusable
> >+      // HTML element will use its default tabindex if we pass 0
> >       tabIndex = 0;
> IsHTML is odd here.

Yeah, but see above check for XUL.

> Could we make HTML to work like XUL. tabindex attribute always overrides the
> css.

It does. However if the tabindex is absent then -moz-user-focus & default tabindex take precedence.

> And perhaps make ignore and none synonyms.

I'm not sure. Their difference is subtle for XUL, we set default for every XUL elements to "ignore" anyway, but I think for HTML "none" should keep the current behavior.

Comment 41

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

> > Could we make HTML to work like XUL. tabindex attribute always overrides the
> > css.
> 
> It does. However if the tabindex is absent then -moz-user-focus & default
> tabindex take precedence.

I don't see how tabindex overrides -moz-user-focus in iframes.

Comment 42

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

(In reply to Neil Deakin from comment #39)
> > And perhaps make ignore and none synonyms.
> 
> Note that ignore and none don't do the same thing.

In most cases they do.
It is not clear to me why 'ignore' has the odd 'suppress blur' effect.
(hyatt added it 13 years ago, but the commit message doesn't have bug#)

Comment 43

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

(In reply to Neil Deakin from comment #39)
> > Neil, are you strongly against -moz-user-focus?
> 
> I don't have anything against using it. Eventually it would be good to
> remove/replace it entirely though.

replace with what?

Comment 44

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #41)
> > > Could we make HTML to work like XUL. tabindex attribute always overrides the
> > > css.
> > 
> > It does. However if the tabindex is absent then -moz-user-focus & default
> > tabindex take precedence.
> 
> I don't see how tabindex overrides -moz-user-focus in iframes.

Hmm.. It does not. I'll add a check in nsGenericHTMLFrameElement::IsHTMLFocusable.

Comment 45

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v2

Comment 46

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Check if our parent frame is focusable v2

carry over r+

Comment 47

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

Comment on attachment 795343 [details] [diff] [review]
Fix -moz-user-focus in HTML v2

>From 77226a775c6e31aad4cbeeab95369cd78e608f97 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?"Kan-Ru=20Chen=20(=E9=99=B3=E4=BE=83=E5=A6=82)"?=
> <[e-mail]>
>Date: Thu, 15 Aug 2013 11:25:55 +0800
>Subject: [PATCH 1/3] Bug 847763 - Fix -moz-user-focus in HTML. r=smaug
>
>---
> content/html/content/src/nsGenericHTMLElement.cpp      |    9 ++++++---
> content/html/content/src/nsGenericHTMLFrameElement.cpp |    6 +++++-
> dom/base/nsFocusManager.cpp                            |    7 +++++--
> layout/generic/nsFrame.cpp                             |    5 ++++-
> 4 files changed, 20 insertions(+), 7 deletions(-)
>
>diff --git a/content/html/content/src/nsGenericHTMLElement.cpp b/content/html/content/src/nsGenericHTMLElement.cpp
>index 12b47e8..b0b8796 100644
>--- a/content/html/content/src/nsGenericHTMLElement.cpp
>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>@@ -2750,39 +2750,42 @@ nsGenericHTMLElement::IsHTMLFocusable(bool aWithMouse,
>     // In designMode documents we only allow focusing the document.
>     if (aTabIndex) {
>       *aTabIndex = -1;
>     }
> 
>     *aIsFocusable = false;
> 
>     return true;
>   }
> 
>-  int32_t tabIndex = TabIndex();
>+  int32_t tabIndex = (aTabIndex && *aTabIndex >= 0) ? TabIndex() : -1;
> 
>   bool override, disabled = false;
>   if (IsEditableRoot()) {
>     // Editable roots should always be focusable.
>     override = true;
> 
>     // Ignore the disabled attribute in editable contentEditable/designMode
>     // roots.
>     if (!HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)) {
>       // The default value for tabindex should be 0 for editable
>       // contentEditable roots.
>       tabIndex = 0;
>     }
>-  }
>-  else {
>+  } else if (tabIndex == -1) {
>+    // -moz-user-focus:ignore
>+    override = true;
This is a bit odd. tabIndex can be -1 also just because TabIndex() returned -1. So the comment isn't right.

And -moz-user-focus ends up overriding TabIndex if -1 is passed as aTabIndex.

Comment 48

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v3

Comment 49

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

Comment on attachment 797736 [details] [diff] [review]
Fix -moz-user-focus in HTML v3

Sorry, still not quite right, if I read this correctly.
We don't want to make iframe focusable if nsContentUtils::IsSubDocumentTabbable returns false, even if there is tabindex attribute. And this really needs tests.

Have you gone through other IsHTMLFocusable implementations?

Comment 50

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #49)
> Comment on attachment 797736 [details] [diff] [review]
> Fix -moz-user-focus in HTML v3
> 
> Sorry, still not quite right, if I read this correctly.
> We don't want to make iframe focusable if
> nsContentUtils::IsSubDocumentTabbable returns false, even if there is
> tabindex attribute. And this really needs tests.

But we wanted to override the behavior! A virtual keyboard is subdocument tabbable because it's a remote iframe but we don't want it be focused.
 
> Have you gone through other IsHTMLFocusable implementations?

Yeah, I made a list. Only some of them do weird things to the return value, otherwise they use the generic IsHTMLFocusable imlp.

Comment 51

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kan-Ru Chen [:kanru] from comment #50)
> (In reply to Olli Pettay [:smaug] from comment #49)
> > Comment on attachment 797736 [details] [diff] [review]
> > Fix -moz-user-focus in HTML v3
> > 
> > Sorry, still not quite right, if I read this correctly.
> > We don't want to make iframe focusable if
> > nsContentUtils::IsSubDocumentTabbable returns false, even if there is
> > tabindex attribute. And this really needs tests.
> 
> But we wanted to override the behavior! A virtual keyboard is subdocument
> tabbable because it's a remote iframe but we don't want it be focused.
>  
> > Have you gone through other IsHTMLFocusable implementations?
> 
> Yeah, I made a list. Only some of them do weird things to the return value,
> otherwise they use the generic IsHTMLFocusable imlp.

Hmm.. on a second thought we indeed should not change the tabindex behavior. I'll make sure they remain unchanged.

Comment 52

[Ivan Tsay (:ITsay)]

Update keyword in white board for the status query more precise

Comment 53

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

Kanru, could you write some tests too. And kick me if I don't review the new patch quickly :)

Comment 54

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v4 and tests

I don't know how to create a !IsSubDocumentTabbable iframe :/

More tests could be easily added to that file.

Comment 55

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

Yeah, !IssubDocumentTabbable is tricky. But tests for other stuff is needed.

I'll try to review during this weekend.

Comment 56

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

Comment on attachment 800689 [details] [diff] [review]
Fix -moz-user-focus in HTML v4 and tests

Could you still add a test where 
tabindex="0"
and
style="-moz-user-focus:ignore">

Comment 57

[Kan-Ru Chen [:kanru] (UTC+9)]

Thanks for the review!

https://tbpl.mozilla.org/?tree=Try&rev=a5e44eefeb22

Comment 58

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 790624 [details] [diff] [review]
Set system app iframe -moz-user-focus:normal

We don't need this anymore. The shell document is in HTML now.

Comment 59

[Neil Deakin]

I tried this patch and it prevents the address bar from being focused, or any <textbox> element.

Also, why are you making the tabindex behaviour different for 'none' and 'ignore'? Only the mousedown behaviour should be different.

Comment 60

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #59)
> I tried this patch and it prevents the address bar from being focused, or
> any <textbox> element.

The try run also got some orange. I'll check it.
 
> Also, why are you making the tabindex behaviour different for 'none' and
> 'ignore'? Only the mousedown behaviour should be different.

tabindex behavior is same for 'none' and 'ignore'. It overrides -moz-user-focus. So the current algorithm for HTML is

 1. If there is tabindex, then the element is focusable by default but could be overrode by the element
 2. If there is no tabindex but -moz-user-focus:ignore then the element is not focusable
 3. If there is no tabindex and -moz-user-focus then the element is focusable and uses it's default tabindex

Comment 61

[Neil Deakin]

(In reply to Kan-Ru Chen [:kanru] from comment #60)
> tabindex behavior is same for 'none' and 'ignore'.

But the patch uses a different value for tabIndex for 'none' as 'ignore' here:

     int32_t tabIndex = (ui->mUserFocus == NS_STYLE_USER_FOCUS_IGNORE ||
-                        ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE) ? -1 : 0;
+                        (aContent->IsXUL() && ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE)) ? -1 : 0;

I didn't examine the patch in detail but that line above suggests you expect something different to happen for html elements in each case.

>  3. If there is no tabindex and -moz-user-focus then the element is
> focusable and uses it's default tabindex

You missed a word there. Did you mean -moz-user-focus: normal ?

Comment 62

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #61)
> (In reply to Kan-Ru Chen [:kanru] from comment #60)
> > tabindex behavior is same for 'none' and 'ignore'.
> 
> But the patch uses a different value for tabIndex for 'none' as 'ignore'
> here:
> 
>      int32_t tabIndex = (ui->mUserFocus == NS_STYLE_USER_FOCUS_IGNORE ||
> -                        ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE) ? -1 :
> 0;
> +                        (aContent->IsXUL() && ui->mUserFocus ==
> NS_STYLE_USER_FOCUS_NONE)) ? -1 : 0;
> 
> I didn't examine the patch in detail but that line above suggests you expect
> something different to happen for html elements in each case.

Yes, because unlike XUL, HTML elements have default tabindex and -moz-user-focus:none shouldn't change it.

> >  3. If there is no tabindex and -moz-user-focus then the element is
> > focusable and uses it's default tabindex
> 
> You missed a word there. Did you mean -moz-user-focus: normal ?

I mean no -moz-user-focus is specified. I hope I'm not get this wrong. When you don't specify the -moz-user-focus style the value is NS_STYLE_USER_FOCUS_NONE, is this case it's not much different from NS_STYLE_USER_FOCUS_NORMAL for HTML.

Comment 63

[Neil Deakin]

(In reply to Kan-Ru Chen [:kanru] from comment #62)
> Yes, because unlike XUL, HTML elements have default tabindex and
> -moz-user-focus:none shouldn't change it.

Yes, but your code sets the default tab index to 0 when -moz-user-focus: none is used but sets it to -1 when -moz-user-focus: ignore is used. When I see this, it suggests to me that you expect something different to happen relating to tabbing behaviour when 'none' is used rather than with 'ignore'.

> I mean no -moz-user-focus is specified. I hope I'm not get this wrong.
> When you don't specify the -moz-user-focus style the value is
> NS_STYLE_USER_FOCUS_NONE, is this case it's not much different from
> NS_STYLE_USER_FOCUS_NORMAL for HTML.

It is different because -moz-user-focus: normal means the element can be focused and -moz-user-focus: none means that this element cannot be focused.

Comment 64

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #63)
> (In reply to Kan-Ru Chen [:kanru] from comment #62)
> > Yes, because unlike XUL, HTML elements have default tabindex and
> > -moz-user-focus:none shouldn't change it.
> 
> Yes, but your code sets the default tab index to 0 when -moz-user-focus:
> none is used but sets it to -1 when -moz-user-focus: ignore is used. When I
> see this, it suggests to me that you expect something different to happen
> relating to tabbing behaviour when 'none' is used rather than with 'ignore'.

OK. But this is the protocol of the aTabindex. It is a inout parameter so when the input is -1 it means -moz-user-focus:ignore. When the input is 0 it means to use the default. The aTabindex will then be filled with the real tabindex from the attribute.
 
> > I mean no -moz-user-focus is specified. I hope I'm not get this wrong.
> > When you don't specify the -moz-user-focus style the value is
> > NS_STYLE_USER_FOCUS_NONE, is this case it's not much different from
> > NS_STYLE_USER_FOCUS_NORMAL for HTML.
> 
> It is different because -moz-user-focus: normal means the element can be
> focused and -moz-user-focus: none means that this element cannot be focused.

I don't think so. If -moz-user-focus:none means not focusable then most of the elements will not be focusable by default.

I see our impl has following properties:

'-moz-user-focus'
    Values:	none | ignore | normal | select-all | select-before | select-after | select-same | select-menu | inherit
    Initial:	none
    Applies to:	all elements
    Inherited:	yes

If we want to make 'none' identical to 'ignore' we should probably change 'normal' to 'auto'

'-moz-user-focus'
    Values:	auto | ignore | select-all | select-before | select-after | select-same | select-menu | inherit
    Initial:	auto
    Applies to:	all elements
    Inherited:	yes

Comment 65

[Neil Deakin]

> I don't think so. If -moz-user-focus:none means not focusable then most of
> the elements will not be focusable by default.
> 

There are no elements that are focusable by default. XUL elements must set -moz-user-focus: normal to be focusable. Alternatively, nsIContent::IsFocusable can override this for specific elements, and XUL elements sometimes override it and HTML elements (which don't want to support -moz-user-focus currently) always override it. If you don't do either, -moz-user-focus: none is used and the element becomes not focusable.

> If we want to make 'none' identical to 'ignore' we should probably change 'normal' to 'auto'

Perhaps, but then you would need to set -moz-user-focus: none on most HTML elements since almost all of them are not focusable.

Comment 66

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #65)
> > If we want to make 'none' identical to 'ignore' we should probably change 'normal' to 'auto'
> 
> Perhaps, but then you would need to set -moz-user-focus: none on most HTML
> elements since almost all of them are not focusable.

Right. Currently all HTML elements override the default by TabIndexDefault() so this is not a problem.

Comment 67

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #59)
> I tried this patch and it prevents the address bar from being focused, or
> any <textbox> element.

Because in xul.css all elements are marked as -moz-user-focus:ignore
We should restrict it to XUL elements only.

And the oranges are caused by the recursive iframe checking patch. Maybe the top-level frame is not focusable?

Comment 68

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v5 and tests

Set html|* to -moz-user-focus:none

Comment 69

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Check if our parent frame is focusable v3

Only check -moz-user-focus. This is not very accurate but a safe option.

Comment 70

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=9afcbe74df69

Comment 71

[Neil Deakin]

> Right. Currently all HTML elements override the default by TabIndexDefault() so this is not a problem.

If is a problem because the patch doesn't match expected behaviour.

For example, take the following test and tab through the buttons:

<button id="one" style="-moz-user-focus: ignore;">Ignore</button>
<button id="two" style="-moz-user-focus: none;">None</button>
<button id="three" style="-moz-user-focus: ignore;" tabindex="1">Tabbable Ignore</button>
<button id="four" style="-moz-user-focus: none;" tabindex="1">Tabbable None</button>

I expect either:
 - the existing behaviour where -moz-user-focus is essentially ignored and all four buttons are in the tab order.
 - the XUL behaviour where only buttons three and four are in the tab order.

Your patch has two, three and four in the tab order, which doesn't make sense to me.

What you should be doing is treating ignore and none the same for tabbing/focusability. The distinction is only relevant for the suppressBlur stuff in the EventStateManager.

Comment 72

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

Comment on attachment 802379 [details] [diff] [review]
Fix -moz-user-focus in HTML v5 and tests

Yeah, something is wrong if you need to change xul.css

Comment 73

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

Comment on attachment 802380 [details] [diff] [review]
Check if our parent frame is focusable v3


>+  nsCOMPtr<nsPIDOMWindow> ownWindow = do_QueryInterface(aContent->OwnerDoc()->GetWindow());
this QI is not needed. (and QI is a bit slow)

>+  if (ownWindow) {
>+    nsCOMPtr<nsIContent> containerElement = do_QueryInterface(ownWindow->GetFrameElementInternal());
this QI is not needed

>+    if (containerElement) {
>+      const nsIFrame* containerFrame = containerElement->GetPrimaryFrame();
>+      const nsStyleUserInterface* ui = containerFrame->StyleUserInterface();
>+      if (ui->mUserFocus == NS_STYLE_USER_FOCUS_IGNORE ||
>+          (aContent->IsXUL() && ui->mUserFocus == NS_STYLE_USER_FOCUS_NONE)) {
Here you're checking isXUL of aContent, not containerElement

Comment 74

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #71)
> > Right. Currently all HTML elements override the default by TabIndexDefault() so this is not a problem.
> 
> If is a problem because the patch doesn't match expected behaviour.
> 
> For example, take the following test and tab through the buttons:
> 
> <button id="one" style="-moz-user-focus: ignore;">Ignore</button>
> <button id="two" style="-moz-user-focus: none;">None</button>
> <button id="three" style="-moz-user-focus: ignore;" tabindex="1">Tabbable
> Ignore</button>
> <button id="four" style="-moz-user-focus: none;" tabindex="1">Tabbable
> None</button>
> 
> I expect either:
>  - the existing behaviour where -moz-user-focus is essentially ignored and
> all four buttons are in the tab order.
>  - the XUL behaviour where only buttons three and four are in the tab order.
> 
> Your patch has two, three and four in the tab order, which doesn't make
> sense to me.
> 
> What you should be doing is treating ignore and none the same for
> tabbing/focusability. The distinction is only relevant for the suppressBlur
> stuff in the EventStateManager.

Then how do we actually disable the focusability of a element?

Comment 75

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kan-Ru Chen [:kanru] from comment #74)
> (In reply to Neil Deakin from comment #71)
> > > Right. Currently all HTML elements override the default by TabIndexDefault() so this is not a problem.
> > 
> > If is a problem because the patch doesn't match expected behaviour.
> > 
> > For example, take the following test and tab through the buttons:
> > 
> > <button id="one" style="-moz-user-focus: ignore;">Ignore</button>
> > <button id="two" style="-moz-user-focus: none;">None</button>
> > <button id="three" style="-moz-user-focus: ignore;" tabindex="1">Tabbable
> > Ignore</button>
> > <button id="four" style="-moz-user-focus: none;" tabindex="1">Tabbable
> > None</button>
> > 
> > I expect either:
> >  - the existing behaviour where -moz-user-focus is essentially ignored and
> > all four buttons are in the tab order.
> >  - the XUL behaviour where only buttons three and four are in the tab order.
> > 
> > Your patch has two, three and four in the tab order, which doesn't make
> > sense to me.
> > 
> > What you should be doing is treating ignore and none the same for
> > tabbing/focusability. The distinction is only relevant for the suppressBlur
> > stuff in the EventStateManager.
> 
> Then how do we actually disable the focusability of a element?

OK. I see. Let's make 'ignore' and 'none' both disable the focusability. But what should be the default for HTML elements then?

Comment 76

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

Default should be the case when -moz-user-focus isn't there at all. (-moz-user-focus: normal I think)

Comment 77

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #76)
> Default should be the case when -moz-user-focus isn't there at all.
> (-moz-user-focus: normal I think)

Do you mean to change the initial value for HTML elements to 'normal'? Currently in xul.css we have this rule
 
 * {
   -moz-user-focus: ignore;
   -moz-user-select: none;
   display: -moz-box;
   -moz-box-sizing: border-box;
 }

So the HTML elements in chrome have -moz-user-focus:ignore by default. HTML elements ignore -moz-user-focus so it's not a problem currently. If we make HTML elements behave like XUL then the address bar will not be focusable. Changing the initial value for HTML or not we still have to modify xul.css.

Comment 78

[Neil Deakin]

If you're actually intending to implement -moz-user-focus for all HTML elements, you would need to set the default to 'none', and set it to 'normal' in the HTML stylesheets (for example in layout/style/forms.css) for each element that needs to be focusable.

But I'm assuming your goal is instead just to fix this bug. Maybe you only want to handle -moz-user-focus on html:iframe.

If you remove the aContent->IsXUL() check, your other patch (https://bugzilla.mozilla.org/attachment.cgi?id=802380) works as I think you want it to for xul:iframe.

Comment 79

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #78)
> If you're actually intending to implement -moz-user-focus for all HTML
> elements, you would need to set the default to 'none', and set it to
> 'normal' in the HTML stylesheets (for example in layout/style/forms.css) for
> each element that needs to be focusable.

Right.
 
> But I'm assuming your goal is instead just to fix this bug. Maybe you only
> want to handle -moz-user-focus on html:iframe.

I don't want to make html:iframe a special case :/
 
> If you remove the aContent->IsXUL() check, your other patch
> (https://bugzilla.mozilla.org/attachment.cgi?id=802380) works as I think you
> want it to for xul:iframe.

Yep, removed.

Comment 80

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Neil Deakin from comment #78)
> If you're actually intending to implement -moz-user-focus for all HTML
> elements, you would need to set the default to 'none', and set it to
> 'normal' in the HTML stylesheets (for example in layout/style/forms.css) for
> each element that needs to be focusable.

Actually I think |* { -moz-user-focus: normal }| will do it.

Comment 81

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v6 and tests

Comment 82

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Check if our parent frame is focusable v4

Comment 83

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=5d6a04dc6411

Comment 84

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=d16d7dc61643

Comment 85

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v5.1 and tests

Comment 86

[Kan-Ru Chen [:kanru] (UTC+9)]

Uh-oh.. still have oranges.

Comment 87

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=630281b7153e

Comment 88

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v7

Comment 89

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Check if our parent frame is focusable v5

Comment 90

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v7.1

Move focus rules to html.css

Comment 91

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=d159294fafb2

Comment 92

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=6845925e4b7b finally all green \o/

Comment 93

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Check if our parent frame is focusable v5.1

Comment 94

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix -moz-user-focus in HTML v7.2

Comment 95

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

Comment on attachment 806071 [details] [diff] [review]
Fix -moz-user-focus in HTML v7.2


>+/********** focus rules **********/
>+
>+a, button, frame, iframe, object, plugin, input, select, textarea {
>+  -moz-user-focus: normal;
>+}

So why can't we we have -moz-user-focus: normal; on all the html elements?
And perhaps none if inside xul, except these few cases.
But the fewer special cases in HTML, the better.

Comment 96

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #95)
> Comment on attachment 806071 [details] [diff] [review]
> Fix -moz-user-focus in HTML v7.2
> 
> 
> >+/********** focus rules **********/
> >+
> >+a, button, frame, iframe, object, plugin, input, select, textarea {
> >+  -moz-user-focus: normal;
> >+}
> 
> So why can't we we have -moz-user-focus: normal; on all the html elements?
> And perhaps none if inside xul, except these few cases.
> But the fewer special cases in HTML, the better.

|* { -moz-user-focus: normal }| is a slow selector and it doesn't pass some tests. If we are going to change the default behavior of -moz-user-focus for XUL and HTML, could we do that in a follow up?

Comment 97

[Kan-Ru Chen [:kanru] (UTC+9)]

If I change the initial value of -moz-user-focus to 'auto' and make the XUL default to 'none' and HTML to 'normal', I still have to override the html elements individually because in some of our xbl bindings the html:input element will inherit from their xul parent. eg. https://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#33 That's why the awesomebar became unfocusable.

Comment 98

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

How can move forward this bug here?
Can we find other reviewers for the patch?

This bug blocks keyboard OOP and an important part of getting 3rd-party IME framework into v1.2, which is an important milestone for more components to be interchangeable.

Comment 99

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Rudy Lu [:rudyl] from comment #98)
> How can move forward this bug here?
> Can we find other reviewers for the patch?
> 
> This bug blocks keyboard OOP and an important part of getting 3rd-party IME
> framework into v1.2, which is an important milestone for more components to
> be interchangeable.

I and :smaug decided to take a safer approach that adds 'ignoreuserfocus' to mozbrowser iframe to prevent focus. Given that we all agree what to do, I'll implement that asap.

Comment 100

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Add ignoreuserfocus to mozbrowser iframe

Comment 101

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=86c3a5e7f221

Comment 102

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

Comment on attachment 822277 [details] [diff] [review]
0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Please add tests for the case when iframe contains <input> and .focus() is called. And we need tests for tabindex and explicit focus using mouse.
And I don't see how the patch handles the case when there is
iframe inside ignoreuserfocus iframe.

Comment 103

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Combined patch 1 and patch 2. Added more test cases.

Comment 104

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

Comment on attachment 824434 [details] [diff] [review]
0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

So this changes focusing behavior if iframe is used inside editable content.
And this wouldn't prevent focusing <area>, as far as I see.

nsCOMPtr<nsIContent> containerElement = do_QueryInterface(ownWindow->GetFrameElementInternal());
doesn't need QI.

Comment 105

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #104)
> Comment on attachment 824434 [details] [diff] [review]
> 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch
> 
> So this changes focusing behavior if iframe is used inside editable content.

It does not. We return early if nsGenericHTMLElement::IsHTMLFocusable overrides and editable content is checked by nsGenericHTMLElement.

> And this wouldn't prevent focusing <area>, as far as I see.

Could you explain?

Comment 106

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

(In reply to Kan-Ru Chen [:kanru] from comment #105)
> (In reply to Olli Pettay [:smaug] from comment #104)
> > Comment on attachment 824434 [details] [diff] [review]
> > 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch
> > 
> > So this changes focusing behavior if iframe is used inside editable content.
> 
> It does not. We return early if nsGenericHTMLElement::IsHTMLFocusable
> overrides and editable content is checked by nsGenericHTMLElement.
Now I can't recall what I was thinking here. Your comment looks valid :)


> > And this wouldn't prevent focusing <area>, as far as I see.
> 
> Could you explain?
See the code in nsFocusManager right before your changes.

Comment 107

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Move the parent test before the area test and add test case for area.

Comment 108

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

Comment on attachment 825377 [details] [diff] [review]
0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch


>+++ b/content/html/content/test/file_ignoreuserfocus.html
>@@ -0,0 +1,10 @@
>+<!DOCTYPE HTML>
>+<html>
>+  <body>
>+    <map name=a>
>+      <area shape=rect coords=25,25,75,75 href=#fail>
>+    </map>
>+    <img usemap=#a src=image.png>
>+    <input><iframe>
Nit, IIRC <iframe> should be closed with </iframe>



>+        var iframe = document.createElement("iframe");
>+        var privilegedIframe = SpecialPowers.wrap(iframe);
>+        privilegedIframe.setAttribute("mozbrowser", "true");
>+        privilegedIframe.setAttribute("ignoreuserfocus", "true");
>+        privilegedIframe.setAttribute("height", "500px");
I don't understand the need for privilegedIframe.
If our principal has the permission for create browser frames and we have the API enabled, this should just
work without privilegedIframe. iframe should be enough.


>+        iframe.src = "file_ignoreuserfocus.html";
>+
>+        iframe.addEventListener('load', function (e) {
>+          // Yield so the inner iframe could finish the setup
>+          SimpleTest.executeSoon(function () {
>+            // Sanity check
>+            witness.focus();
>+            is(document.activeElement, witness, "witness should have the focus");
>+
>+            iframe.focus();
>+            is(document.activeElement, witness, "[explicit iframe.focus()] iframe shouldn't get the focus");
Oh, this is not what I described in W3C bug. I thought we'd allow explicit focus, but focusing using mouse or 
tabbing wouldn't work. Web page would need to effectively opt-in to those.


>+
>+            privilegedIframe.setAttribute("ignoreuserfocus", "true");
>+
>+            // Test the case when iframe contains <input> and .focus()
>+            // is called and explicit focus using mouse
>+            var iframeWindow = SpecialPowers.unwrap(iframe.contentWindow);
>+
>+            witness.focus();
>+            is(document.activeElement, witness, "witness should have the focus");
>+
>+            var innerInput = privilegedIframe.contentDocument.getElementsByTagName("input")[0];
>+            innerInput.focus();
>+            is(document.activeElement, witness, "[explicit innerInput.focus()] witness should have the focus");
Now I'm lost. explicit .focus() does work in some cases...
This way the setup is inconsistent. Either .focus() shouldn't work ever on or inside ignoreuserfocus iframe, or it should work
always.


this doesn't seem to quite give us the expected behavior. Feel free to disagree, but I'd like to have
such behavior which can be exposed to the web, and not b2g only API.
(We'd just implement it first for b2g only)

Comment 109

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Now calling .focus on ignoreuserfocus iframe or the elements inside the iframe will focus the iframe. Neither direct mouse click nor tab key could focus the iframe. Blur is suppressed if the ancestors has ignoreuserfocus.

Comment 110

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

Comment on attachment 825782 [details] [diff] [review]
0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Why can't you check in EventStateManager that whether the frame is
CheckIfFocusable ?
I'd prefer keeping the focus handling in focusmanager and in
element implementations as much as possible.

Comment 111

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

Comment on attachment 825782 [details] [diff] [review]
0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch


>+          if (!suppressBlur) {
>+            // Check if our mozbrowser iframe ancestors suppress blur.
>+            nsCOMPtr<nsPIDOMWindow> ownWindow = activeContent->OwnerDoc()->GetWindow();
nsPIDOMWindow* ownWindow

>+            while (ownWindow) {
>+              nsCOMPtr<nsIContent> frameElement = ownWindow->GetFrameElementInternal();
Element* frameElement


>+              if (frameElement) {
>+                nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(frameElement);
>+                if (browserFrame && browserFrame->GetReallyIsBrowserOrApp() &&
>+                    frameElement->HasAttr(kNameSpaceID_None, nsGkAtoms::ignoreuserfocus)) {
>+                  suppressBlur = true;
break;

>+                }
>+                ownWindow = frameElement->OwnerDoc()->GetWindow();
>+              } else {
>+                ownWindow = nullptr;
just break; here.


>+
>+        iframe.addEventListener('load', function (e) {
>+          // Wait for first paint to setup frames
>+          var privilegedIframe = SpecialPowers.wrap(iframe);
>+          privilegedIframe.contentWindow.addEventListener("MozAfterPaint", function afterPaint(e) {
>+            privilegedIframe.contentWindow.removeEventListener("MozAfterPaint", afterPaint);
I don't see what privilegedIframe gives you. Just using iframe should work.



>+  // Recursively check if our parent is focusable.
>+  nsCOMPtr<nsPIDOMWindow> ownWindow = aContent->OwnerDoc()->GetWindow();
>+  if (ownWindow) {
>+    nsCOMPtr<nsIContent> containerElement = ownWindow->GetFrameElementInternal();
>+    if (containerElement && !CheckIfFocusable(containerElement, aFlags)) {
>+      return nullptr;
>+    }
>+  }
I became worried about IsSubDocumentTabbable here and also the editable stuff.
I guess we should have similar check here as in EventStateManager.
That way this would be least regression risky.

Could you make a helper method to do ignoreuserfocus in parent iframe chain and use that in EventStateManager and in
nsFocusManager

Comment 112

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #111)
> >+        iframe.addEventListener('load', function (e) {
> >+          // Wait for first paint to setup frames
> >+          var privilegedIframe = SpecialPowers.wrap(iframe);
> >+          privilegedIframe.contentWindow.addEventListener("MozAfterPaint", function afterPaint(e) {
> >+            privilegedIframe.contentWindow.removeEventListener("MozAfterPaint", afterPaint);
> I don't see what privilegedIframe gives you. Just using iframe should work.

I can't access iframe.contentWindow.addEventListener without chrome privilege because mozbrowser iframe has a different principal with its parent.

Comment 113

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Still need to touch nsGenericHTMLFrameElement::IsHTMLFocusable for deciding tabindex order.

Comment 114

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-847763-Add-ignoreuserfocus-attribute-for-mozbrow.patch

Addresses comment from IRC. Remove the tab handling part first.

Comment 115

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

Attachment: more strict ignoreuserfocus

This disables focus() in ignoreuserfocus context.

Fixes also few bugs. The previous patch for example dispatched blur events
to iframe.contentDocument even if it had ignoreuserfocus. Added a test for that.
This also makes sure the
performance cost of this feature is close to nil when 
dom.mozBrowserFramesEnabled is false. (it is false on desktop).

The patch renames nsIContent::IsFocusable to nsIFocusableInternal and
adds a new nsIContent::IsFocusable which takes care of checking ignoreuserfocus.
For performance reasons the check is done only if IsFocusedInternal would otherwise return true.
So in common case even in b2g IsFocusable() stays pretty fast.
The change to IsFocusable() is good for the future too, since it let's us do
global changes quite easily, and we should perhaps merge IsFocusableInternal and IsHTMLFocusable -
but in a followup. We need this bug fixed asap, I've been told.

I'm not super happy with the nsFocusManager changes, but those were the easiest way to achieve
consistent behavior. (nsFocusManager could use some cleanups) 


Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=92d114c92d4b
and will ask feedback and review once I have result.

Kanru, feel free to comment even before that :)

Comment 116

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

Fun, the testcase fails on
B2G ICS Emulator Opt

Comment 117

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

Looks like synthesizeMouseAtCenter(); doesn't work quite the same way on b2g.

Comment 118

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

Comment on attachment 828637 [details] [diff] [review]
more strict ignoreuserfocus

I'm still investigating those test failures, and I have two plans to fix.
One should certainly work, but another option would be better. But not changes
to the actual patch should be required.

Comment 119

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

Attachment: test fixes v1

https://tbpl.mozilla.org/?tree=Try&rev=6df04e36186f
Simpler fixes

Comment 120

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

Attachment: test fixes v2

https://tbpl.mozilla.org/?tree=Try&rev=da66981f9943

Using sendMouseEventToWindow, not sendMouseEvent

Comment 121

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

v2 which has better test fixes seems to pass.
But triggered the test still few more times.

Comment 122

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 828637 [details] [diff] [review]
more strict ignoreuserfocus

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

Looks good to me.

Comment 123

[Neil Deakin]

Comment on attachment 828637 [details] [diff] [review]
more strict ignoreuserfocus

The callers of IsUserFocusIgnored seem inconsistent. In EventStateManager.cpp, it will be called with any node, meaning it will prevent blurs no matter what is clicked on. But in the focus manager you only call it on document root nodes, and not at all on other nodes. In SendFocusOrBlurEvent, you call it on an nsIDocument.

Comment 124

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

those root node cases are there because we special case root there. We call IsFocusable on other nodes.

SendFocusOrBlurEvent could indeed check the target and not document.

Comment 125

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

Well, SendFocusOrBlurEvent needs to check both since target can be nsPIDOMWindow.


  nsCOMPtr<nsINode> n = do_QueryInterface(aTarget);
  if (!n) {
    nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aTarget);
    n = win ? win->GetExtantDoc() : nullptr;
  }
  bool dontDispatchEvent = n && nsContentUtils::IsUserFocusIgnore(n);

  would be more correct. And the 'if' should still have check for aDocument to be non-null.

Comment 126

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

Would that be enough for you?

Comment 127

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

Attachment: v3

Comment 128

[Kan-Ru Chen [:kanru] (UTC+9)]

Reassign to :smaug.

Comment 129

[Preeti Raghunath(:Preeti)]

Ivan

Any reason why this is a blocker? 3rd party keyboard shouldn't impact us right?

Comment 130

[Ivan Tsay (:ITsay)]

This is definitely a blocker and this is the platform work to prevent from losing focus on keyboard. It is also needed for OOP to work in 3rd party keyboard so that 3rd party keyboard won't crash out process.

Comment 131

[Ivan Tsay (:ITsay)]

Hi Olli,

Thank you for taking this bug. I wonder why it takes so long for back and forth on the review for this bug in the past weeks. Withe very tight schedule for v1.2 release, we need this one to be fixed before 11/22 so that our QA have two more weeks for the testing on this one. Let me know if you have any question.

Comment 132

[[:fabrice] Fabrice Desré]

Because it's a hard bug to fix? I'm quite puzzled by your question Ivan...

Comment 133

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yeah, let's have some more faith in :smaug.

Comment 134

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

Oh, Neil is on vacation :/

Trying to find another reviewer...

Comment 135

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

Comment on attachment 829768 [details] [diff] [review]
v3

Jst, you could perhaps review this.

I'm trying to be super conservative here, and the feature
is exposed in b2g only. In case the feature is not enabled, affect to
performance shouldn't be noticeable (and even in b2g perf shouldn't be too bad).

Comment 136

[Kai-Chih Hu [:khu]]

Since 1.2 C4 was passed, could you assign a new target milestone that you're trying to hit?

Comment 137

[Preeti Raghunath(:Preeti)]

Tim,

We blocked on this issue for security concerns. But comment 135 tends to believe that the performance drop should be small.

Comment 138

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Preeti Raghunath(:Preeti) from comment #137)
> Tim,
> 
> We blocked on this issue for security concerns. But comment 135 tends to
> believe that the performance drop should be small.

Hum, how comment 135 contrary to that? This bug has always been about enabling keyboard frame being able to run out-of-process.

Comment 139

[Ivan Tsay (:ITsay)]

Reassign target milestone

Comment 140

[Johnny Stenback (:jst)]

Comment on attachment 829768 [details] [diff] [review]
v3

Looks good, r=jst

Comment 141

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e864bbc290f5

Comment 142

[Kan-Ru Chen [:kanru] (UTC+9)]

\o/

Comment 143

[Jason Smith [:jsmith]]

Renoming per discussion on bug 940879 - we're going to discuss in detail if this should ride the trains or not.

Comment 144

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Smith [:jsmith] from comment #143)
> Renoming per discussion on bug 940879 - we're going to discuss in detail if
> this should ride the trains or not.

Please read my bug 940879 comment 9 for suggestions to move forward.

Comment 145

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e864bbc290f5

Comment 146

[Jason Smith [:jsmith]]

Moving back to koi+ per discussion on comment 11 of bug 940879.

Comment 147

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/62592de364d0