Bug 1026047 (shadowcurrentdoc)

Figure out what to do to Shadow DOM's IsInDoc/GetCurrentDoc()

RESOLVED FIXED in mozilla33

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 6 bugs)

29 Branch
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Bug 992521 broke some functionality because it added new invariants
("Not-in-doc-but-has-nsIFrames" etc).

In bug 992521 only some parts of code was checked whether changes for the
IsInDoc/GetCurrentDoc/GetDocument usage are needed.

I think we should re-evaluate the IsInDoc behavior we want and then
go through _all_ the code using IsInDoc()/GetCurrentDoc()/GetDocument().
And we should also rename IsInDoc() and GetCurrentDoc(), and get rid of the ancient GetDocument().
(Assignee)

Updated

3 years ago
Summary: Figure out how to handle Shadow DOM's IsInDoc/GetCurrentDoc() → Figure out what to do to Shadow DOM's IsInDoc/GetCurrentDoc()
My suggestion is to add a GetDisplayDoc() which returns the same thing as GetCrossShadowCurrentDoc().

Then change the layout code to use this function as appropriate.

And yes, I too would love to see GetDocument() go away. It has been slated for death for a decade now I think :)
Blocks: 1026164
We already have a concept of "display document" which means something different, fwiw.  See nsIDocument::GetDisplayDocument.
GetRenderDoc()?
Works for me.  I suspect we'll want to replace pretty much all the GetDocument/GetCurrentDoc() callsites with that, but we'll see.
(Assignee)

Comment 5

3 years ago
I think we need to also rename GetCurrentDoc() and IsInDoc() so that whoever uses them realizes
in case of shadow dom null/false is always returned.

We should have a name for the document which doesn't contain shadow stuff, and then for the tree which contains also shadow dom.
RenderDoc sounds ok-ish for the latter, or ComposedDoc, that would be closer to the spec terminology, and doesn't
hint anything about rendering, which shadow dom doesn't necessarily have.

BaseDoc for the first one? DOMDoc would be nice, but has different meaning in Gecko.
Or even UncomposedDoc? Looks ugly and is a bit long, but would be rather clear.


So IsInUncomposedDoc()/GetUncomposedDoc() and IsInComposedDoc()/GetComposedDoc()  and remove IsInDoc()/GetCurrentDoc()/GetDocument()/GetCrossShadowCurrentDoc() ?
So I like IsInComposedDoc()/GetComposedDoc().

IsInUncomposedDoc() is not great, though.  How about IsInDocumentTree()?  And maybe GetDocumentTreeRoot() instead of GetUncomposedDoc()?  Not totally happy with that one, unlike IsInDocumentTree().

We definitely want to remove the things you list as needing removal.
If we wanted to align with the spec terminology we would probably want:

IsInRootTree(), GetRootTreeDoc(), IsInComposedTree(), GetComposedTreeDoc()
(Assignee)

Comment 8

3 years ago
IsInDocumentTree is ok, but GetDocumentTreeRoot()...
I think the name should indicate that the return value is a Document object.

IsInRootTree() doesn't really indicate the node needs to be in composed (or uncomposed) doc - just that it is in a tree of subtrees.
Blocks: 1024428
Alias: shadowcurrentdoc
> I think the name should indicate that the return value is a Document object.

GetDocumentRootOfTree?  I still don't like it.  Maybe GetRootDocument()?  That might confuse people in terms of frame/subframe relationship.

GetRootAsDocument?  GetTreeRootAsDocument?  GetTreeRootDoc?

I agree that IsInRootTree() doesn't mean what we want.  That just says we're in the light DOM; a root tree may not have a document root.  I'm not sure that IsInComposedTree() does what we want either: you can be in a composed tree that's not rooted at a document, right?

The spec term for the thing we want is "document tree" hence IsInDocumentTree()...
(Assignee)

Comment 10

3 years ago
When not thinking shadow dom, IsInDocumentTree() sounds pretty much the same as IsInDoc(), 
and doesn't really say much whether we're talking about DOM tree, or flattened tree, IMO.
(shadom dom spec doesn't talk about flattened tree.)
Sure, 'document tree' is a spec term, but not necessarily very good one.
(Assignee)

Comment 11

3 years ago
IsInCoreDoc() / GetCoreDoc()
IsInComposedDoc() / GetComposedDoc()
?
I still suspect that RenderDoc will be more intuitively understandable to more gecko hackers than ComposedDoc. And it's shorter to type.
(Assignee)

Comment 13

3 years ago
But you may have documents without any rendering. That is why I don't like RenderDoc
(Assignee)

Comment 14

3 years ago
But I still prefer composed/uncomposed. That forces the API user to think about shadow dom composition.
(Assignee)

Comment 15

3 years ago
Created attachment 8445437 [details] [diff] [review]
proposal 1

So this add methods for the following cases (which I think cover all the interesting cases. We haven't had anything like GetRenderDoc and I don't see why we'd need such)
- is the node in a DOM tree where Document object is the root?
- if the node is in a DOM tree where Document object is the root, return the Document object.
- is the node in flattened tree where a Document object is the root?
- if the node is in a flattened tree where a Document object is the root, return the Document object.
- is the node in a shadow dom.

Use of composed/uncomposed force, IMO, the caller to think whether shadow dom should be taken care of.
If uncomposed is replaced with something else, that something should make it clear that shadow dom isn't being handled.
Attachment #8445437 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

3 years ago
Note, once we propagate is-in-doc state to shadow trees (via BindToTree, I think), we can
improve performance of these checks for shadow dom nodes a bit.
(Assignee)

Comment 17

3 years ago
(In reply to Olli Pettay [:smaug] from comment #16)
> Note, once we propagate is-in-doc state to shadow trees (via BindToTree, I
> think),
and I'm not talking about the current IsInDoc(), but the stuff that needs to happen when
an element is added to a composed tree, which has doc as root.
Comment on attachment 8445437 [details] [diff] [review]
proposal 1

OK, let's just go with these names for now.  They're searchable enough we can do a mass rename later if we find something better.

>+  bool IsInComposedDoc() const
>+  {
>+    return IsInUncomposedDoc() || GetComposedDoc();

This will do extra work for the not-in-shadow-tree, not in doc case, no?

How about:

  return IsInUncomposedDoc() || (IsInShadowTree() &&  GetComposedDocInternal());

?

r=me
Attachment #8445437 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

3 years ago
Sure. (I'm hoping to improve GetComposedDocInternal perf by having a flag for the state when it would
return non-null.)
(Assignee)

Comment 20

3 years ago
Created attachment 8450326 [details] [diff] [review]
faster IsInComposedDoc
Assignee: nobody → bugs
(Assignee)

Comment 21

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7bb4b37e0f
https://hg.mozilla.org/mozilla-central/rev/fb7bb4b37e0f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Updated

3 years ago
Blocks: 1037610
(Assignee)

Updated

3 years ago
Blocks: 1037612
(Assignee)

Updated

3 years ago
Blocks: 1037643
(Assignee)

Updated

3 years ago
Blocks: 1037664
(Assignee)

Updated

3 years ago
Blocks: 1037668
(Assignee)

Updated

3 years ago
Blocks: 1037687
(Assignee)

Updated

3 years ago
Blocks: 1037709
You need to log in before you can comment on or make changes to this bug.