Closed Bug 1536453 Opened 5 months ago Closed 5 months ago

Any computer upgraded to Firefox 66 (latest) (Mac or PC) if you log into Office 365 and use Powerpoint the text in boxes automatically vanishes after typing. You cannot add any text to text boxes.

Categories

(Core :: DOM: Events, defect, P1, major)

66 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 blocking fixed
firefox67 + verified
firefox68 --- fixed

People

(Reporter: tbuxton, Assigned: denschub)

References

Details

(Keywords: regression, site-compat)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

Update Firefox to latest (66)
Open Microsoft 365 online.
Open PowerPoint
Try to type anything anywhere.

Actual results:

Text vanishes forever

Expected results:

Text appears in box like normal

this is reproducible & got introduced with bug 1502795.

Blocks: 1502795
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Flags: needinfo?(masayuki)
Product: Firefox → Core
Version: 64 Branch → 66 Branch

Can we fix this via Normandy on release?

Adding Mythmon for Normandy mitigation comment. Looking at the patch the regressed it, it may not be fixable via a Normandy recipe since it's not as simple as a pref-flip.

Adding Adam to see if there is a webcompat impact and/or mitigation that is possible for this.

Flags: needinfo?(mcooper)
Flags: needinfo?(astevenson)

there is the dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode pref which is supposed to get specified domains back to the legacy behaviour afaik

Not something we can address with GoFaster. Let's see what Masayuki says about using this pref to whitelist the site for now.

Flags: needinfo?(astevenson)

If there is a pref to handle this, Normandy can set that pref to a value. However, since the pref is a list of sites we'd have some complication. We can only set the entire pref to a particular value; we can't append a value to it. I think that is still doable as long as these kind of interventions are fairly rare.

Flags: needinfo?(mcooper)

(In reply to [:philipp] from comment #4)

there is the dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode pref which is supposed to get specified domains back to the legacy behaviour afaik

Right. If we disable it again completely, we need to disable Window.event and Event.returnValue again, that causes breaking a lot of web sites again. So, I recommend to use the blacklist pref if it's reproducible only in specific domain. Otherwise, we need to add new hack in the path added by bug 1514940.

(Unfortunately, I don't have Office 365 account.)

Flags: needinfo?(masayuki)

Thanks Masayuki, sending Office 365 credentials to you now.

common.min.js:

function sn(e) {
var t = e.keyCode;
return 'charCode' in e ? 0 === (e = e.charCode) && 13 === t && (e = 13)  : e = t,
10 === e && (e = 13),
32 <= e || 13 === e ? e : 0
}

dn = Bt.extend({
key: function (e) {
  if (e.key) {
    var t = ln[e.key] || e.key;
    if ('Unidentified' !== t) return t
  }
  return 'keypress' === e.type ? 13 === (e = sn(e)) ? 'Enter' : String.fromCharCode(e)  : 'keydown' === e.type || 'keyup' === e.type ? cn[e.keyCode] || 'Unidentified' : ''
},
location: null,
ctrlKey: null,
shiftKey: null,
altKey: null,
metaKey: null,
repeat: null,
locale: null,
getModifierState: Ht,
charCode: function (e) {
  return 'keypress' === e.type ? sn(e)  : 0
},
keyCode: function (e) {
  return 'keydown' === e.type || 'keyup' === e.type ? e.keyCode : 0
},
which: function (e) {
  return 'keypress' === e.type ? sn(e)  : 'keydown' === e.type || 'keyup' === e.type ? e.keyCode : 0
}
}),

MicrosoftAjax.js

if (b === 'keypress') this.charCode = a.charCode || a.keyCode;
 else if (a.keyCode && a.keyCode === 46) this.keyCode = 127;
 else this.keyCode = a.keyCode;

Looks like that keyCode of keypress events are treated correctly. Do we receive different code from the domain?

I believe Makoto was able to reproduce some odd behavior when typing in Powerpoint using Nightly.

Priority: -- → P1

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(PTO: 3/21-3/22) from comment #9)

Hmm, oddly, I cannot reproduce this with Release nor Nightly...

  • Step
  1. Open new PPT in PowerPoint online
  2. Input 'aaaa' in title
  3. set focus to sub title
  • Result
    When setting focus to sub title, 'aaaa' in title is removed.

I was able to reproduce the issue with Release 66 and Nightly on MacOS. Release 65 is working as expected.

We can definitely consider taking a patch for a 66 dot release (likely next week - not this week.)
Masayuki, is this something you can work on, or can you suggest a possible owner?

Flags: needinfo?(masayuki)

I can reproduce the issue. PowerPoint itself seems to be a giant iFrame to powerpoint.officeapps.live.com for both the Free/Personal Office 365 (which runs under at onedrive.live.com) and the enterprise one (which seem to be subdomains of sharepoint.com).

Setting the dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode preef to powerpoint.officeapps.live.com fixes the issue for me. It may be wise to use *.officeapps.live.com, though, because other Office 365 apps might be broken in some way as well, and/or ping Microsoft to check if there are other domains as well.

Tania, can you help find someone from QA to test other Office 365 apps for similar issues? That would also help in our assessment of whether or not this should block unthrottling the release.

Flags: needinfo?(tmaity)

Thanks Dennis. I reached out to Microsoft to ask about additional domains.

Then is this something we can ship through the blacklist pref and not have to include it in a dot release?

Talking with Adam, Benson, and Dennis now, what I'd like us to do if we can do it quickly, is ship this fix via Normandy with just the powerpoint domain (since we have no evidence of other sites breaking). If we can do that tomorrow, then we will be clear to unthrottle the 66 release and ship it to 100% of the release channel population.

That would be very helpful for any responses to security issues that we will need to make by Friday i.e. responding to the pwn2own contest entries.

(In reply to Dennis Schubert [:denschub] from comment #15)

Setting the
dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode preef to
powerpoint.officeapps.live.com fixes the issue for me.

Does this require a restart to take effect or is it immediate?

Does this require a restart to take effect or is it immediate?

Reloading the tab is enough.

:smaug, I've r?'ed you on a patch here, and it would be great if you could have a look at that soon. We're working on getting the pref changed for release users via Normandy, but we need a patch for Nightly and Beta. Since you reviewed the original patch, it seems fitting. :)

Flags: needinfo?(bugs)

(In reply to Dennis Schubert [:denschub] from comment #15)

I can reproduce the issue. PowerPoint itself seems to be a giant iFrame to powerpoint.officeapps.live.com for both the Free/Personal Office 365 (which runs under at onedrive.live.com) and the enterprise one (which seem to be subdomains of sharepoint.com).

I tested under sharepoint.com, but the <iframe> is in powerpoint.officeapps.live.com. But oddly, I still cannot reproduce this...

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #19)

Talking with Adam, Benson, and Dennis now, what I'd like us to do if we can do it quickly, is ship this fix via Normandy with just the powerpoint domain (since we have no evidence of other sites breaking).

I agree with that. If we'd include other subdomains, it could break something which worked currently.

Flags: needinfo?(masayuki)
Attachment #9052453 - Attachment description: Bug 1536453 - Use legacy keyCode and charCode for powerpoint.officeapps.live.com. r=smaug → Bug 1536453 - Use legacy keyCode and charCode for powerpoint.officeapps.live.com. r=masayuki

Thanks for the review, Masayuki. Let's remove smaug's request then, and get this into Nightly.

Assignee: nobody → dschubert
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/342c28406c95
Use legacy keyCode and charCode for powerpoint.officeapps.live.com. r=masayuki

Keywords: checkin-needed
Severity: normal → major

In addition to the PowerPoint issue, QA has found two issues with Word that are related:

  • CMD+A selects the text but shifts the focus out of the word_document, user cannot add extra text after this form of selection
  • After a text is added double clicking on it and writing something else doesn't replace the string but instead adds the new text to the existing one(happens with a flicker)

They could be fixed by adding word-edit.officeapps.live.com to the pref. However, both are minor, and thus, according to Liz, shouldn't block the release. :) I'm leaving them as a comment here so we have that on our radar for both outreach and in-tree patches. (But maybe those should be in their own separate issues)

Flags: needinfo?(tmaity)

Tracking the Normandy hotfix for release in bug 1537900.

Note: We shipped a temporary hotfix with Normandy recipe id 721
see: https://normandy.cdn.mozilla.net/api/v3/recipe/721/

See Also: → 1538126

Given that we successfully fixed this for both release users (via Normandy) and for Nightly users (with the patch attached), I filed a second issue to track the Word issues, bug 1538126, to give us a place to decide what we should do about that.

As the patch is already in Nightly (the pref is set correctly in the 2019-03-22 build), I'll mark this issue as resolved. I only added leave-open because people where working on the Normandy rollout and the Word issues, which both have their own bug now. :)

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED

Comment on attachment 9052453 [details]
Bug 1536453 - Use legacy keyCode and charCode for powerpoint.officeapps.live.com. r=masayuki

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1502795
  • User impact if declined: N/A, patch is changing a pref to a value that's currently being rolled out to Release users via Normandy.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Given we have verified the pref value itself by QA and are rolling it out to Release, I don't see any additional risk in this patch.
  • String changes made/needed:
Attachment #9052453 - Flags: approval-mozilla-release?
Attachment #9052453 - Flags: approval-mozilla-beta?
Target Milestone: --- → mozilla68

You can visit office.com and login with your Microsoft account and access the free version of office online and create new documents.

https://docs.microsoft.com/en-us/office365/enterprise/Office-365-endpoints lists all the domains we have across all M365 products in various markets.

Do we understand what the regression is? Why does it block these domains in the first place?

Thanks anigo!

The office team is now aware of the issue.

After a side discussion on Twitter we have a list of domains that would cover all versions of powerpoint and word being served for the blacklist.

powerpoint.officeapps.live.com, word-edit.officeapps.live.com, *.partner.officewebapps.cn,* .online.office.de, *.gov.online.office365.us,* .officeapps-df.live.com.

Masayuki - do you think there is a risk of breaking things unintentionally by adding these domains to the blacklist?

Flags: needinfo?(masayuki)

Please add *.officeapps.live.com instead of specific subdomains, we have other apps (onenote, Excel) that could potentially be impacted as well.

Masayuki - do you think there is a risk of breaking things unintentionally by adding these domains to the blacklist?

Honestly, I have no idea. But if something is actually broken, we have no choice except including them into the blacklist.

Flags: needinfo?(masayuki)

It seems that the patch which added powerpoint.officeapps.live.com to a whitelist is only a partial fix since we know that:

It seems that at a minimum, the patch for beta and release should cover the apps we know for sure are impacted:
powerpoint.officeapps.live.com, word-edit.officeapps.live.com

Dennis, Adam, can we agree on a final safe list of domains to add to the whitelist and uplift that to beta and release? Thanks

Flags: needinfo?(dschubert)
Flags: needinfo?(astevenson)
See Also: → 1538317
See Also: → 1538652
See Also: → 1538651
See Also: → 1537913

Comment on attachment 9052453 [details]
Bug 1536453 - Use legacy keyCode and charCode for powerpoint.officeapps.live.com. r=masayuki

Let's cancel the uplift requests, the patch in bug 1538966 will contain the Powerpoint fix as well.

Flags: needinfo?(dschubert)
Attachment #9052453 - Flags: approval-mozilla-release?
Attachment #9052453 - Flags: approval-mozilla-beta?

So, I had a very long look at the Office source, trying to figure out why things break if we change our behavior. And while I got a bit closer, I didn't manage to catch the difference between Chrome and Firefox yet. As Masayuki noticed, Microsoft has written an abstraction to keyboard events, which correctly deals with keyCode and charCode and shouldn't cause any issues. And in fact, it doesn't.

The issue is somewhere else. In Edit.Core.HermesV7.js, there is a function called VAc that gets triggered to handle keyboard events. In there, we this snippet:

"keypress" === a && wac_dg() && (wac_uja(c) || c.altKey || c.ctrlKey || wac_Ck(c, "metaKey"))
  ? (a = !1)
  : ("keydown" === a &&
      wac_Ge() &&
      (6 >= wac_8ea() || b === window.document) &&
      window.focus(),
    d || (d = wac_vja(this, c, a, b)),
    (a = d));

which looks like it's deciding whether to handle or discard an event based on some conditions. The conditions look like they attempt to stop handling the event if a modifier key is pressed. Also, the event will be discarded if wac_uja(c) returns something truey, and here is where our issue is. This function looks like:

function wac_uja(a) {
  return a ? (a.rawEvent ? a.rawEvent.keyCode : a.keyCode) : 0;
}

a is the result of their custom event parsing function, but rawEvent is just a reference to the original event that got dispatched. If we mirror keyCode and charCode, keyCode will be set here, which is causing the above logic not to do what it is supposed to do, which results in the error we have in this report.

Now, that would fail in Chrome as well, because in Chrome, keyCode would also be set. The catch is the other function in that chain: wac_dg, which is...

function wac_dg() {
  return Sys.Browser.agent === Sys.Browser.Firefox;
}

And that's it. I don't know what the Firefox-specific logic is supposed to be doing, but as far as I can tell, if I replace the call mentioned above with false, the app is working fine, even if we mirror keyCode and charCode. Maybe this information can be useful to our friends at Microsoft. :)

Updating Fx67 status based on status-firefox67 in bug 1538966

I believe we can call this fixed in 66.0.2 as well?

Flags: needinfo?(astevenson)

FYI Microsoft also makes an on-premises Office Online and SharePoint servers that an organization can install locally to get this same functionality. The bug is present in these also, and won't be covered by the domain check.

Thanks -- we're in touch with Microsoft about the issue and hope to have a solution.

I use 66.0.5 and still experience the issue on a Word online editor embedded in cernbox.cern.ch. I've tried adding that domain to dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode but it didn't help.

Any update on this? Still present in 67.0.2...

(In reply to bornerdogge from comment #44)

I use 66.0.5 and still experience the issue on a Word online editor embedded in cernbox.cern.ch. I've tried adding that domain to dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode but it didn't help.

Can you paste the contents of dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode here please?

Flags: needinfo?(bornerdogge)

Here it is:
*.collabserv.com,*.gov.online.office365.us,*.officeapps-df.live.com,*.officeapps.live.com,*.online.office.de,*.partner.officewebapps.cn,*.scniris.com,cernbox.cern.ch,*.cernbox.cern.ch

Flags: needinfo?(bornerdogge)
You need to log in before you can comment on or make changes to this bug.