Problem/Motivation

When randomizing A/B tests, it can be confusing for some users if they start on version A, navigate to another page, and come back to see version B. We think that it would be a useful feature to be able to persist a variation for the users' browsing session.

Selfishly, this also allows a very easy integration with Google Optimize server-side experiments.

Proposed resolution

  • Persist variant storage on the client-side via Session Storage if available.

Remaining tasks

  • Discuss!
  • Harden initial patch (it omits error handling - ie the user agent does not support Session Storage, lack of ES5 support)
  • Add test(s)

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/A

CommentFileSizeAuthor
#36 3162053-36.patch10.87 KBjrockowitz
#32 3162053-32.patch10 KBjrockowitz
#24 3162053-24.patch6.94 KBjrockowitz
#21 3162053-persistent-variants-21.patch6.92 KBluke.leber
#20 3162053-persistent-variants-20.patch5.07 KBluke.leber
#15 3162053-persistent-variants-15.patch1.36 KBluke.leber
#9 3162053-persistent-variants-9.patch5.65 KBluke.leber
#8 3162053-persistent-variants-8.patch5.19 KBluke.leber
#7 3162053-persistent-variants-7.patch3.66 KBluke.leber
#2 ui-change.png53.42 KBluke.leber
#2 3162053-persistent-variants-2.patch3.17 KBluke.leber

Comments

Luke.Leber created an issue. See original summary.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-2.patch3.17 KB
new ui-change.png53.42 KB
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Issue summary: View changes
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Issue summary: View changes
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Issue summary: View changes
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Issue summary: View changes
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-7.patch3.66 KB
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-8.patch5.19 KB

Fix broken variants tests.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-9.patch5.65 KB
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Issue summary: View changes
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Status: Active » Needs review
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Status: Needs review » Needs work

Actually, the local storage needs to be scoped to the webform as well.

jrockowitz’s picture

jrockowitz commented

I think this is a design mistake, and a randomly selected variant should always persist for the user's entire session. Variants are still considered experimental, and we can make this improvement.

We might only need to commit some variation of the code in _webform_page_attachments?

Right now, the Webform module's JavaScript is not using ES6 and instead of using let for variable declariation, we probably need continue to use var.

If the Webform module moves to ES6, we should update all the JS code.

jrockowitz’s picture

jrockowitz commented

We might also need to adjust the label/description for the 'Randomly load variant' checkbox.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-15.patch1.36 KB

Patch includes just a Javascript tweak.

I did not test the JS on obscure configurations like IE, private windows, and customized browser settings related to storage. I'll try to do extensive testing after hours today.

Safe cross browser storage access is a minefield.

jrockowitz’s picture

jrockowitz commented

Ideally, we should be able to convert \Drupal\Tests\webform\Functional\Variant\WebformVariantRandomizeTest to JavaScript Test and check that the randomized variant persists.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

Examining \Drupal\Tests\webform\Functional\Variant\WebformVariantRandomizeTest, I now wonder whether we need to update session storage if a stored variant no longer exists.

Taking the scenario from the test, if a user has variant 'A' stored locally and the site owner disables 'A', then should the user transition to use another random variant? Likewise, if all variants are disabled (for example, if the A/B test concludes), should the local storage be cleared out for the variant?

jrockowitz’s picture

jrockowitz commented

I think the new code needs to add session handling in two steps, load and then save.

  • Load (and validate) the variant id from the session.
  • If the session has no valid variant id, a random one is selected
  • Save (or resave) the current variant id in the session.
jrockowitz’s picture

jrockowitz commented

BTW, you might need to create getSessionVariantID and setSessionVariantID helper functions to make the code more manageable.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-20.patch5.07 KB

Does this get things moving in the right direction?

Note - I still have yet to perform device testing on the JS. After a good looking patch emerges, I can run through some Browserstack testing on various devices.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
StatusFileSize
new 3162053-persistent-variants-21.patch6.92 KB

Forgot to remove the old Functional test.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Issue summary: View changes
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

I can't really explain those webform_group failures. May be related to https://www.drupal.org/project/group/releases/8.x-1.1 that came out yesterday?

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review
StatusFileSize
new 3162053-24.patch6.94 KB

Attached patch has minor improvements

  • Tweak getSessionVariantID and setSessionVariantID to remove the variant_id variable.
  • Changed WebformVariantRandomizeTest to WebformVariantRandomizeJavaScriptTest
jrockowitz’s picture

jrockowitz commented

Darn the "Composer require-dev failure" is a DrupalCI issue that is being worked on. I already bothered Neil Drumm. I am going to wait until next Monday to nudge Neil. I might be able to fix it by pushing a commit to 8.x-5.x over the weekend.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

I'll do some browserstack device testing later on tonight and report back.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

Results from device testing:

Browser OS Device Pass
Chrome Android Galaxy S20 Yes
Samsung Internet Browser Android Galaxy S20 Yes
UCBrowser Android Redmi Note 8 Yes
Firefox Android Google Pixel 4 Yes
Chrome Android OnePlus 8 Yes
Safari iOS iPhone 8 Yes
Internet Explorer (unknown version) Windows Phone 8.1 Nokia Lumia 930 No(?)
Edge Windows (10) N/A Yes
Safari Mac (Catalina) N/A Yes

The windows phone configuration didn't seem to redirect to any variants. I'm not quite sure why, as dev tools are unavailable with that configuration.

#24 looks A-OK to me!

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Status: Needs review » Reviewed & tested by the community
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

Actually...I tried one other test variation and got a somewhat unexpected result!

Toggling off cookie storage in Google Chrome results in:

Uncaught DOMException: Failed to read the 'sessionStorage' property from 'Window': Access is denied for this document.
    at getSessionVariantID (https://stm5f25bc976fc66-mgj3znjqcgxefqnebdof00hlfatcyod3.tugboat.qa/form/contact:14:23)
    at https://stm5f25bc976fc66-mgj3znjqcgxefqnebdof00hlfatcyod3.tugboat.qa/form/contact:44:22
    at https://stm5f25bc976fc66-mgj3znjqcgxefqnebdof00hlfatcyod3.tugboat.qa/form/contact:55:3

It looks like the if (window.sessionStorage)... expression is throwing an exception (Chrome 84). As a result, the redirect doesn't function at all.

I'm not sure if this constitutes an unacceptable condition or not. Anyone that blocks cookies and such these days on the web should be fairly used to a sub-par user experience as it is.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Status: Reviewed & tested by the community » Needs work
jrockowitz’s picture

jrockowitz commented

I think Chrome is working as expected. In webform.element.message.js, we are just checking for the window.sessionStorage and we probably should just repeat the same approach here

@see https://git.drupalcode.org/project/webform/-/blob/8.x-5.x/js/webform.ele...

We can probably change...

if (typeof(window.sessionStorage) === 'undefined') {

...to...

if (!window.sessionStorage) {

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review
StatusFileSize
new 3162053-32.patch10 KB

To address "Uncaught DOMException: Failed to read the 'sessionStorage' property from 'Window'" all calls to localStorage and sessionStorage must be wrapped in a try/catch statement. I am not sure we even need to keep the if (window.sessionStorage) {} statements

https://stackoverflow.com/questions/58152236/what-to-do-when-sessionstor...

The attached patch address all calls to localStorage and sessionStorage.

jrockowitz’s picture

jrockowitz commented

I did a little more research and found this article.

https://mathiasbynens.be/notes/localstorage-pattern

I would lean toward using the Modernizer approach.

var hasLocalStorage = (function() {
  try {
    return !!localStorage.getItem;
  } catch (exception) {
    return false;
  }
}());
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

This almost seems like something that should be bundled into a library to be reused everywhere in the module (this might even be a candidate for a core feature, honestly). There seems to be a lot of boilerplate involved in "safely" accessing storage (localStorage + sessionStorage).

I can test out the Modernizr approach with this issue on different devices and see how it goes, but I probably won't have time to update the patch and give it a drive until at least tomorrow evening.

jrockowitz’s picture

jrockowitz commented

This almost seems like something that should be bundled into a library to be reused everywhere in the module.

I looked through Drupal core and there is no exception handling around localStorage or sessionStorage. For now, I would be okay with including the Modernizr approach escapsulted within each JS file as needed. Right now, there is no dedicated generic JS library webform file .

jrockowitz’s picture

jrockowitz commented
StatusFileSize
new 3162053-36.patch10.87 KB
luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

Tested #36 against Safari on iOS 11 (Browserstack) + Chrome 84 with storage disabled - looks good!

Going to move to RTBC pending test runner. Thanks!

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented
Status: Needs review » Reviewed & tested by the community

  • jrockowitz authored fc978c6 on 8.x-5.x 
    Issue #3162053 by Luke.Leber, jrockowitz: Webform Variants - Add option...

  • jrockowitz authored 62a6f10 on 8.x-5.x 
    Issue #3162053 by Luke.Leber, jrockowitz: Webform Variants - Add option...
jrockowitz’s picture

jrockowitz commented
Status: Reviewed & tested by the community » Fixed

  • jrockowitz authored 62a6f10 on 6.x 
    Issue #3162053 by Luke.Leber, jrockowitz: Webform Variants - Add option...
  • jrockowitz authored fc978c6 on 6.x
    Issue #3162053 by Luke.Leber, jrockowitz: Webform Variants - Add option...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.