Skip to content
This repository has been archived by the owner. It is now read-only.

Removes orphaned elements #8391

Merged
merged 1 commit into from May 3, 2017
Merged

Removes orphaned elements #8391

merged 1 commit into from May 3, 2017

Conversation

@jonathansampson
Copy link
Collaborator

jonathansampson commented Apr 19, 2017

Testing Plan:

Navigate to an article on GlennBeck.com and confirm that no modal is shown.

PR Details:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fixes #8390

@jonathansampson jonathansampson added this to the 0.15.1 milestone Apr 19, 2017
@jonathansampson jonathansampson self-assigned this Apr 19, 2017
@jonathansampson jonathansampson requested a review from lukemulks Apr 19, 2017
Copy link
Collaborator

lukemulks left a comment

@jonathansampson it looks like the update works to resolve, with one exception:

  • From a clean profile, the modal does open, but only for the first page load.

  • After the first page load, the modal no longer opens for additional refreshes, or while navigating to other pages on the same domain.

I've tested the following from the site settings menu with a clean profile each time:

  • If I block all cookies, I see the modal on every page load.
  • If I allow all cookies, after the first page load I no longer see the modal appear.
  • If I block 3rd party cookies, after the first page load I no longer see the modal appear.

From the issue: #8390 and IIRC, we were seeing the modal load every time the page was refreshed before this fix.

I think this solves once the cookie is set, but requires the modal to load once in order to set the cookie (even when all cookies are enabled).

@jonathansampson
Copy link
Collaborator Author

jonathansampson commented Apr 19, 2017

If the modal opens at all, then this PR is broken. :)

The page checks for the gb3lightbox=1 cookie entry. If it's not found, the modal is shown. Once the modal is shown, the cookie entry is created with a 24-hour expiration.

This PR intends the preemptively add that cookie entry, and thus prevent even the first showing.

@lukemulks
Copy link
Collaborator

lukemulks commented Apr 19, 2017

Cool. Got it. Was primarily reporting that there is intermittency that wasn't present before, when the modal was consistently opening.

It's odd to me that we're able to see an ad modal load with a 1x/24hr freq cap without there being a call to the ad server, which is typically expected for serving the ad to the slot. They may be doing something a little more custom here. Sounds like it.

Can take a closer look at the page beyond spot checking the test if you'd like, and let you know if anything stands out. I didn't dig too far into this yesterday beyond spot checking network traffic.

@jonathansampson jonathansampson force-pushed the sitehack-glennbeck branch from f4d55a1 to 293d1e3 Apr 22, 2017
@jonathansampson
Copy link
Collaborator Author

jonathansampson commented Apr 22, 2017

@lukemulks Okay, issue was simpler than I thought. The site checks document.cookies for the following value: ' gb3lightbox=1' (note the space before the key). This little issue managed to pass under my radar for a couple of hours :)

@lukemulks
Copy link
Collaborator

lukemulks commented Apr 22, 2017

@jonathansampson I just checked this out, and I'm still getting the modal on the first pageload with a clean profile.

@bbondy bbondy force-pushed the master branch from f0df59d to 70e5d77 Apr 23, 2017
@bsclifton
Copy link
Member

bsclifton commented May 1, 2017

pinging @jonathansampson- anything left for this one? (see comment by @lukemulks)

@jonathansampson
Copy link
Collaborator Author

jonathansampson commented May 2, 2017

@lukemulks Can you double-check for me; I'm not seeing the modal.

This is the condition on the site:

if (cookies.indexOf(" gb3lightbox=1") > -1) {
} else {
    var date = new Date();
    date.setTime(date.getTime()+(24*60*60*1000));
    document.cookie = "gb3lightbox=1;expires=" + date.toGMTString() + ";domain=.www.glennbeck.com;path=/";
  
    fireOnLoadLightbox();
}

We work around the issue by adding this:

document.cookie = '__a=1'
document.cookie = 'gb3lightbox=1'

Earlier we were only setting the second cookie value, which mean there wouldn't be a leading space when the site checked document.cookie. By adding a preceding entry, we're guaranteed there will be a leading space (provided keys are presented in the order in which they are added).

@lukemulks
Copy link
Collaborator

lukemulks commented May 2, 2017

@jonathansampson
Copy link
Collaborator Author

jonathansampson commented May 2, 2017

@lukemulks I'm happy to hop on a screen share to make sure we're following similar STR :)

@lukemulks
Copy link
Collaborator

lukemulks commented May 3, 2017

@jonathansampson I have better news now. :-)

I had to re-build Brave locally, and pulled this back in with a squeaky-clean profile, and am able to confirm that the modal no longer shows up on 1st visit, or from any additional visits.

We're g2g! Sorry for the delay!

@lukemulks lukemulks merged commit 64b5579 into master May 3, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@bsclifton bsclifton deleted the sitehack-glennbeck branch May 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.