dotcom-rendering
dotcom-rendering copied to clipboard
Run native element positioning on 'load'
Why?
We have a recurring issue where the advertising (which is rendered natively) appears higher than the ad-slot and obscures page content.
Both videos and advertising are rendered natively and layered over the webview at the expected position in the view to overlay the placehodler that the web-layer draws.
It's proven difficult to replicate this issue reliably. One theory is that the web layer tells the native layer where the slots should be, and then something additional loads (an image or a figure?) which pushes down the article content, but it doesn't update the native layer with the new position for the slot (which is now further down), and the native ad floats over article content.
Looking at the source I see it essentially does three things:
-
265: void document.fonts.ready.then(() => {
when the fonts are loaded -
251: window.addEventListener( 'resize',
when the window is resized (I think the most common case for this will be orientation change) -
261: const observer = new MutationObserver(callback);
on DOM tree mutation. Note: there are changes, (loading images or text changes for example) that will cause reflow, but won't cause the mutationObserver to fire.
For each of these events it checks to see if the position has changed (line 239), and then if it has, it updates the advert position.
This PR adds a single additional invocation of the check for positioning against the DOM 'load' event, which should occur after all images and subviews are loaded.
The other change this PR makes it it moves the initial reportNativeElementPositionChanges();
to be the last in a chain of events in rendering an article, so now it runs after as much of 'work' is done as possible. This is a belt-and-braces approach and I could be persuaded to drop that part for now.
Screenshots of the bug
In my unscientific testing, I haven't yet managed to trigger the bug whereas it was easier for me to do so before deploying this branch.
That's good news. Thanks for your help. Lets merge early next week.
@mohammad-haque FYI
In my unscientific testing, I haven't yet managed to trigger the bug whereas it was easier for me to do so before deploying this branch.
That's good news. Thanks for your help. Lets merge early next week.
@mohammad-haque FYI
Thanks for this PR, hopefully this will fix this bug, we will only verify it once it goes out to users.