1556135 - getClosestBackgroundImage inspector actor utility throws an exception when working with a node from Shadow DOM.

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

When a node is passed to the function (happens in a11y panel where the platform a11y stitches together the tree and includes the shadow DOM) that belongs to a shadow DOM, there will be a case when we iterate to the shadow root that will have an ownerDocument (e.g. host's owner document) but no parentNode (null) and thus throwing a type error when checking current.ownerDocument when current is null.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Bug 1556135 - only check if the current node is null in getClosestBackgroundImage. r=gl

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

This failure is seen when we try to run an audit in a11y panel on nytimes.com and it fails.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/096dd8162ff9
only check if the current node is null in getClosestBackgroundImage. r=gl

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 9069156 [details]
Bug 1556135 - only check if the current node is null in getClosestBackgroundImage. r=gl

Beta/Release Uplift Approval Request

• User impact if declined: Accessibility panel full page colour contrast (ships in 68) will fail on any page that contains a video because we now include picture-in-picture controls that use shadow DOM for video elements.
• Is this code covered by automated tests?: No (not explicitly, the tests that already exist would catch possible regression).
• Has the fix been verified in Nightly?: No (did not land on Nightly yet)
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky because the original bug happened because of a check that was incorrectly specified and was also more strict (did not consider shadow DOM).
• String changes made/needed: None

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/096dd8162ff9

Comment 6

[Julien Cristau [:jcristau] (back April 22)]

(In reply to Yura Zenevich [:yzen] from comment #4)

• User impact if declined: Accessibility panel full page colour contrast (ships in 68) will fail on any page that contains a video because we now include picture-in-picture controls that use shadow DOM for video elements.

Is this also an issue on beta where PiP is disabled?

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

If PiP is disabled on beta it will not happen (this is more of an example of obvious failure). However, if there is a chance that Firefox front end or any website uses shadow DOM, and we try to calculate contrast inside, it will fail.

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9069156 [details]
Bug 1556135 - only check if the current node is null in getClosestBackgroundImage. r=gl

small devtools fix, approved for 68.0b8

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/f474ee2548c6