Move popup related attributes from XULDocument to Document

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
2 months ago

People

(Reporter: bdahl, Assigned: bdahl)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

10 months ago
The attributes popupNode, popupRangeParent, popupRangeOffset, and tooltipNode all need to be moved from XULDocument.webidl to Document.webidl so that tooltips and popups work when we switch to a top level HTML window.
Comment hidden (mozreview-request)
Assignee

Comment 2

10 months ago
One thing I should note on the test changes:

Previously, popupNode and tooltipNode were available to chrome and content privilege XUL documents. Now that I've changed them to chrome only the attributes become "undefined" when loading a XUL content privilege page. I've updated the test files to fix this, but this really defeats the purpose of what those tests were originally testing. However, with remote XUL going away I don't think this is really a problem. I'm open to re-working the tests if needed though.
Assignee

Comment 3

10 months ago
Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

Found a bug, clearing r? until I fix it.
Attachment #8996846 - Flags: review?(bzbarsky)
Priority: -- → P2
Comment hidden (mozreview-request)
Assignee

Comment 5

10 months ago
I converted crashtest 473284.xul to a chrome test since crashtests load files as content privileged XUL.

Comment 6

10 months ago
mozreview-review
Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

https://reviewboard.mozilla.org/r/260870/#review268170

::: dom/base/nsDocument.cpp:10168
(Diff revision 2)
>      readyPromise->MaybeResolve(this);
>    }
>  }
>  
> +static JSObject*
> +GetScopeObjectOfNode(nsINode* node)

So... I know you just moved this code, but I wonder whether we actually need it.  Certainly the comments should stop referencing nsNodeSH::PreCreate, which hasn't existed in a while.  And I would expect the  new setup to not ever throw; just to use the current global if one cannot be gotten from the node.

Please adjust the comment and file a followup to consider deleting this code, ok?

::: dom/base/nsDocument.cpp:10195
(Diff revision 2)
> +{
> +  if (!mDocumentContainer) {
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> piWin = mDocumentContainer->GetWindow();

It's worth adding comment here acknowledging that it's not clear why this code can't just GetWindow().

::: dom/base/nsDocument.cpp:10215
(Diff revision 2)
> +        if (pm) {
> +            node = pm->GetLastTriggerPopupNode(this);
> +        }
> +    }
> +
> +    if (node && nsContentUtils::CanCallerAccess(node)

The nsContentUtils::CanCallerAccess check is pointless now.  If the caller is JS, this function is [ChromeOnly] so it's system-principal and will pass the check.  If the caller is C++... well, there aren't any C++ callers of this method.

::: dom/base/nsDocument.cpp:10244
(Diff revision 2)
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        return nullptr;
> +    }
> +
> +    nsINode* rangeParent = pm->GetMouseLocationParent();
> +    if (rangeParent && !nsContentUtils::CanCallerAccess(rangeParent)) {

Again, the CanCallerAccess check is pointless here.  So you can just do:

    return pm->GetMouseLocationParent();
    
and remove a bunch of code here.

::: dom/base/nsDocument.cpp:10253
(Diff revision 2)
> +
> +    return rangeParent;
> +}
> +
> +// Returns the rangeOffset element from the XUL Popup Manager. We check the
> +// rangeParent to determine if the caller has rights to access to the data.

Again, should just remove that stuff.

::: dom/base/nsDocument.cpp:10264
(Diff revision 2)
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        return 0;
> +    }
> +
> +    nsINode* rangeParent = pm->GetMouseLocationParent();
> +    if (rangeParent && !nsContentUtils::CanCallerAccess(rangeParent)) {

Again, remove CanCallerAccess.  And then we don't need this rangeParent thing at all.

::: dom/base/nsDocument.cpp:10278
(Diff revision 2)
> +nsIDocument::GetTooltipNode()
> +{
> +  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
> +  if (pm) {
> +    nsCOMPtr<nsINode> node = pm->GetLastTriggerTooltipNode(this);
> +    if (node && nsContentUtils::CanCallerAccess(node)) {

Again, remove CanCallerAccess.

::: dom/base/nsIDocument.h:1386
(Diff revision 2)
>    /**
>     * Clears any Servo element data stored on Elements in the document.
>     */
>    void ClearStaleServoData();
>  
> +  already_AddRefed<nsPIWindowRoot> GetWindowRoot();

Needs to be documented.

::: dom/webidl/Document.webidl:412
(Diff revision 2)
> +
> +  [ChromeOnly]
> +  attribute Node? popupNode;
> +
> +  /**
> +   * These attributes correspond to trustedGetPopupNode().rangeOffset and

There is no trustedGetPopupNode().

I guess say that these correspond to the rangeParent and rangeOffset of the popup node or something?  The original documentation here is not great.  :(

::: toolkit/content/tests/chrome/popup_trigger.js:456
(Diff revision 2)
>        for (var t = 0; t < 2; t++) {
>          var child = childframe.contentDocument;
>          var evt = child.createEvent("Event");
>          evt.initEvent("click", true, true);
>          child.documentElement.dispatchEvent(evt);
> -        is(child.documentElement.getAttribute("data"), "xnull",
> +        is(child.documentElement.getAttribute("data"), "xundefined",

Why this change?  Commit message should say.

::: toolkit/content/tests/chrome/window_tooltip.xul:116
(Diff revision 2)
>  
>      var child = $("childframe").contentDocument; 
>      var evt = child.createEvent("Event");
>      evt.initEvent("click", true, true);
>      child.documentElement.dispatchEvent(evt);
> -    is(child.documentElement.getAttribute("data"), "xnull",
> +    is(child.documentElement.getAttribute("data"), "xundefined",

And this one.
Attachment #8996846 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Assignee

Updated

10 months ago
Blocks: 1481279
Assignee

Comment 8

10 months ago
mozreview-review-reply
Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

https://reviewboard.mozilla.org/r/260870/#review268170

> So... I know you just moved this code, but I wonder whether we actually need it.  Certainly the comments should stop referencing nsNodeSH::PreCreate, which hasn't existed in a while.  And I would expect the  new setup to not ever throw; just to use the current global if one cannot be gotten from the node.
> 
> Please adjust the comment and file a followup to consider deleting this code, ok?

https://bugzilla.mozilla.org/show_bug.cgi?id=1481279

> Why this change?  Commit message should say.

I left a comment in the bug about this, but I'll add it the commit message too.

Comment 9

10 months ago
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f054fef3e0b
Move XULDocument popup attributes to Document. r=bz

Comment 10

10 months ago
mozreview-review
Comment on attachment 8996846 [details]
Bug 1480206 - Move XULDocument popup attributes to Document.

https://reviewboard.mozilla.org/r/260870/#review268614

::: dom/webidl/Document.webidl:410
(Diff revision 3)
>    // fired DOMContentLoaded and are ready to start layout.  This is used for the
>    // "document_idle" webextension script injection point.
>    [ChromeOnly, Throws]
>    readonly attribute Promise<Document> documentReadyForIdle;
> +
> +  [ChromeOnly]

Would using `Func="IsChromeOrXBL"` here help with solving the test issues mentioned in Comment 2?
Assignee

Comment 11

10 months ago
(In reply to ExE Boss from comment #10)
> Would using `Func="IsChromeOrXBL"` here help with solving the test issues
> mentioned in Comment 2?

Nope, the pages are XUL with content privilege which means IsChromeOrXBL=false.

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f054fef3e0b
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.