Opened 5 years ago

Closed 5 years ago

#452 closed defect (duplicate)

Adblock Plus for Chrome adding shadow dom breaks CSS transitions

Reported by: twk3 Assignee:
Priority: P3 Milestone:
Module: Platform Keywords:
Cc: smultron45@…, trev, sebastian Blocked By:
Blocking: Platform: Unknown
Ready: no Confidential: no
Tester: Verified working: no
Review URL(s):

Description

Environment

Windows 7, Chrome 34, Addblock Plus 1.8.1

(The shadowdom insertion code also breaks chrome on other OSes if you use the same syntax in your page, but Windows was the only one where I could get the actual adblock plus plugin to cause problems.)

Observed behaviour

Chrome aborts active css transitions when the shadow dom is inserted into the documentElement. Switching the insertion to happen in document.head fixes the problem. (similar to how the non-shadow dom codepath works.)

The buggy line of code is here: https://hg.adblockplus.org/adblockpluschrome/file/ac01e8433a86/include.preload.js#l34

It should instead be similar to https://hg.adblockplus.org/adblockpluschrome/file/ac01e8433a86/include.preload.js#l56

The Chrome issue that initially required us to use document.head was fixed.

Change History (13)

comment:1 Changed 5 years ago by twk3

I've been playing with it more, and I'm not confident that simply switching to document.head will fix the problem. It appeared to when I was first testing it. I will try and create a test tomorrow that demonstrates the issue better.

comment:2 Changed 5 years ago by mapx

  • Cc smultron45@… added
  • Component changed from Unknown to Platform
  • Priority changed from Unknown to P1

comment:3 Changed 5 years ago by trev

  • Priority changed from P1 to P3

Could you please post proper steps to reproduce? Do you have any web pages in mind that are affected by this? Does this only affect CSS transitions that are active when the page loads? What does it mean that the transitions are "aborted", do they skip to the final state immediately or do they restart?

So far this doesn't sound like a reason to abandon Shadow DOM - not using it (meaning inserting the style into the head element) causes a lot more issues. If anything, this needs to be filed as a Chrome bug.

comment:4 Changed 5 years ago by trev

  • Cc trev added
  • Summary changed from adblockpluschrome shadow dom breaks css transitions to Adblock Plus for Chrome adding shadow dom breaks CSS transitions

comment:5 Changed 5 years ago by sebastian

  • Cc sebastian added

comment:6 Changed 5 years ago by sebastian

I was able to reproduce it without Adblock Plus. When creating a shadow root at the same time a CSS transition is triggered, the transition doesn't run. This is a bug in Chrome. However not using Shadow DOM isn't a suitable workaround, since this will lead to other (probably more) issues.

If you show us an affected real world website, we still might be able to find an other workaround.

Last edited 5 years ago by sebastian (previous) (diff)

comment:7 Changed 5 years ago by twk3

Sites using twitter bootstrap at a version less than their 3.x line are relying on transitionEnd events, and not using an additional timeout.

The bootstrap plugins that break from this are: Alerts, Carousel, Collapse, Modal, Tooltip, Popover, and Tab

Our problem can be seen here: https://swarm.workshop.perforce.com/reviews/8727, where the area on that page will start expanding on domReady, and with adblock installed it will sometimes cause the transition to cut the end/cancel and no transitionEnd is fired.

It is much easier to reproduce on windows for some reason. Refreshing the page usually fixes it. Using enter in the address bar is how we reproduce it. It is sometimes hard to reproduce, so I had also narrowed it down to the shadow root insertion, and had manually run the insertion code to reliably reproduce it.

comment:8 Changed 5 years ago by sebastian

  • Resolution set to rejected
  • Status changed from new to closed

So we have following options:

  1. Abandon Shadow DOM and insert the style tag visible to the web page again. That will break web pages in other cases and potentially in worse ways, see #309.
  2. Using a delay when creating the shadow root. This should mitigate the issue in your case, but won't completely eliminate it. Also this can lead to ads showing up for the fraction of a second, before they will be hidden when our stylesheet is injected.
  3. Wait until Google fixes the bug in Chrome.

I am sorry, but the consequences of 1. and 2. clearly exceed the impact of your issue. So lets hope that Google is going to fix that bug soon.

comment:9 Changed 5 years ago by twk3

In the test file you submitted to chrome, it seems to start working for me if you do document.head.webkitCreateShadowRoot().appendChild(document.createElement("shadow"));

Which was the original suggestion I was trying to make above. (I apologize, I realize it wasn't clear.)

I can't say I'm too familiar with the shadow dom, so does this somehow not meet your requirements?

comment:10 Changed 5 years ago by sebastian

When you create a shadow root for an element, the shadow tree is rendered instead the original element. However a <shadow> element can be used to insert the original element (or older shadow root if any) into the shadow tree.

Creating a shadow root with just a <shadow> element should not have any effect. But it is sufficient to reproduce the bug in Chrome, breaking transitions that are triggered at the same time the shadow root is created. Of course this only affects transitions for elements rendered as part of the shadow tree.

Adblock Plus also adds a stylesheet to the shadow tree in order to hide elements inside the original tree without revealing the stylesheet to JavaScript that operates on the original tree. But this only works if the styled elements are inside the element we create the shadow root on. So the shadow root must be created on the documentElement.

comment:11 Changed 5 years ago by twk3

Thanks for explaining. And thanks for logging the Chrome bug.

comment:12 Changed 5 years ago by sebastian

  • Platform set to Unknown
  • Resolution rejected deleted
  • Status changed from closed to reopened

comment:13 Changed 5 years ago by sebastian

  • Resolution set to duplicate
  • Status changed from reopened to closed

This was actually fixed with #450, by creating the shadow root earlier, before the transitions would run.

Note: See TracTickets for help on using tickets.