Closed Bug 1440189 Opened 6 years ago Closed 6 years ago

Stop dispatching keypress event on web content in Nightly

Categories

(Core :: DOM: Events, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(1 file, 2 obsolete files)

      No description provided.
Keywords: site-compat
Priority: -- → P2
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
Attachment #8954657 - Flags: review?(bugs) → review-
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?
Attachment #8954658 - Flags: review?(bugs) → review-
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
Attachment #8954657 - Flags: review?(bugs) → review+
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
Attachment #8954658 - Flags: review?(bugs) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1442528
Depends on: 1442546
Depends on: 1442847
No longer depends on: 1442847
Depends on: 1443220
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.
Flags: needinfo?(masayuki)
(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?
Status: RESOLVED → REOPENED
Flags: needinfo?(masayuki) → needinfo?(cpeterson)
Resolution: FIXED → ---
(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. :)
Flags: needinfo?(cpeterson)
(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.
Attachment #8954657 - Attachment is obsolete: true
Attachment #8954658 - Attachment is obsolete: true
This is a bit of a complicated one to document, given changes, reverts, etc. - what has actually happened here? 

Thanks in advance ;-)
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
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?
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
That might make sense, at least on Nightly.
Perhaps we should blacklist *.google.com
Flags: needinfo?(bugs)
(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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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 ..."
Attachment #8966197 - Flags: review?(bugs) → review-
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.
Attachment #8966197 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/2ab582bacc98
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla60 → mozilla61
Depends on: 1454262
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.
Flags: needinfo?(bugs) → needinfo?(imyxhuang)
(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.
Flags: needinfo?(imyxhuang)
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.
Depends on: 1455059
This bug breaks Etherpad, see bug 1455059
Depends on: 1455224
Depends on: 1456038
Depends on: 1456444
Depends on: 1457247
please see bug 1461085, this regresses keyboard navigation within the RAP tree widget.
Blocks: 1461085
No longer blocks: 1461085
Depends on: 1461085
Depends on: 1473705
Depends on: 1483553
Depends on: 1490410
Depends on: 1524956
Depends on: 1529570
Depends on: 1529671
No longer depends on: 1529671
Depends on: 1537913
Regressions: 1729653
You need to log in before you can comment on or make changes to this bug.