Comment on attachment 8954657 [details] Bug 1440189 - part 1: Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) https://reviewboard.mozilla.org/r/223752/#review229844 ::: modules/libpref/init/all.js:221 (Diff revision 1) > pref("dom.gamepad.haptic_feedback.enabled", true); > > // If this is true, TextEventDispatcher dispatches keydown and keyup events > // even during composition (keypress events are never fired during composition > // even if this is true). > +#ifdef EARLY_BETA_OR_EARLIER could you explain how this is related to this bug? ::: modules/libpref/init/all.js:230 (Diff revision 1) > +#endif > > // If this is true, TextEventDispatcher dispatches keypress event with setting > // WidgetEvent::mFlags::mOnlySystemGroupDispatchInContent to true if it won't > // cause inputting printable character. > -pref("dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content", false); > +pref("dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content", true); Per the email to dev.platform, don't you want this inside ifdef EARLY_BETA_OR_EARLIER

Comment on attachment 8954658 [details] Bug 1440189 - part 2: Make test_assign_event_data.html aware of strict keypress event dispatching mode https://reviewboard.mozilla.org/r/223754/#review229846 ::: widget/tests/test_assign_event_data.html:85 (Diff revision 1) > + SpecialPowers.getBoolPref("dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content"); > + > const kIsMac = (navigator.platform.indexOf("Mac") == 0); > const kIsWin = (navigator.platform.indexOf("Win") == 0); > > +var gDescription = ""; What is this gDescription? It is always just "", no?

Comment on attachment 8954657 [details] Bug 1440189 - part 1: Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) https://reviewboard.mozilla.org/r/223752/#review229844 > could you explain how this is related to this bug? Ah, sorry. It's just mistake. I added #ifdef to the wrong position (because I rewrite this from patch for bug 968056).

Comment on attachment 8954658 [details] Bug 1440189 - part 2: Make test_assign_event_data.html aware of strict keypress event dispatching mode https://reviewboard.mozilla.org/r/223754/#review229846 > What is this gDescription? It is always just "", no? Oh, right. I added unnecessary |var| at the bottom function.

Comment on attachment 8954657 [details] Bug 1440189 - part 1: Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) https://reviewboard.mozilla.org/r/223752/#review229954

Comment on attachment 8954658 [details] Bug 1440189 - part 2: Make test_assign_event_data.html aware of strict keypress event dispatching mode https://reviewboard.mozilla.org/r/223754/#review229956

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/741e9659cd4c part 1: Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) r=smaug https://hg.mozilla.org/integration/autoland/rev/eb86519cc44f part 2: Make test_assign_event_data.html aware of strict keypress event dispatching mode r=smaug

https://hg.mozilla.org/mozilla-central/rev/741e9659cd4c https://hg.mozilla.org/mozilla-central/rev/eb86519cc44f

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/non-printable-keys-will-soon-stop-firing-keypress-event/

Masayuki, instead of disabling this pref on Nightly, can we add a site whitelist so (only) google.com temporarily receives the old keypress behavior? This would allow Nightly users to keep testing the new keypress behavior on other sites while we wait for Google to fix Google Docs.

(In reply to Chris Peterson [:cpeterson] from comment #17) > Masayuki, instead of disabling this pref on Nightly, can we add a site > whitelist so (only) google.com temporarily receives the old keypress > behavior? This would allow Nightly users to keep testing the new keypress > behavior on other sites while we wait for Google to fix Google Docs. If we do that, cannot Google test Firefox with new behavior?

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18) > (In reply to Chris Peterson [:cpeterson] from comment #17) > > Masayuki, instead of disabling this pref on Nightly, can we add a site > > whitelist so (only) google.com temporarily receives the old keypress > > behavior? This would allow Nightly users to keep testing the new keypress > > behavior on other sites while we wait for Google to fix Google Docs. > > If we do that, cannot Google test Firefox with new behavior? Good question. Perhaps we could store the whitelist as a comma-separated list of domains in a user-editable pref? Google engineers could just remove google.com from the pref when they are testing. This would also allow other people to whitelist other broken domains that may block them from using Nightly. Or a simpler solution: Google engineers could just test with one of last week's Nightly builds that had the new behavior enabled for all sites. :)

(In reply to Chris Peterson [:cpeterson] from comment #19) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18) > > (In reply to Chris Peterson [:cpeterson] from comment #17) > > > Masayuki, instead of disabling this pref on Nightly, can we add a site > > > whitelist so (only) google.com temporarily receives the old keypress > > > behavior? This would allow Nightly users to keep testing the new keypress > > > behavior on other sites while we wait for Google to fix Google Docs. > > > > If we do that, cannot Google test Firefox with new behavior? > > Good question. Perhaps we could store the whitelist as a comma-separated > list of domains in a user-editable pref? Google engineers could just remove > google.com from the pref when they are testing. This would also allow other > people to whitelist other broken domains that may block them from using > Nightly. Hmm, sound like it's possible, but needs some complicated code and needs new member in InputContext. I'll try to keep it in mind. Anyway, I'm trying to fix bug 354358 which should be released at same release.

Since apparently, the Google Closure library is causing this, it doesn't seem very effective to whitelist only Google given the number of websites potentially using it.

This is a bit of a complicated one to document, given changes, reverts, etc. - what has actually happened here? Thanks in advance ;-)

Currently, the new behavior is still disabled even in Nightly channel due to waiting Google's fix, we're still discussing when we enable the new behavior.

I tried to create a patch to enable new behavior in nightly but disabling it with blacklist of URI. (comment 25) Patch: https://hg.mozilla.org/try/rev/3946adb557ba1037a53aac8387dc1996efd31b28 Do you think that we should take this approach to test new behavior on other websites?

That might make sense, at least on Nightly. Perhaps we should blacklist *.google.com

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #27) > That might make sense, at least on Nightly. > Perhaps we should blacklist *.google.com Hmm, but one of the motivation to fix this bug is, making Firefox for Android tire-1 browser on Google Search. Do you think that we should use *.google.com only on desktop?

I think the idea of blacklisting Google properties was simply to get more reports from our Nightly users while Google implements a fix for their properties. I don't think we want to ship to release with Google in the blacklist since that would suggest that the underlying problem in closure has not been fixed and other non-Google properties are still likely to be affected.

(In reply to Brian Birtles (:birtles) from comment #29) > I think the idea of blacklisting Google properties was simply to get more > reports from our Nightly users while Google implements a fix for their > properties. Exactly. > I don't think we want to ship to release with Google in the blacklist since > that would suggest that the underlying problem in closure has not been fixed > and other non-Google properties are still likely to be affected. Yeah, I don't want to use it in release channel especially due to performance reason. However, some major web apps keeping using current or older Closure even after our release... I don't know how much major web apps use Closure now, though.

Yeah, I just wanted to get more feedback from non-Google web sites, since we already know that several Google sites have broken keyboard handling. This isn't anything I'd be ready to ship. The blacklisting should be Nightly only.

Okay, I wrapped all changes in PresShell with #ifdef to restricting only to Nightly and early Beta. And I still do not use *.google.com in the blacklist but if you think that we should use "*" for subdomain even with Nightly for Android, let me know.

I think we want this on nightly only, for now.

Comment on attachment 8966197 [details] Bug 1440189 - Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) unless web page isn't in blacklist https://reviewboard.mozilla.org/r/234944/#review240794 ::: layout/base/PresShell.h:878 (Diff revision 1) > bool mHasHandledUserInput : 1; > > +#ifdef EARLY_BETA_OR_EARLIER > + // Whether we should dispatch keypress events even for non-printable keys > + // for keeping backward compatibility. > + bool mForceDispatchKeyPressEventsForNonPrinableKeys : 1; ...NonPrintableKeys ::: layout/base/PresShell.h:880 (Diff revision 1) > +#ifdef EARLY_BETA_OR_EARLIER > + // Whether we should dispatch keypress events even for non-printable keys > + // for keeping backward compatibility. > + bool mForceDispatchKeyPressEventsForNonPrinableKeys : 1; > + // Whether mForceDispatchKeyPressEventsForNonPrinableKeys is initialized. > + bool mInitializedForceDispatchKeyPressEventsForNonPrinableKeys : 1; same here, not NonPrin, but NonPrint ::: layout/base/PresShell.cpp:831 (Diff revision 1) > , mHasCSSBackgroundColor(false) > , mScaleToResolution(false) > , mIsLastChromeOnlyEscapeKeyConsumed(false) > , mHasReceivedPaintMessage(false) > , mHasHandledUserInput(false) > +#ifdef EARLY_BETA_OR_EARLIER So, I think this should be nightly only for now. ::: layout/base/PresShell.cpp:7931 (Diff revision 1) > + // even with breaking the standard. > + if (!mInitializedForceDispatchKeyPressEventsForNonPrinableKeys) { > + mInitializedForceDispatchKeyPressEventsForNonPrinableKeys = true; > + nsPresContext* presContext = GetPresContext(); > + nsIDocument* document = presContext ? presContext->Document() : nullptr; > + nsIURI* uri = document ? document->GetDocumentURIObject() : nullptr; This doesn't quite work. Document can be coming for example from docs.google.com, yet its document uri can be about:blank, as an example. It would be better to get the uri from the principal of the document. Though, that wouldn't catch the case when some sandboxed document or data: document or such is used in an iframe. Perhaps if principal is nullprincipal, one should try to look at the parent document to find principal with proper uri. (I've seen Google websites using about:blank documents at least) ::: modules/libpref/init/all.js:230 (Diff revision 1) > #endif > > // If this is true, TextEventDispatcher dispatches keypress event with setting > // WidgetEvent::mFlags::mOnlySystemGroupDispatchInContent to true if it won't > // cause inputting printable character. > +#ifdef EARLY_BETA_OR_EARLIER here too, I think I'd prefer nightly only initially. ::: modules/libpref/init/all.js:234 (Diff revision 1) > // cause inputting printable character. > +#ifdef EARLY_BETA_OR_EARLIER > +pref("dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content", true); > +// Blacklist of domains of web apps which are not aware of strict keypress > +// dispatching behavior. This is comma separated list. If you need to match > +// all sub-domains, you can specify as "*.example.com". Additionally, you "you can specify it as ..."

Comment on attachment 8966197 [details] Bug 1440189 - Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) unless web page isn't in blacklist https://reviewboard.mozilla.org/r/234944/#review240794 > This doesn't quite work. Document can be coming for example from docs.google.com, yet its document uri can be about:blank, as an example. > It would be better to get the uri from the principal of the document. > Though, that wouldn't catch the case when some sandboxed document or data: document or such is used in an iframe. Perhaps if principal is nullprincipal, one should try to look at the parent document to find principal with proper uri. > (I've seen Google websites using about:blank documents at least) Really interesting case. I (probably) understand about the case of data: document. However, I don't understand about the case, the document URI becomes aboult:blank. Do you know how to create such case? (Anyway, I rewrite the patch with nsIPrincipal.)

Just add <iframe> to a document. No need to set its src or anything, and its url is about:blank by default. And for some reason some Google Suite apps, I think it was Spreadsheet at least, was/is using about:blank iframes for something. Basically the parent document injected some script to iframe and run it there. I don't know why.

Comment on attachment 8966197 [details] Bug 1440189 - Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) unless web page isn't in blacklist https://reviewboard.mozilla.org/r/234944/#review242242 ::: layout/base/PresShell.cpp:7878 (Diff revision 2) > + Preferences::GetCString(kPrefNameOfBlackList, blackList); > + if (blackList.IsEmpty()) { > + return false; > + } > + > + for (;;) { I'm sure we have some helper method to split , separated list, but can't recall now where. And perhaps it wouldn't really simplify this.

Comment on attachment 8966197 [details] Bug 1440189 - Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) unless web page isn't in blacklist https://reviewboard.mozilla.org/r/234944/#review242242 > I'm sure we have some helper method to split , separated list, but can't recall now where. And perhaps it wouldn't really simplify this. Yeah. And also user runs this path at first non-printable key press. So, I'd make it faster as far as possible. Perhaps, utilities need to create array of comma separated items, if so, I worry about the heap allocation cost.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 0f54007bf4b43dcc5996b717d6863d76a71156a0 -d 1e6febc9f5af: rebasing 458007:0f54007bf4b4 "Bug 1440189 - Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) unless web page isn't in blacklist r=smaug" (tip) merging layout/base/PresShell.cpp merging modules/libpref/init/all.js warning: conflicts while merging layout/base/PresShell.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/2ab582bacc98 Stop dispatching keypress event to the default event group in web content (only Nightly and early Beta) unless web page isn't in blacklist r=smaug

Er, mail.google.com still doesn't receive keyboard shortcuts for me with Nightly 61.0a1 (2018-04-16) (64-bit) on Ubuntu.

(In reply to imyxhuang from comment #47) > Er, mail.google.com still doesn't receive keyboard shortcuts for me with > Nightly 61.0a1 (2018-04-16) (64-bit) on Ubuntu. Could you file a bug with STR? I don't see any regression on mail.google.com with testing briefly.

Updated the site compat note: https://www.fxsitecompat.com/en-CA/docs/2018/non-printable-keys-will-soon-stop-firing-keypress-event/

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #48) > (In reply to imyxhuang from comment #47) > > Er, mail.google.com still doesn't receive keyboard shortcuts for me with > > Nightly 61.0a1 (2018-04-16) (64-bit) on Ubuntu. > > Could you file a bug with STR? I don't see any regression on mail.google.com > with testing briefly. Okay, I just tried this with the latest tarball of Nightly and filed Bug 1454833.

Haha, I forgot to enable keyboard shortcuts for Gmail in the settings; the real problem is that the Hangouts sub-documents don't have shortcuts working (like Ctrl+B to bold text) and that was what made me suspect Gmail. I would suggest adding Hangouts to the blacklist, but at some point we would end up making allowances for most Google sites which would be a lot more annoying (though better practice) than using a wildcard for sub-domains. What about a user-configurable .txt in the Firefox installation folder? Could that cause security issues?

(In reply to imyxhuang from comment #51) > Haha, I forgot to enable keyboard shortcuts for Gmail in the settings; the > real problem is that the Hangouts sub-documents don't have shortcuts working > (like Ctrl+B to bold text) and that was what made me suspect Gmail. Ah, okay, thanks. > I would suggest adding Hangouts to the blacklist, but at some point we would > end up making allowances for most Google sites which would be a lot more > annoying (though better practice) than using a wildcard for sub-domains. > What about a user-configurable .txt in the Firefox installation folder? > Could that cause security issues? No, the reason why we cannot use *.google.com is, this behavior change request comes from a team of Google. So, we need to enable new behavior on such apps.

This bug breaks Etherpad, see bug 1455059

please see bug 1461085, this regresses keyboard navigation within the RAP tree widget.