-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ad markup with <!DOCTYPE html> fails to render on mobile #134
Comments
Can you recommend a solution to this issue @rokostik ? Do we just have to replace @YuriyVelichkoPI - could you take a look? |
It seems we just strip that line using a regex before this line? prebid-universal-creative/src/utils.js Line 35 in 4066462
from the issue: html = html.replace(/<(?xml|(!DOCTYPE[^\>\[]+([[^\]]+)?))+[^>]+>/g, ''); |
It appears https://github.com/krux/postscribe/pull/507/files may also solve ; @nickjacob could you comment? |
Hi @patmmccann -- that's true, our fork of prescribe skips over invalid/unrecognized HTML; this is the relevant commit -- AppMonet/prescribe@cfc81a3 We've used it a lot in mobile app environments, but it doesn't have great test coverage. I'm not currently using it (we just build an additional iframe & contentDocument.write when rendering asynchronously). |
I can clean up my prescribe & postscribe forks and publish to NPM if you want to test the behavior on this |
@patmmccann looks like you're right -- https://jsfiddle.net/76xcq12d/ the @appmonet/postscribe does handle the doctype in the ADM I made a PR to prescribe with only the change that would fix this. Not sure if you want to go off of our fork? Looks like smaato also has a fork with some good changes. I just published @appmonet/postscribe to NPM, you could try it in universal creative like this:
So the import doesn't have to change |
I am unable to reproduce, but I'm afraid I'm missing some context. I tried rendering the example markup ( |
Looking through the code, it appears that setting up ads for mobile is more complicated than I can afford, and not documented (at least not here), which explains why it was not seeing the issue. If I force use of postscribe rendering does fail, and #172 seems to fix it. However this raises the question: why are we using postscribe? ads rendered "normally" seem to work fine in an android webview. It's also useless bytes for everyone not serving mobile ads. Is it a relic of the past? |
Describe the bug
If an ad markup contains the
<!DOCTYPE html>
preamble, the prescribe library (which postscribe uses to parse HTML: https://github.com/krux/prescribe) fails to parse the preamble and silently stops parsing and only the HTML up to that point is inserted into the document. This is a know and old issue with postscribe (krux/postscribe#358).To Reproduce
From prebid server return any ad markup with
<!DOCTYPE html>
(eg.<!DOCTYPE html><p>test<p>
) and inspect the DOM in chrome://inspect.In my case I only see the comment and two additional divs that are made, but not the ad markup.
Removing the doctype preamble renders the ad as expected.
Expected behavior
Render the ad with doctype correctly as on the web with prebid.js.
Screenshots
Render if doctype included:
Render without doctype:
Smartphone:
Tested on the android emulator.
The text was updated successfully, but these errors were encountered: