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

VERIFIED FIXED in Firefox 50



Developer Tools: Inspector
2 years ago
2 years ago


(Reporter: arni2033, Assigned: joseph, Mentored)


Firefox 50

Firefox Tracking Flags

(firefox46 affected, firefox50 verified)


(Whiteboard: [good next bug][lang=js][btpp-fix-later], URL)


(2 attachments, 1 obsolete attachment)



2 years ago
Created attachment 8703378 [details]
testcase 1 - object and embed nodes in markup-view can always be expanded.htm

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160101030330
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.


2 years ago
Has STR: --- → yes
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.
Mentor: pbrosset@mozilla.com
Severity: trivial → normal
Priority: -- → P2
Whiteboard: [good next bug][lang=js][btpp-fix-later]

Comment 2

2 years ago
Hi Patrick,

I trace the code and found the problem here.


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?

Assignee: nobody → jyeh
Flags: needinfo?(pbrosset)
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)) {
Flags: needinfo?(pbrosset)

Comment 4

2 years ago
Created attachment 8767840 [details] [diff] [review]

Thanks for your advice, I'll push this patch to try server later.
Attachment #8767840 - Flags: review?(pbrosset)
Comment on attachment 8767840 [details] [diff] [review]

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
Attachment #8767840 - Flags: review?(pbrosset) → review+

Comment 7

2 years ago
Created attachment 8767865 [details] [diff] [review]

Sorry about that, I've update the patch.

Thanks for your help!
Attachment #8767840 - Attachment is obsolete: true
Attachment #8767865 - Flags: review?(pbrosset)
Comment on attachment 8767865 [details] [diff] [review]

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

R+ from me, please land this as soon as you get a green TRY.
Attachment #8767865 - Flags: review?(pbrosset) → review+

Comment 9

2 years ago
All green on try.

Keywords: checkin-needed

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
Don't mistake <embed> and <object> as having 1 child node when they don't contain anything; r=pbro
Keywords: checkin-needed
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
Flags: needinfo?(jyeh)

Comment 12

2 years ago
Backout by cbook@mozilla.com:
Backed out changeset 3468f546b085
The regression was mine from bug 1171614, so this is fine to go back in.
Flags: needinfo?(jyeh)
Keywords: checkin-needed

Comment 14

2 years ago
Pushed by ryanvm@gmail.com:
Don't mistake <embed> and <object> as having 1 child node when they don't contain anything. r=pbro
Keywords: checkin-needed

Comment 15

2 years ago
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Comment 16

2 years ago
Fixed on:   Win7_64, Nightly 50, 32bit, ID 20160714030208 (2016-07-14)
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.