Replace customizationTarget expando property / XBL property with CustomizableUI.getCustomizationTarget

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: Gijs, Assigned: surkov, Mentored)

Tracking

(Blocks 1 bug)

Trunk
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

To fix this bug, we need to:

- add a getCustomizationTarget() public method to CustomizableUI ( https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm ). There should be an implementation on CustomizableUIInternal, and a stub one on CustomizableUI that just calls the CustomizableUIInternal one with all its argument, and returns the result. Both should take 1 argument, the node for which we need a customizationTarget.

If the node passed is null, return null. Otherwise, check for a `_customizationTarget` property on the node. If it doesn't exist, get the `customizationtarget` attribute on the node. If that is non-empty (truthy), run `document.getElementById()` on it, and assign to node._customizationTarget. If the attribute doesn't exist either, assign the node to node._customizationTarget . Always return `node._customizationTarget` (which is then guaranteed to be defined).


- remove the `customizationTarget` XBL implementation ( https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/customizableui/content/toolbar.xml#68 )
- remove the `insertItem` XBL implementation ( https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/customizableui/content/toolbar.xml#156 and https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/customizableui/content/toolbar.xml#39 )
- remove this test:
browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js

and remove it from browser/components/customizableui/test/browser.ini

- replace any `.customizationTarget` accesses in CustomizableUI.jsm, toolbar.xml, and CustomizeMode.jsm (e.g. https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/customizableui/CustomizeMode.jsm#521 ) with `CustomizableUI.getCustomizationTarget(node)`.
- replace all those accesses in the test files in browser/components/customizableui/test/ . Just searching for `customizationTarget` should find all of them.
Posted patch wip (obsolete) — Splinter Review
this one makes the tests asserting [1]

Assertion failure: aElement->HasServoData() (Element without Servo data on a post-traversal? How?)

I assume there's some problem with the patch, but c++ probably shouldn't assert on JS. Emilio, could you take a look please if this is something that needs a fix on c++ side?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=209100426&repo=try&lineNumber=1575
Assignee: nobody → surkov.alexander
Flags: needinfo?(emilio)
(Looking)
Hmm, I haven't been able to repro that at all, running the tests in dom/tests/mochitest/chrome, plus some others I saw asserting there, with that patch applied...

Is there any way you can repro that locally? Maybe the base revision was somehow busted?
Flags: needinfo?(emilio)
(This was a --enable-debug --disable-optimize build if it matters, but it shouldn't)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> (This was a --enable-debug --disable-optimize build if it matters, but it
> shouldn't)

on try-server a11y tests shows that assertion is on linux only (both debug and release) [1], no luck to reproduce locally on os x 

[1] TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/actions/test_link.html | application terminated with exit code 11
Could you please try 
./mach test accessible/tests/mochitest/actions/
on linux (comment #6) and see if you can reproduce it?
Flags: needinfo?(emilio)
Comment on attachment 9021745 [details] [diff] [review]
wip

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

With the patch applied, the browser gets broken, when Customize tab is open, i.e. tabs are not switching, and console has error:
JavaScript error: chrome://browser/content/tabbrowser.js, line 1116: TypeError: gURLBar.closePopup is not a function

Gijs, could you check if I miss anything obvious here please?
Attachment #9021745 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to alexander :surkov (:asurkov) from comment #7)
> Could you please try 
> ./mach test accessible/tests/mochitest/actions/
> on linux (comment #6) and see if you can reproduce it?

No such luck :(
Flags: needinfo?(emilio)
(In reply to alexander :surkov (:asurkov) from comment #8)
> Comment on attachment 9021745 [details] [diff] [review]
> wip
> 
> Review of attachment 9021745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With the patch applied, the browser gets broken, when Customize tab is open,
> i.e. tabs are not switching, and console has error:
> JavaScript error: chrome://browser/content/tabbrowser.js, line 1116:
> TypeError: gURLBar.closePopup is not a function
> 
> Gijs, could you check if I miss anything obvious here please?

FWIW on a debug build this patch spews:

XML Parsing Error: mismatched tag. Expected: </implementation>.
Location: file:///home/emilio/src/moz/gecko-beta/browser/components/customizableui/content/toolbar.xml
Line Number 38, Column 5:
  </binding>

So that sounds pretty likely to be the cause of most of this havoc.
Comment on attachment 9021745 [details] [diff] [review]
wip

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

What Emilio said. This generally seems to be heading in the right direction, though I noticed one other subtle bug that makes me worry if there are others hiding - you probably want to make sure you run the tests in browser/components/customizableui/ locally and that they all pass, once the issue you describe is fixed.

::: browser/components/customizableui/CustomizableUI.jsm
@@ +1379,5 @@
>      // in a different toolbar.
>      let node = document.getElementById(aId);
>      if (node) {
>        let parent = node.parentNode;
> +      while (parent && !(this.getCustomizationTarget(parent) ||

This loop depends on `node.customizationTarget` being falsy for nodes other than customizable areas (toolbars + overflow panel container), but your implementation of `getCustomizationTarget` always returns a node, so this needs rewriting or `getCustomizationTarget` needs updating to Not Do That.

There might be other consumers that implicitly depend on that, that'd require more careful auditing that I need to be more awake to do.

@@ +4221,5 @@
>    this._toolbar.addEventListener("underflow", this);
>  
>    this._toolbar.setAttribute("overflowable", "true");
>    let doc = this._toolbar.ownerDocument;
> +  this._target = this.getCustomizationTarget(this._toolbar);

This won't work - `OverflowableToolbar` obviously doesn't have a method called `getCustomizationTarget` - you want to call CustomizableUI.getCustomizationTarget instead.
Attachment #9021745 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
FWIW, if you could put the next iteration up on phabricator, that'd really help with reviewing, as it easily offers access to more context etc.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

> XML Parsing Error: mismatched tag. Expected: </implementation>.
> Location:
> file:///home/emilio/src/moz/gecko-beta/browser/components/customizableui/
> content/toolbar.xml
> Line Number 38, Column 5:
>   </binding>
> 
> So that sounds pretty likely to be the cause of most of this havoc.

(In reply to :Gijs (he/him) from comment #11)
> Comment on attachment 9021745 [details] [diff] [review]
> wip
> 
> Review of attachment 9021745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What Emilio said. This generally seems to be heading in the right direction,
> though I noticed one other subtle bug that makes me worry if there are
> others hiding - you probably want to make sure you run the tests in
> browser/components/customizableui/ locally and that they all pass, once the
> issue you describe is fixed.
> 
> ::: browser/components/customizableui/CustomizableUI.jsm
> @@ +1379,5 @@
> >      // in a different toolbar.
> >      let node = document.getElementById(aId);
> >      if (node) {
> >        let parent = node.parentNode;
> > +      while (parent && !(this.getCustomizationTarget(parent) ||
> 

> There might be other consumers that implicitly depend on that, that'd
> require more careful auditing that I need to be more awake to do.
> 
> @@ +4221,5 @@
> >    this._toolbar.addEventListener("underflow", this);
> >  
> >    this._toolbar.setAttribute("overflowable", "true");
> >    let doc = this._toolbar.ownerDocument;
> > +  this._target = this.getCustomizationTarget(this._toolbar);
> 
> This won't work - `OverflowableToolbar` obviously doesn't have a method
> called `getCustomizationTarget` - you want to call
> CustomizableUI.getCustomizationTarget instead.

sorry, didn't upload a fresh version, my local version had those fixed

> This loop depends on `node.customizationTarget` being falsy for nodes other
> than customizable areas (toolbars + overflow panel container), but your
> implementation of `getCustomizationTarget` always returns a node, so this
> needs rewriting or `getCustomizationTarget` needs updating to Not Do That.
> 

aha, thanks for the catch, it has to be a problem, checking
Posted patch patchSplinter Review
Attachment #9021745 - Attachment is obsolete: true
Attachment #9022910 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #12)
> FWIW, if you could put the next iteration up on phabricator, that'd really
> help with reviewing, as it easily offers access to more context etc.

it appears phabricator doesn't work with mq. It says: "Found patches applied with `mq`, unable to continue". How do manage your patches having no mq?
(In reply to alexander :surkov (:asurkov) from comment #15)
> (In reply to :Gijs (he/him) from comment #12)
> > FWIW, if you could put the next iteration up on phabricator, that'd really
> > help with reviewing, as it easily offers access to more context etc.
> 
> it appears phabricator doesn't work with mq. It says: "Found patches applied
> with `mq`, unable to continue". How do manage your patches having no mq?

See https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/workflows.html. But if you want to keep using mq, as long as you qfinish any applied patches before pushing they should turn into normal hg commits. Phabricator will then rewrite the commit message so you can then qimport the rev with the new ID after pushing to phab.
Comment on attachment 9022910 [details] [diff] [review]
patch

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

r=me with the below addressed.

::: browser/components/customizableui/CustomizableUI.jsm
@@ +552,5 @@
>      this.saveState();
>    },
>  
> +  getCustomizationTarget(aEl)
> +  {

Nit: brace on previous line is prevailing style in this file. Also, please use at least `aElement` as a variable name - we're not short on space here.

@@ +1001,5 @@
>      if (gBuildAreas.has(aArea) && gBuildAreas.get(aArea).has(aPanelContents)) {
>        return;
>      }
>  
> +    aPanelContents._customizationTarget = aPanelContents;

Can you file a follow-up to get rid of this? AFAICT from a quick read of the code, we could just set customizable=true in the markup, and then we could remove these hacky (because assigning to 'private' variables) assignments.

@@ +2573,5 @@
>  
>      for (let node of buildAreaNodes) {
>        if (node.ownerGlobal == aWindow) {
> +        return this.getCustomizationTarget(node) ?
> +          this.getCustomizationTarget(node) : node;

Nit: simplify to:

return this.getCustomizationTarget(node) || node;

@@ +3948,5 @@
>      parent.appendChild(aSubview);
>    },
> +
> +  getCustomizationTarget(aEl) {
> +    return CustomizableUIInternal.getCustomizationTarget(aEl);

Same nit here about using a longer variable name

::: browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js
@@ +72,2 @@
>       "Widget " + id + " should be in " + containerId + " in customizing window.");
> +  is(CustomizableUI.getCustomizationTarget(otherWin.document.getElementById(containerId)).lastElementChild.id, id,

Nit: please use some temp vars to make this more readable, eg (pseudocode):

let thisTarget = CUI.getCT(doc.getEl(containerId));
let otherTarget = CUI.getCT(otherWin.doc.getEl(containerId));

is(thisTarget.last.first.id, id, ...);
is(otherTarget.last.id, id, ...);

::: browser/components/customizableui/test/browser_913972_currentset_overflow.js
@@ +14,5 @@
>  add_task(async function() {
>    let originalWindowWidth = window.outerWidth;
>    ok(!navbar.hasAttribute("overflowing"), "Should start with a non-overflowing toolbar.");
>    ok(CustomizableUI.inDefaultState, "Should start in default state.");
> +  let oldChildCount = CustomizableUI.getCustomizationTarget(navbar).childElementCount;

Nit: could probably just create a local var that points to the navbar's customization target element, and reuse that, to make this less verbose.

::: browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js
@@ -1,3 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> - * License, v. 2.0. If a copy of the MPL was not distributed with this
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

I know my comment said to remove this test, but I think I've changed my mind - this should continue to be useful, at least for now. Can you restore it, please? (also to browser.ini)
Attachment #9022910 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3da3f53f804
replace customizationTarget XBL property by CustomizableUI method, r=gijs
Blocks: 1505674
https://hg.mozilla.org/mozilla-central/rev/b3da3f53f804
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.