Closed
Bug 1263696
Opened 9 years ago
Closed 9 years ago
Block embed content loading when ancestor is object element with usable content
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: qdot, Assigned: qdot)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 5 obsolete files)
Followup to bug 1262184, make embed nodes not load if they have an object element ancestor with its own content.
Assignee | ||
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Whiteboard: btpp-active
Assignee | ||
Updated•9 years ago
|
Summary: Block embed content loading when ancestor is object element with content → Block embed content loading when ancestor is object element with usable content
Assignee | ||
Comment 2•9 years ago
|
||
Trying to figure out how things would work if the object tag hits the fallback case, since we've already parsed the embed element and skipped calling LoadObject. Should I just check for an embed element child when we hit the fallback case code and try to load it again there?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 3•9 years ago
|
||
That's probably the simplest thing.
The other option is that if an <embed> finds that it should not load due to an <object> up the tree that's not falling back, it could add itself to some list on that <object>. Then we'd have to worry about removing it from the list if it got removed from under the <object>, though, so I'm not sure it's worth it.
In practice, I expect there to be very few pages with large subtrees under <object>s that fall back, so just walking the subtree at that point seems fine.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 4•9 years ago
|
||
Ok, got a loading order issue here. Turns out that we already search descendants of a node when we have a fallback. However, I'm not quite sure how to deal with trying to load an embed with content at that point? Just calling StartObjectLoad ends up with LoadObject failing because we don't have a channel. Is there a better way to reload the embed node contents?
Attachment #8740097 -
Attachment is obsolete: true
Attachment #8744513 -
Flags: feedback?(bzbarsky)
![]() |
||
Comment 5•9 years ago
|
||
> Just calling StartObjectLoad ends up with LoadObject failing because we don't have a channel.
Er... don't have a channel at which point?
Anyway, you want to end up calling nsObjectLoadingContent::LoadObject but possibly passing "true" for the aForceLoad arg. Though if we never called LoadObject before, doesn't seem like that should be needed...
![]() |
||
Updated•9 years ago
|
Flags: needinfo?(kyle)
![]() |
||
Comment 6•9 years ago
|
||
Comment on attachment 8744513 [details] [diff] [review]
Patch 1 (v2): WIP - Block embed content loading when ancestor of object element with content
You can't stop at the fist non-param thing, unfortunately. You might have <embed> nested somewhere under you (e.g. under a <div>). Please add a test for that.
>+ // If we have an ancestor that is an object with a source, it'll have an
>+ // associated loading type.
Except it's called the displayed type, not the loading type, right?
Flags: needinfo?(kyle)
Attachment #8744513 -
Flags: feedback?(bzbarsky) → feedback+
![]() |
||
Comment 7•9 years ago
|
||
Or for that matter you could have a <div> and then a sibling <embed>. ;)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8744513 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Ok, I /think/ this is the right way to traverse the node tree, and the tests cover the basic cases.
Attachment #8745647 -
Attachment is obsolete: true
Attachment #8745830 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•9 years ago
|
||
Comment on attachment 8745830 [details] [diff] [review]
Patch 1 (v4): Block embed content loading when ancestor of object element with content
Why are we only looking for the first <embed>? We should load _all_ our descendant <embed>s, no? Worth adding a testcase for this too.
You just want to walk over the subtree rooted at the <object> using nsINode::GetNextNode.
>+ object->LoadObject(false, true);
This is wrong. We want to call something that will check BlockEmbedContentLoading() again before calling LoadObject, because the embed might still be blocked. For example if it's nested in another, not-falling-back, <object>. Again, need a test.
Attachment #8745830 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 11•9 years ago
|
||
Traversal cleaned up, now load all valid embeds, added more tests.
Attachment #8745830 -
Attachment is obsolete: true
Attachment #8745873 -
Flags: review?(bzbarsky)
![]() |
||
Comment 12•9 years ago
|
||
Comment on attachment 8745873 [details] [diff] [review]
Patch 1 (v5): Block embed content loading when ancestor of object element with content
> if (!child->IsHTMLElement(nsGkAtoms::param) &&
>- nsStyleUtil::IsSignificantChild(child, true, false)) {
>+ nsStyleUtil::IsSignificantChild(child, true, false)) {
Please add
aType != eFallbackAlternate
to the start of that if, so we don't keep testing kids of IsSignificantChild (which is not cheap) once we've found a significant child.
This is subtly different from the behavior we used to have before in the case of <param> that has kids, but I don't think we care about that, honestly. That can only happen with manual DOM manipulation or XML. But worth pointing out that we really only care about immediate kids for changing aType; it's just that non-immediate kids are probably going to be found only after aType == eFallbackAlternate anyway.
>+ if (child->IsHTMLElement(nsGkAtoms::embed)) {
>+ HTMLSharedObjectElement* object = HTMLSharedObjectElement::FromContent(child);
Since you added NS_IMPL_FROMCONTENT_HTML_WITH_TAG to HTMLSharedObjectElement, HTMLSharedObjectElement::FromContent will already do the tag check for you, and return null if the tag is not embed.
That's actually a bit weird. I'd rather we not add NS_IMPL_FROMCONTENT_HTML_WITH_TAG and just static_cast here directly.
>+ object->StartObjectLoad(false, true);
Should be (true, true) for the arguments, I would think. Why did you decide to have it not notify?
r=me with those addressed. Thank you for doing this!
Attachment #8745873 -
Flags: review?(bzbarsky) → review+
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Addressed review comments, fixed windows.h symbol undef.
Attachment #8745873 -
Attachment is obsolete: true
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Is a self closing object (<object ... />) valid? This bug is failing mochitests because the embed at https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/performance_timeline_main_test.html#97 thinks the object tag above it is actually a parent even though it closes itself. Adding </object> fixes it.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 17•9 years ago
|
||
> Is a self closing object (<object ... />) valid?
In HTML, no. Or rather, it's just an unclosed <object>. That test needs </object>.
Flags: needinfo?(bzbarsky)
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•