Closed Bug 1506792 Opened 6 years ago Closed 6 years ago

Inspector panel doesn’t show <body>’s children if body::after exists

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Version:
65 Branch
Type:
defect

Tracking

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 verified, firefox65 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 65
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- verified
firefox65 --- verified

People

(Reporter: me, Assigned: pbro)

References

Details

(Keywords: regression, Whiteboard: [lang=js])

Attachments

(1 file)

Bug 1506792 - Bail out when trying to get getComputedStyle for a non attached node; r?jdescottes!
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

1. Load this minimal HTML: <style>body::after{content:""}</style><hr>. Alternatively, https://www.topicbox.com will do.
2. Open the Inspector tab of the dev tools, and look at its representation of the DOM tree.


Actual results:

The <body> node in the tree is shown as expanded, but contains no children.


Expected results:

The <body> should display all its children (an <hr> in my minimal example, or a lot more on https://www.topicbox.com).


Note that you can work around this bug by closing the dev tools and reopening them. But you’ll need to do this on every page load, which is tedious.

This has been annoying me for many, many months (it’s been a bug from the very start of the new dev tools, I think), but for some reason I never got round to minimising it and filing this bug report until today.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=631545ef79251ea54347ebcb76420b7c1c9ba333&tochange=bfb92e2d55e43b25ee6256c7a037ead4022c3f8d

I can reproduce the problem with the link, or with the page saved locally, but not with a reduced testcase in the form of
data:text/html,<style>body::after{content:""}</style><hr>

Pressing F12 twice to close and reopen the Inspector panel makes the nodes appear.
Blocks: 1499322
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Inspector
Ever confirmed: true
Keywords: regression
Product: Firefox → DevTools
Flags: needinfo?(pbrosset)
There are 2 problems here:

1. The old problem of the inspector not supporting ::after in <body> which is very bad, but for some reason had never been reported unfortunately. I'm going to start investigating this one.

2. A bug related to the new flex inspector tool, which seem to happen in beta/devedition but not in nightly.

At least I think those are 2 different things for now.
And I'm not worried about #2 because I think it's fixed in nightly now, and the flexbox inspector is going to be OFF by default in release 64, so people won't see that problem.

For #1, I'm seeing this stacktrace in  the browser console:

Protocol error (unknownError): Argument 1 of Window.getComputedStyle is not an object. markup.js:187:7

    _handleRejectionIfNotDestroyed resource://devtools/client/inspector/markup/markup.js:187 _updateChildren resource://devtools/client/inspector/markup/markup.js:1845 _expandContainer resource://devtools/client/inspector/markup/markup.js:1266 expandNode resource://devtools/client/inspector/markup/markup.js:1281 MarkupView resource://devtools/client/inspector/markup/markup.js:137 _onMarkupFrameLoad resource://devtools/client/inspector/inspector.js:1936 _initMarkup resource://devtools/client/inspector/inspector.js:1925 _deferredOpen resource://devtools/client/inspector/inspector.js:262 init resource://devtools/client/inspector/inspector.js:170 open resource://devtools/client/inspector/panel.js:12 onLoad resource://devtools/client/framework/toolbox.js:1784
Flags: needinfo?(pbrosset)
So, the stop-gap solution is in /devtools/server/actors/inspector/css-logic.js, in the CssLogic.getComputedStyle function: if the bindingElement object returned by CssLogic.getBindingElementAndPseudo(node) is null, then return null instead of attempting to call getComputedStyle on it, since that is what currently fails.

This is only a stop-gap measure that will make the inspector show all of its children nodes, but the root cause still persists.
There is something odd at work here that makes the ::after pseudo-element behave as if it wasn't attached to the document tree.
Indeed, when the inspector starts, it tries to get the children of the <body> element, and does find the ::after pseudo, but later when trying to get styles for it, it fails because node.parentNode is null, as if this pseudo wasn't attached.

We should ideally fix this too, but I can't figure out what the problem is here.
In fact, even with the fix I described done, if you later click on the ::after element in the inspector, you can see that the inspector does not entirely recognize this node. It doesn't show any CSS rules for it for example.
It does as soon as you close and reopen DevTools though.

I guess we could start by implementing the stop-gap measure since its known and quick to do, and does show all children of <body>. And then file a subsequent bug to investigate the underlying issue.

If someone is interested in doing this simple fix, I'm willing to mentor them and review the code.
Make sure you read through our contribution docs first: http://docs.firefox-dev.tools/
And once you're ready and able to make changes to and reload Firefox, feel free to ask for this bug to be assigned to you/attach a patch here.
Mentor: pbrosset
Keywords: good-first-bug
Priority: -- → P1
Whiteboard: [lang=js]
See Also: → 1507528
Willing to take a look at this in the upcoming weekend
(In reply to Patrick Brosset <:pbro> from comment #2)
> And I'm not worried about #2 because I think it's fixed in nightly now, and
> the flexbox inspector is going to be OFF by default in release 64, so people
> won't see that problem.
I think I was wrong here. It does happen in nightly, and will be a problem when 64 merges to releases (I tested that use) also.
So we do need this stop-gap solution fixed in 65 and then uplifted to 64 asap, so I'll take this bug right now.
Sorry Jay, I know you wanted to work on it this week-end, but I'll suggest other bugs :)
Assignee: nobody → pbrosset
Mentor: pbrosset
Status: NEW → ASSIGNED
Keywords: good-first-bug
(In reply to Patrick Brosset <:pbro> from comment #5)
> (In reply to Patrick Brosset <:pbro> from comment #2)
> > And I'm not worried about #2 because I think it's fixed in nightly now, and
> > the flexbox inspector is going to be OFF by default in release 64, so people
> > won't see that problem.
> I think I was wrong here. It does happen in nightly, and will be a problem
> when 64 merges to releases (I tested that use) also.
> So we do need this stop-gap solution fixed in 65 and then uplifted to 64
> asap, so I'll take this bug right now.
> Sorry Jay, I know you wanted to work on it this week-end, but I'll suggest
> other bugs :)

Understandable, I can start looking into the underlying issue as well.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aed811658fae
Bail out when trying to get getComputedStyle for a non attached node; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/aed811658fae
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Please nominate this for Beta approval when you get a chance.
status-firefox63: --- → unaffected
status-firefox64: --- → affected
status-firefox-esr60: --- → unaffected
Flags: qe-verify+
Flags: needinfo?(pbrosset)
Flags: in-testsuite+
Comment on attachment 9025661 [details]
Bug 1506792 - Bail out when trying to get getComputedStyle for a non attached node; r?jdescottes!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1499322

User impact if declined: If declined, users of devtools might see missing nodes in the inspector panel when they refresh a page that uses pseudo-elements.
This is quite common, and the inspector panel is the most used of devtools. So this could impact a lot of people.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

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): This is a one liner that is making some code more defensive. I've not changed any important logic here, just added an early `return null;` in one specific case.

String changes made/needed: None
Flags: needinfo?(pbrosset)
Attachment #9025661 - Flags: approval-mozilla-beta?
I verified this issue on Mac OS X 10.12, Windows 10, Ubuntu 16.04 with FF Nightly 65.0a1(2018-11-19) using the steps from the description and I can confirm the fix.
Status: RESOLVED → VERIFIED
status-firefox65: fixed → verified
Comment on attachment 9025661 [details]
Bug 1506792 - Bail out when trying to get getComputedStyle for a non attached node; r?jdescottes!

devtools fix, verified in nightly, approved for 64.0b12
Attachment #9025661 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1507185
I verified this issue on Mac OS X 10.14, Windows 10, Ubuntu 16.04 with FF Beta 64.0b12 using the steps from the description and I can confirm the fix.
status-firefox64: fixed → verified
Flags: qe-verify+

I have an issue where often the first Time I do Inspect on an element I see a blank "Inspector". I noticed there is bug https://bugzilla.mozilla.org/show_bug.cgi?id=1509008 which describes exactly the issue I have and it was marked duplicate of this and this is marked as resolved but I am still having the issue all the time. Here is an example: https://imagebin.ca/v/5j7UtUzKgZR9

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: