Closed Bug 1452707 Opened 2 years ago Closed 2 years ago

Build site patch for ib.absa.co.za

Categories

(Web Compatibility :: Interventions, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: denschub, Assigned: denschub)

References

Details

(Whiteboard: [sitepatch-built])

Attachments

(1 file, 3 obsolete files)

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1448045#c4, ib.absa.co.za ises user agent matching, which results in Firefox getting a "Browser not supported" warning, although there is no technical reason for it.

Adding "webkit" to the UA string should already be enough to make the site work. This bug is to track the minimal override and the rollout of said patch.
Priority: -- → P1
PR is against the `release` branch, as this is the latest version approved and landed in mozilla-central, which means we could roll this out via GoFaster at any time, and not add any new features, or potential perf regressions.

r? Tom, f? Mike, as Bugzilla does not like having two r?s on one patch.
Attachment #8966506 - Flags: review?(twisniewski)
Attachment #8966506 - Flags: feedback?(miket)
Attachment #8966506 - Flags: feedback?(miket) → review?(miket)
Attached file WebCompat v.1.1.1, Unsigned XPI (obsolete) —
Attaching an unsigned XPI for testing purposes.
Only adding "AppleWebKit" to the User Agent without any version number did not work, as they try to parse the version number and their source throws if it's not there, so we need to append "AppleWebKit/605.1.15". With the XPI installed (and Firefox restarted), I can verify that I no longer see the "Unsupported" message, and the site *seems* to work.

That being said, I do not have a testing account there, so the internal impact may be higher. ni? Mike, as I do not feel 100% comfortable with this for some reasons:

* We cannot verify that the application works when signed in.
* Spoofing the UA actually breaks mobile, but since the GoFaster add-on is not on Mobile yet, this is fine. With 2.0, we have to possibility to only target Desktop, so we shouldn't have any issues here.
* There are a lot of user-agent specific cases, where they check for the browser and do different things. I first tried to spoof as opera, but it did break keyboard event handling, as they have an exception for that. I checked all the instances of WebKit-specific logic in the sources I can see, but I cannot test the site to its full extend. The JS does not feel complete enough to be used for the logged-in part as well, so there are likely more JS files I cannot check.

As the changes in question only break the site in the 61 tree, we should not do a 100% rollout of this override, probably. If anything, it should be limited to the version affected (61+), which would also give us a bit more time to try outreach here.

Mike, what do you think?
Flags: needinfo?(miket)
> As the changes in question only break the site in the 61 tree, we should not do a 100% rollout of this override, probably. If anything, it should be limited to the version affected (61+), which would also give us a bit more time to try outreach here.

So, we haven't actually removed window.controllers yet, it's just disabled in Nightlies. So if you think it would be safer for us to block on v2, I support that. It seems crappy to break mobile.

Also yeah, it's kinda scary to make these changes on bank without an account. Could you try to PM the user who reported the issue, and see if they could install an XPI and test @ https://support.mozilla.org/en-US/user/johannesstander83? (https://support.mozilla.org/en-US/questions/997702)?
Flags: needinfo?(miket) → needinfo?(dschubert)
Okay, I thought about this again, and as a result, I made this depending on the GoFaster v2 shipping.

With a UA override, the site will treat us as a different browser, which leads to some side effects I'm not in control over. And while I can get in touch with the reporter on SuMo (which I'll do) and have an override tested, I'd still feel much more comfortable with another solution.

Here, they only really check if window.controllers is not falsy. With GoFaster v2, we could easily set window.controllers to a string "window.controllers has been shimmed with a string to get this site's browser detection to work in Firefox 61+", and have the site detecting us as Firefox.

That would make me much more comfortable deploying this.
Depends on: 1386807
Flags: needinfo?(dschubert)
Summary: Build User Agent override for ib.absa.co.za → Build site patch for ib.absa.co.za
I built a site patch on top of version 2 instead, this is the PR. As Tom is now also a member of the GitHub org, I asked for explicit review there, so I don't have to mess with r? flags here on Bugzilla.
Attachment #8966506 - Attachment is obsolete: true
Attachment #8966506 - Flags: review?(twisniewski)
Attachment #8966506 - Flags: review?(miket)
Attached file WebCompat v.2.0.1, Unsigned XPI (obsolete) —
And here is an XPI if someone wants to test the site patch without building the addon themselves.
Attachment #8966507 - Attachment is obsolete: true
Comment on attachment 8967694 [details]
WebCompat v.2.0.1, Unsigned XPI

Marking old XPI as obsolete, as the code got rebased due to changes during the m-c review. Will create a new XPI as part of the patch containing multiple site patches.
Attachment #8967694 - Attachment is obsolete: true
Blocks: 1481395
Site patch is landed in our repo on GitHub, so this can be closed. Landing the site patches (and eventually updating Release users via GoFaster) is tracked in 1481395.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Whiteboard: [sitepatch-built]
You need to log in before you can comment on or make changes to this bug.