1236283 - <object> and <embed> nodes in markup-view can always be expanded, even if they don't contain other nodes

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[arni2033]

Attachment: testcase 1 - object and embed nodes in markup-view can always be expanded.htm

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160101030330
STR:
1. Open attached "testcase 1" or this url:
>   data:text/html,<object></object><embed></embed>
2. Open Inspector
3. Click the dropmarker next to <embed>  element in markup-view
4. Click the dropmarker next to <object> element in markup-view

Result:       Both elements are expanded in markup-view
Expectations: This is inconsistency (due to bug 892935): there should be no dropmarkers at all.

Note (IMO):
 Looking at all mistakes in implementing bug 892935 and all usability issues it has caused,
 I believe that the best way was NOT to forbid expanding nodes, but, conversely,
 to improve the way nodes (including text nodes) are expanded.

Comment 1

[Patrick Brosset <:pbro>]

Thanks for filing. This is indeed pretty weird.
After a quick investigation, I can see that these nodes are flagged as having 1 child element (with the node selected, inspector.selection.nodeFront.numChildren == 1), but then, trying to get the child element returns nothing:
walker.children(inspector.selection.nodeFront.numChildren).then(({nodes}) => nodes.length == 0);

Investigation should take place in /devtools/server/actors/inspector.js
especially in the WalkerActor's children method and the way the numChildren property is set in NodeActor.
Somehow, there's a mismatch between the two.

Comment 2

[Joseph Yeh [:joseph]]

Hi Patrick,

I trace the code and found the problem here.

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#352

The truthy value from `rawNode.getSVGDocument` let the numChildren always equal to 1. And thus will not entered the next if statement which giving the correct numChildren value through `this.walker.children(this).nodes.length`.

It looks like we are having several solutions here, like avoiding the assignment from `numChildren = 1;` or let it consult with the walker. Which do you think is the proper solution and won't break the design here?

Thanks!

Comment 3

[Patrick Brosset <:pbro>]

Thanks for the investigation Joseph.
You're right `rawNode.getSVGDocument` is truthy here and therefore sets `numChildren` to 1.
I think the right might be to actually execute `getSVGDocument` instead of just checking that it exists.

We test the existence of `rawNode.getSVGDocument` to see if the node embeds SVG, but we don't execute that function. And it turns out that for <object> and <embed> this function exists, but may actually return `null`.

I believe we might be able to fix the issue by changing the if to:

let hasContentDocument = rawNode.contentDocument;
let hasSVGDocument = rawNode.getSVGDocument && rawNode.getSVGDocument();
if (numChildren === 0 && (hasContentDocument  || hasSVGDocument)) {

Comment 4

[Joseph Yeh [:joseph]]

Attachment: 0001-Bug-1236283-object-and-embed-nodes-in-markup-view-ca.patch

Thanks for your advice, I'll push this patch to try server later.

Comment 5

[Joseph Yeh [:joseph]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe243c8cf769

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8767840 [details] [diff] [review]
0001-Bug-1236283-object-and-embed-nodes-in-markup-view-ca.patch

Review of attachment 8767840 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks.
Can you update the comment message however? The commit message should not state the problem (or bug title) but instead explain what you changed. Plus, it should contain the reviewer's nick. Something like:

Bug 1236283 - Don't mistake <embed> and <object> as having 1 child node when they don't contain anything; r=pbro

Comment 7

[Joseph Yeh [:joseph]]

Attachment: 0001-Bug-1236283-Don-t-mistake-embed-and-object-as-having.patch

Sorry about that, I've update the patch.

Thanks for your help!

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8767865 [details] [diff] [review]
0001-Bug-1236283-Don-t-mistake-embed-and-object-as-having.patch

Review of attachment 8767865 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
R+ from me, please land this as soon as you get a green TRY.

Comment 9

[Joseph Yeh [:joseph]]

All green on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52490d08acfe

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3468f546b085
Don't mistake <embed> and <object> as having 1 child node when they don't contain anything; r=pbro

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b9cc2dcbf4363c8049183110fe79e026fae87ecb since one of this 2 changes seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=10330926&repo=fx-team 

can you take a look since i'm not sure which of this 2 changes caused this regressions

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9ef4fa72ea0b
Backed out changeset 3468f546b085

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

The regression was mine from bug 1171614, so this is fine to go back in.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9f227558d11f
Don't mistake <embed> and <object> as having 1 child node when they don't contain anything. r=pbro

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f227558d11f

Comment 16

[arni2033]

Fixed on:   Win7_64, Nightly 50, 32bit, ID 20160714030208 (2016-07-14)