Closed Bug 1346623 Opened 7 years ago Closed 7 years ago

anonymous content created with nsIDocument::InsertAnonymousContent can change from non-native to native AC

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox54 + verified
firefox55 --- verified

People

(Reporter: heycam, Unassigned)

References

Details

Attachments

(4 files, 1 obsolete file)

chrome mochitest
1.24 KB, text/html
bholley
: feedback+
Details
Bug 1346623 - Part 1: Prevent canvas custom content from becoming NAC when reframing the root element.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.
59 bytes, text/x-review-board-request
bholley
: review+
gchang
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta-
Details
Bug 1346623 - Part 3: Test.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
We have the nsIDocument::InsertAnonymousContent API, exposed through nsIDOMWindowUtils, which allows script to create anonymous content and append it to the nsCanvasFrame.  This is used, among other things, by devtools to draw overlays in the page when selecting elements in the inspector.

Content added using this API is appended to a NAC div, nsCanvasFrame::mCustomContentContainer, but this content is not marked as NAC itself.  However, if the root element of the document is reframed, we end up cloning this custom content (in nsCanvasFrame::DestroyFrom), and returning it as part from nsCanvasFrame::CreateAnonymousContent, when we re-create the nsCanvasFrame.  The frame constructor calls SetNativeAnonymousBitOnDescendants on it, and this content is now considered native anonymous content.  This can change where we inherit styles from.
Attached file chrome mochitest — 
Here's a chrome mochitest demonstrating the problem.  The anonymous content div should remain green, but due to reframing the nsCanvasFrame, it starts being considered as NAC, meaning we start inheriting styles from further up the tree.
I haven't checked, so I don't know whether the anonymous content that devtools creates relies on normal inheritance in its AC subtree.  (It also creates ::before/::after pseudo-elements in the AC subtree, which is what was tripping up my changes in bug 1330843.)

We could note on the AC that it came from this API somehow, and so when recreating the nsCanvasFrame, we don't then mark it as native anonymous content.
Testing with https://hg.mozilla.org/try/rev/468aa60c40c350af59b1c07a9162290176086f87 to force the nsIDocument::InsertAnonymousContent to be NODE_IS_NATIVE_ANONYMOUS from the start, it looks like at least the eye dropper tool isn't styled properly.  So I guess we need to ensure that we don't add the NAC bit in that reframing case.
Attached patch patch v0.1 (obsolete) — — Splinter Review 
How about something like this?
Attachment #8846399 - Flags: feedback?(bobbyholley)
Attachment #8846399 - Attachment is patch: true
Comment on attachment 8846399 [details] [diff] [review]
patch v0.1

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

This seems like the right approach. Please check in the mochitest too.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4243,5 @@
> +    } else if (parentFrameType == nsGkAtoms::canvasFrame &&
> +               content == static_cast<nsCanvasFrame*>(aParentFrame)
> +                            ->GetCustomContentContainer()) {
> +      // don't mark descendants of the custom content container
> +      // as native anonymous

Please expand the comment to explain this in more detail, or to point to comment 0.

Also, worth re-ordering this to emphasize that we're still marking the root, just skipping the SetNativeAnonymousBitOnDescendants bit. I think we should probably keep the else {} block as-is, and make the SetNAtiveAnonymousBitOnDescendants call conditional.

@@ +4245,5 @@
> +                            ->GetCustomContentContainer()) {
> +      // don't mark descendants of the custom content container
> +      // as native anonymous
> +      content->SetIsNativeAnonymousRoot();
> +      content->SetFlags(NODE_IS_NATIVE_ANONYMOUS);

This line is redundant with the previous one.
Attachment #8846399 - Flags: feedback?(bobbyholley) → review+
Comment on attachment 8846393 [details]
chrome mochitest

Test looks good, though IIUC it needs a bit more work to be fully automated.
Attachment #8846393 - Flags: feedback+
Comment on attachment 8848089 [details]
Bug 1346623 - Part 1: Prevent canvas custom content from becoming NAC when reframing the root element.

https://reviewboard.mozilla.org/r/121064/#review123104
Attachment #8848089 - Flags: review?(bobbyholley) → review+
Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

https://reviewboard.mozilla.org/r/121066/#review123106

::: dom/webidl/AnonymousContent.webidl:86
(Diff revision 1)
> +  /**
> +   * Get the computed value of a property on an element inside this custom
> +   * anonymous content.
> +   */
> +  [Throws]
> +  DOMString? getComputedStylePropertyValue(DOMString elementId, 

Nit: Trailing whitespace.
Attachment #8848090 - Flags: review?(bobbyholley) → review+
Comment on attachment 8848091 [details]
Bug 1346623 - Part 3: Test.

https://reviewboard.mozilla.org/r/121068/#review123112

Nice! Thanks for doing this.
Attachment #8848091 - Flags: review?(bobbyholley) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9e40bc33e620
Part 1: Prevent canvas custom content from becoming NAC when reframing the root element. r=bholley
https://hg.mozilla.org/integration/autoland/rev/39ee9556d4a0
Part 2: Add an API to get computed style values through an AnonymousContent object. r=bholley
https://hg.mozilla.org/integration/autoland/rev/1f9a6ce59a56
Part 3: Test. r=bholley
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0696366039f2
Backed out changeset 1f9a6ce59a56 
https://hg.mozilla.org/integration/autoland/rev/2f9cdacc5359
Backed out changeset 39ee9556d4a0 
https://hg.mozilla.org/integration/autoland/rev/6ed991361ec1
Backed out changeset 9e40bc33e620 for perma leak in chrome-style stylo tests
Attachment #8846399 - Attachment is obsolete: true
Flags: needinfo?(cam)
Depends on: 1348493
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f85eb6b49794
Part 1: Prevent canvas custom content from becoming NAC when reframing the root element. r=bholley
https://hg.mozilla.org/integration/autoland/rev/f5a5a605ae82
Part 2: Add an API to get computed style values through an AnonymousContent object. r=bholley
https://hg.mozilla.org/integration/autoland/rev/cfe58f9ace35
Part 3: Test. r=bholley
We'd like to uplift bug 1349275, but it relies on the new `getComputedStylePropertyValue` method here added – by the way, thanks a lot for that, we really need it!

It would be possible uplift this bug too? It has some other dependencies…? Or it would be too risky…?
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
I would say too late for beta uplift. Maybe to aurora.
Not entirely sure what dependencies we'd need for the uplift (and how much we'd need to uplift to get what you want), so I'll leave this one to heycam.
Flags: needinfo?(bobbyholley)
I am only moderately confident that uplifting part 1 is OK, but I'd have to dig into it to see what the implications are to be sure.  Since you're just looking for getComputedStylePropertyValue, which is in part 2, and a safer part to uplift, are you happy to uplift just that part?
Flags: needinfo?(cam) → needinfo?(zer0)
(In reply to Cameron McCormack (:heycam) (away Apr 1-4) from comment #26)

> I am only moderately confident that uplifting part 1 is OK, but I'd have to
> dig into it to see what the implications are to be sure.  Since you're just
> looking for getComputedStylePropertyValue, which is in part 2, and a safer
> part to uplift, are you happy to uplift just that part?

I'm totally happy to uplift just that part, if the there are no others dependencies that prevent `getComputedStylePropertyValue` to works as expected. :)
Flags: needinfo?(zer0) → needinfo?(cam)
No other dependencies; uplifting just that part should be fine.
Flags: needinfo?(cam)
Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1312103
[User impact if declined]: devtools' infobar won't be properly displayed anymore – after the scrolling, it would be misplaced outside the viewport.
[Is this code covered by automated tests?]: yes (the usage of the API introduced)
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: See Bug 1349275
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: It shouldn't
[Why is the change risky/not risky?]: It's a new API used mostly by devtools
[String changes made/needed]: None

This part is necessary for the proper fix of bug 1349275, that needs to be uplifted urgently in both aurora and beta, since the devtools' infobar would be broken otherwise. I talked with :pbro, that talked with :lizzard, and we should be able to get this on 53.
Attachment #8848090 - Flags: approval-mozilla-beta?
Attachment #8848090 - Flags: approval-mozilla-aurora?
Sorry, too late for 53. We still might be able to try this on 54, I leave it up to gchang.
status-firefox53: --- → wontfix
status-firefox54: --- → affected
tracking-firefox54: --- → +
Attachment #8848090 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8848090 [details]
Bug 1346623 - Part 2: Add an API to get computed style values through an AnonymousContent object.

Fix a display issue of devtools' infobar. Aurora54+.
Attachment #8848090 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
I have verified that this issue is not reproducible anymore using Firefox 55.0a1 (Build Id:20170427030231) and Firefox 54.0b2 (Build Id:20170424145525) on Windows 10 64bit, Ubuntu 14.04 64bit and macOS 10.11.6.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: