Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4328 closed change (fixed)

Check shadowRoot doesn't already exist before creating it

Reported by: kzar Assignee:
Priority: P3 Milestone: Adblock-Plus-1.13-for-Chrome-Opera
Module: Platform Keywords:
Cc: sebastian, kzar, mapx, BrentM Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29349823/

Description

Background

If supported by the user's browser we create a shadowRoot on the document element for our element hiding rules. Since our code to do this always runs before the website's code we assumed that there couldn't already be a shadowRoot there. However reading this forum post made me realise that if another extension was installed that did the same thing, for example AdBlock, then there might already have been a shadowRoot created.

While creating multiple shadow roots for the same element does work, it's deprecated and displays the following warning in Chrome: "Calling Element.createShadowRoot() for an element which already hosts a shadow root is deprecated. See https://www.chromestatus.com/features/4668884095336448 for more details.".

What to change

Check that there is not already a shadowRoot before creating one in include.preload.js.

Hints for testers

Install both Adblock Plus and AdBlock in a modern version of Chrome. Browse a few websites with the developer console for the page open. Ensure the warning message mentioned above isn't displayed.

Change History (11)

comment:1 Changed 3 years ago by kzar

@mapx Note: The second warning the user complained about removeAttribute will no longer be a problem with the 1.12.2 release, since that code was removed.

comment:2 Changed 3 years ago by kzar

  • Priority changed from Unknown to P3
  • Ready set
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:3 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Fixes 4328 - Multiple shadowRoot deprecated warnings

comment:4 Changed 3 years ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:5 Changed 3 years ago by mapx

still getting warnings in console here:
http://www.carbuyer.co.uk/

Calling Element.createShadowRoot() for an element which already hosts a shadow root is deprecated. See https://www.chromestatus.com/features/4668884095336448 for more details.
Y @ abs17508.js:6
value @ abs17508.js:7
(anonymous) @ abs17508.js:7
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2126:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2122:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2057:3
u @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2119:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2115:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2049:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2027:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2025:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2178:3
(anonymous) @ abs17508.js:1
(anonymous) @ abs17508.js:1
value @ abs17508.js:1
h @ implied-consent.min.js:7
(anonymous) @ VM2176:3
(anonymous) @ abs17508.js:2
nrWrapper @ (index):8

Last edited 3 years ago by mapx (previous) (diff)

comment:6 Changed 3 years ago by kzar

  • Cc BrentM added

Until AdBlock updates this code too you will still get the warning depending on the order the extensions are loaded. Not much we can do there other than to wait.

comment:7 Changed 3 years ago by mapx

AB ? :O it's about ABP, last dev build which still provokes that warning / error in console.

I use only ABP in this test.

Last edited 3 years ago by mapx (previous) (diff)

comment:8 Changed 3 years ago by kzar

Oh I see, sorry I misunderstood you there. Yes, well if the website creates the shadowRoot for the same element Adblock Plus did then Chrome will display a warning. Not much we can do about that.

(This issue was about the fact that both Adblock Plus and AdBlock create the shadowRoot for the same element, so if you had both installed you would constantly see the warning for all websites.)

comment:9 Changed 3 years ago by Ross

Done. The error is still displayed when using both ABP/AB as AB has not been updated yet (as Dave says) but ABP no longer causes the error itself. Will mark fixed closer to release or after AB has updated to match.

ABP 1.12.4.1704
Chrome 49/52/55 / Windows 7
Chrome 49/52/55 / OS X 10.10

comment:10 Changed 3 years ago by kzar

I'd rather you verified the fix worked rather than just took my word for it. (Also I haven't checked that AdBlock didn't update the adblockpluschrome dependency, I just explained that until they do (or did) the exception was expected to still appear depending on the order the extensions were installed.)

Also it would be useful to know which version of AdBlock you used for testing here since that's just as important as the Adblock Plus version.

So I'd request that you first ensure you're running the latest version of AdBlock. If the exception still appears uninstall both extensions and install them again, making sure to install AdBlock first. Then confirm the exception no longer occurs.

comment:11 Changed 3 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

I agree. I've tested with this the versions below with and didn't see the shadowroot error whichever I installed first.

ABP 1.12.4.1739 / AdBlock 3.8.8
Chrome 49 / 56 / Windows 10
Chrome 56 / OS X 10.12
Chrome 56 / Ubuntu 16.04
Opera 37 / 41 / Windows 7
Safari 10 / OS X 10.12

Note: See TracTickets for help on using tickets.