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.

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.

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?

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

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

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

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

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.

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?

(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.

(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.

(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?

(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.

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.

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.

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

(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.

(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?

(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.

+1 for new argument.

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.

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

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

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

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.

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

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; }

(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

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

(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.

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.

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

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

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

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 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.

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.

> 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.

(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.

> > 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.

(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#)

(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?

(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.

carry over r+

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)"?= > <kanru@kanru.info> >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 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?

(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.

(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.

Update keyword in white board for the status query more precise

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

I don't know how to create a !IsSubDocumentTabbable iframe :/ More tests could be easily added to that file.

Yeah, !IssubDocumentTabbable is tricky. But tests for other stuff is needed. I'll try to review during this weekend.

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">

Thanks for the review! https://tbpl.mozilla.org/?tree=Try&rev=a5e44eefeb22

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.

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.

(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

(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 ?

(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.

(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.

(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

> 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.

(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.

(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?

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

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

> 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 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 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

(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?

(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?

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

(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.

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.

(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.

(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.

Uh-oh.. still have oranges.

Move focus rules to html.css

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

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.

(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?

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.

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.

(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.

Add ignoreuserfocus to mozbrowser iframe

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.

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

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.

(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?

(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.

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

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)

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 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 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

(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.

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

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

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 :)

Fun, the testcase fails on B2G ICS Emulator Opt

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

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.

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

https://tbpl.mozilla.org/?tree=Try&rev=da66981f9943 Using sendMouseEventToWindow, not sendMouseEvent

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

Comment on attachment 828637 [details] [diff] [review] more strict ignoreuserfocus Review of attachment 828637 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

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.

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.

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.

Would that be enough for you?

Reassign to :smaug.

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

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.

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.

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

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

Oh, Neil is on vacation :/ Trying to find another reviewer...

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).

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

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

(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.

Reassign target milestone

Comment on attachment 829768 [details] [diff] [review] v3 Looks good, r=jst

\o/

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

(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.

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