Closed Bug 1417042 Opened 7 years ago Closed 7 years ago

Remove the "panelview" binding

Categories

(Firefox :: Toolbars and Customization, task, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

Bug 1417042 - Remove the "panelview" binding.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
We control when subviews are shown, so we can remove this binding and create the header elements directly. This also simplifies the navigation and event handling a bit.
Comment on attachment 8928150 [details]
Bug 1417042 - Remove the "panelview" binding.

I think this is in the bucket of replacing binding instances where we can use some other technique. This approach has the advantage of creating the header elements lazily, so they're not created for subviews that are never shown. I don't know if this could be done if <panelview> was a custom element.

While <panelviewheader> could definitely be a custom element, even with lazy loading, creating the elements in the <panelmultiview> implementation is probably the same amount of code.
Attachment #8928150 - Flags: feedback?(bgrinstead)
Note that this implementation assumes that the titles are static, in other words that the subviews are not reused with different titles. It think this is currently true, but the implementation could be easily changed to update the label if needed.
Blocks: 1416231
(In reply to :Paolo Amadini from comment #2)
> Comment on attachment 8928150 [details]
> Bug 1417042 - Remove the "panelview" binding.
> 
> I think this is in the bucket of replacing binding instances where we can
> use some other technique. This approach has the advantage of creating the
> header elements lazily, so they're not created for subviews that are never
> shown. I don't know if this could be done if <panelview> was a custom
> element.

Why might this not work with Custom Elements? If it's because of assigning the data-subviewbutton-tooltip on the <content> tag I think this would actually be more straightforward with CE, since we could grab the string value directly from our l10n framework in JS.
Attachment #8928150 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Comment on attachment 8928150 [details]
Bug 1417042 - Remove the "panelview" binding.

https://reviewboard.mozilla.org/r/199392/#review205668

This mostly looks fine, though it's drawn my attention to a few points that I've raised below.

::: browser/components/customizableui/PanelMultiView.jsm:377
(Diff revision 1)
> +    if (header && header.classList.contains("panel-header")) {
> +      // We already created a header, no need to update it.
> +      return;
> +    }

Can the title ever change? I know that this is a bit interesting for the items where we determine the title from the label attribute... IIRC if you put the bookmark toolbar items and/or the bookmarks menu button in the overflow panel, strange things can happen... Are you sure we don't need to check the new title matches the old one?

::: browser/components/customizableui/PanelMultiView.jsm:386
(Diff revision 1)
> +    backButton.classList.add(
> +      "subviewbutton",
> +      "subviewbutton-iconic",
> +      "subviewbutton-back"
> +    );

TBH, I would just set className to the string-concatenated version, but it's up to you.

::: browser/components/customizableui/PanelMultiView.jsm:397
(Diff revision 1)
> +    backButton.setAttribute("tabindex", "0");
> +    backButton.setAttribute("tooltip",
> +      this.node.getAttribute("data-subviewbutton-tooltip"));
> +    backButton.addEventListener("command", () => {
> +      // The panelmultiview element may change if the view is reused.
> +      backButton.closest("panelmultiview").goBack();

Why can't we use viewNode.panelMultiView here?

::: browser/components/customizableui/PanelMultiView.jsm:1079
(Diff revision 1)
>          stop();
>          let dir = this._dir;
>          if ((dir == "ltr" && keyCode == "ArrowLeft") ||
>              (dir == "rtl" && keyCode == "ArrowRight")) {
>            if (this._canGoBack(view))
> -            this.goBack(view.backButton);
> +            this.goBack(view.getElementsByClassName("subviewbutton-back")[0]);

Why getElementsByClassName rather than just `.querySelector()`? Also, why do we pass the back button here at all? As far as I can tell this gets used as the `anchor` for the showSubView call, which doesn't seem right anyway.
Attachment #8928150 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8928150 - Flags: feedback?(bgrinstead) → feedback+
(In reply to :Gijs from comment #6)
> Can the title ever change? I know that this is a bit interesting for the
> items where we determine the title from the label attribute...

I haven't found a case where a subview is reused by an item with a different label, even after trying some of the things you suggested, but maybe I didn't look close enough? Do you have more ideas about where this could happen?

> Why can't we use viewNode.panelMultiView here?

Probably we can! I'll try when I resume working on this.

> Why do we pass the back button here at all? As far as I can tell this gets used as
> the `anchor` for the showSubView call, which doesn't seem right anyway.

I believe the anchor element simply keeps the highlighting during the transition. However, I don't know whether this is important for this button. What do you think?
(In reply to :Paolo Amadini from comment #7)
> (In reply to :Gijs from comment #6)
> > Can the title ever change? I know that this is a bit interesting for the
> > items where we determine the title from the label attribute...
> 
> I haven't found a case where a subview is reused by an item with a different
> label, even after trying some of the things you suggested, but maybe I
> didn't look close enough? Do you have more ideas about where this could
> happen?

I think I was thinking of https://bugzilla.mozilla.org/show_bug.cgi?id=1389625#c5 . I don't know how that situation changes with this patch off-hand, but it'd be good to check.

> > Why do we pass the back button here at all? As far as I can tell this gets used as
> > the `anchor` for the showSubView call, which doesn't seem right anyway.
> 
> I believe the anchor element simply keeps the highlighting during the
> transition. However, I don't know whether this is important for this button.
> What do you think?

I don't think that's important for the back button, so I'd rather just stop passing it if that's the only thing. Simpler code = better code -- not like this file isn't complicated enough as it is. :-)
(In reply to :Gijs from comment #8)
> https://bugzilla.mozilla.org/show_bug.cgi?id=1389625#c5 . I don't know how
> that situation changes with this patch off-hand, but it'd be good to check.

Thanks, that does indeed end up using the cached title. I'm always amazed at how many customization scenarios we support...

(In reply to :Gijs from comment #8)
> not like this file isn't complicated enough as it is. :-)

Haha, totally agree :-)
Comment on attachment 8928150 [details]
Bug 1417042 - Remove the "panelview" binding.

Just small differences, but worth a quick sanity check.
Attachment #8928150 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8928150 [details]
Bug 1417042 - Remove the "panelview" binding.

https://reviewboard.mozilla.org/r/199392/#review208196

Thanks!
Attachment #8928150 - Flags: review?(gijskruitbosch+bugs) → review+
Backed out for browser-chrome failures, e.g. browser/components/customizableui/test/browser_987640_charEncoding.js and browser/base/content/test/urlbar/browser_page_action_menu.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0245a5ba4ba74b16c24771dae0e50fe22443898

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bce38a7817ee39ccd38f57f73f5acf81a41e1f69&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry

Failure log browser/components/customizableui/test/browser_987640_charEncoding.js: https://treeherder.mozilla.org/logviewer.html#?job_id=147845413&repo=mozilla-inbound
06:53:14     INFO - TEST-START | browser/components/customizableui/test/browser_987640_charEncoding.js
06:53:14     INFO - GECKO(811) | Waiting for browser load
06:53:14     INFO - GECKO(811) | Saw state f0001 and status 0
06:53:14     INFO - GECKO(811) | Saw state c0010 and status 0
06:53:14     INFO - GECKO(811) | Browser loaded http://mochi.test:8888/browser/browser/components/customizableui/test/support/test_967000_charEncoding_page.html
06:53:15     INFO - GECKO(811) | Waiting for browser load of http://mochi.test:8888/browser/browser/components/customizableui/test/support/test_967000_charEncoding_page.html
06:53:59     INFO - TEST-INFO | started process screencapture
06:53:59     INFO - TEST-INFO | screencapture: exit 0
06:53:59     INFO - Buffered messages logged at 06:53:14
06:53:59     INFO - Entering test bound 
06:53:59     INFO - Check Character Encoding panel functionality
06:53:59     INFO - Waiting for overflow button to have non-0 size
06:53:59     INFO - Buffered messages logged at 06:53:15
06:53:59     INFO - TEST-PASS | browser/components/customizableui/test/browser_987640_charEncoding.js | The western encoding is initially selected - 
06:53:59     INFO - Buffered messages finished
06:53:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_987640_charEncoding.js | Test timed out - 

Failure log browser/base/content/test/urlbar/browser_page_action_menu.js: https://treeherder.mozilla.org/logviewer.html#?job_id=147845417&repo=mozilla-inbound
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "Account Not Verified" == "Account Not Verified" - 
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "toolbarseparator" == "toolbarseparator" - 
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "-moz-box" == "-moz-box" - 
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | false == false - 
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - 
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "Verify Your Account..." == "Verify Your Account..." - 
06:53:42     INFO - Leaving test bound sendToDevice_syncNotReady_other_states
06:53:42     INFO - Entering test bound sendToDevice_syncNotReady_configured
06:53:42     INFO - Waiting for main page action button to have non-0 size
06:53:42     INFO - Waiting for main page action panel to be open
06:53:42     INFO - promisePageActionViewChildrenVisible waiting for a child node to be visible
06:53:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - 
06:53:42     INFO - promisePageActionViewShown waiting for ViewShown
06:53:42     INFO - Buffered messages finished
06:53:42     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_page_action_menu.js | 4 == 1 - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js :: checkSendToDeviceItems :: line 750
06:53:42     INFO - Stack trace:
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js:checkSendToDeviceItems:750
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js:testSendTabToDeviceMenu:301
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js:sendToDevice_syncNotReady_configured/</<:278
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js -> resource://testing-common/sinon-2.3.2.js:invoke:423
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js -> resource://testing-common/sinon-2.3.2.js:functionStub:2852
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js -> resource://testing-common/sinon-2.3.2.js:invoke:2426
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js -> resource://testing-common/sinon-2.3.2.js:proxy:2312
06:53:42     INFO - resource:///modules/PageActions.jsm:onShowing:1248
06:53:42     INFO - resource:///modules/PageActions.jsm:onShowing:1073
06:53:42     INFO - chrome://browser/content/browser-pageActions.js:doCommandForAction:548
06:53:42     INFO - chrome://browser/content/browser-pageActions.js:_makePanelButtonNodeForAction/<:143
06:53:42     INFO - chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:doApply:148
06:53:42     INFO - chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:apply:213
06:53:42     INFO - chrome://mochikit/content/tests/SimpleTest/EventUtils.js:synthesizeMouseAtPoint:426
06:53:42     INFO - chrome://mochikit/content/tests/SimpleTest/EventUtils.js:synthesizeMouse:357
06:53:42     INFO - chrome://mochikit/content/tests/SimpleTest/EventUtils.js:synthesizeMouseAtCenter:490
06:53:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js:sendToDevice_syncNotReady_configured/<:294
Flags: needinfo?(paolo.mozmail)
Assuming this isn't making 58, and doesn't need to.
status-firefox57: --- → wontfix
Priority: -- → P1
The test caught breakage in the "Send Tab to Device" page action, because the subview body was not seen as the first child of the "panelview" element anymore. This was simple to fix once the issue was identified.

Brian, is it correct that even with Custom Elements, but without Shadow DOM, we would have to handle issues like this on a case by case basis? I seem to remember that Shadow DOM for XUL will be implemented later, if ever.
Flags: needinfo?(paolo.mozmail) → needinfo?(bgrinstead)
Posting the base revision for screenshots, because mozilla-central uses a different product icon:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39f0ce8e1ee4dfa97583e7db8d2c8d6928d981e2
Err, actually de-58-backlogging this.
status-firefox58: --- → wontfix
https://hg.mozilla.org/mozilla-central/rev/6b5a357d277b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
(In reply to :Paolo Amadini from comment #17)
> The test caught breakage in the "Send Tab to Device" page action, because
> the subview body was not seen as the first child of the "panelview" element
> anymore. This was simple to fix once the issue was identified.
> 
> Brian, is it correct that even with Custom Elements, but without Shadow DOM,
> we would have to handle issues like this on a case by case basis? I seem to
> remember that Shadow DOM for XUL will be implemented later, if ever.

If I understand what happened in this case, I don't think Custom Elements would have had an effect. From my reading, the issue is that the "panelViewNode" variable at https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5a357d277b#l1.11 is now referencing a <panelmultiview> element instead of a <panelview> element, as in https://searchfox.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#12. So the DOM ordering has changed and the traversal needed to be updated.

I think for a Custom Element migration, there will also need to be some traversal changes needed case by case if calling code is digging into the XBL content (i.e. instead of `document.getAnonymousElementByAttribute(el, "foo", "bar");` we would have `el.querySelector("[foo=bar]")`. If/when we start using Shadow DOM then we'll need to change it again to something like `el.shadowRoot.querySelector("[foo=bar]");` (or extract whatever is happening away into a method on the Custom Element so that the calling code doesn't need to dig into the shadow content).
Flags: needinfo?(bgrinstead)
The panelViewNode variable still references the <panelview> element, but the first child is now the header box instead of the subview body box. The header used to be anonymous content before. I guess we'll need traversal changes for this case too.
Depends on: 1423891
Depends on: 1424672
Blocks: 1437512
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: