Merge 'panelmultiview' and 'photonpanelmultiview' into a single binding

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bgrins, Assigned: Paolo)

Tracking

(Blocks 3 bugs)

unspecified
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox59 fixed)

Details

(Whiteboard: [xbl-remove-unused])

Attachments

(7 attachments)

(Reporter)

Description

2 years ago
Once we aren't referencing <panelmultiview> directly anymore, we should get rid of the binding, either by folding the panelmultiview code into photonpanelmultiview or vice versa.

  > panelmultiview Used features: resources (1), content (1), children (1), constructor (1), destructor (1)
    > photonpanelmultiview Used features: content (1), children (1)
(Assignee)

Updated

2 years ago
Depends on: 1416192

Comment 1

2 years ago
Paolo, why does the removal of the old binding depend on bug 1416192? The patch in bug 1409301 already stops using the old binding, right?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 2

2 years ago
That was an artifact of cloning the bug. Thanks for spotting it!
No longer depends on: 1416192
Flags: needinfo?(paolo.mozmail)
(Assignee)

Updated

2 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1417042
Attachment #8927839 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Attachment #8927840 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Attachment #8927841 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Attachment #8927842 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Attachment #8927843 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Attachment #8927844 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)
Attachment #8927845 - Flags: review?(mdeboer) → review?(gijskruitbosch+bugs)

Comment 11

a year ago
mozreview-review
Comment on attachment 8927839 [details]
Bug 1414244 - Part 1 - Remove the "panelid" attribute.

https://reviewboard.mozilla.org/r/199122/#review206560
Attachment #8927839 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8927840 [details]
Bug 1414244 - Part 2 - Remove the "viewtype" attribute.

https://reviewboard.mozilla.org/r/199124/#review206562

Tentative r+, but see issues below.

::: browser/components/customizableui/PanelMultiView.jsm:169
(Diff revision 1)
>    get _panel() {
>      return this.node.parentNode;
>    }
>  
>    get showingSubView() {
> -    return this.node.getAttribute("viewtype") == "subview";
> +    return !!this._showingSubView;

We don't need the boolean coercion here if we set a default value on every instance by just setting this to false in the constructor, right? That seems neater to me.

::: browser/components/customizableui/content/panelUI.css:22
(Diff revision 1)
> -
>  .panel-subviews[panelopen] {
>    transition: transform var(--panelui-subview-transition-duration);
>  }
>  
> -.panel-viewcontainer[panelopen]:-moz-any(:not([viewtype="main"]),[transitioning]) {
> +.panel-viewcontainer[panelopen] {

I think we should keep the `[transitioning]` selector here? Or is there some reason it is no longer necessary?
Attachment #8927840 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 13

a year ago
mozreview-review
Comment on attachment 8927841 [details]
Bug 1414244 - Part 3 - Remove the "panel-subviews" container.

https://reviewboard.mozilla.org/r/199126/#review206580
Attachment #8927841 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 14

a year ago
mozreview-review
Comment on attachment 8927842 [details]
Bug 1414244 - Part 4 - Remove the "panel-clickcapturer" element.

https://reviewboard.mozilla.org/r/199128/#review206584
Attachment #8927842 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 15

a year ago
mozreview-review
Comment on attachment 8927843 [details]
Bug 1414244 - Part 5 - Remove the "panel-mainview" container.

https://reviewboard.mozilla.org/r/199130/#review206586
Attachment #8927843 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8927844 [details]
Bug 1414244 - Part 6 - Fold the "photonpanelmultiview" binding into "panelmultiview".

https://reviewboard.mozilla.org/r/199132/#review206588

Nice. Nearly there, just a few questions left.

::: browser/base/content/browser.css:89
(Diff revision 1)
>  panelview:not([mainview]):not([current]):not([in-transition]) {
>    transition: visibility 0s linear var(--panelui-subview-transition-duration);
>    visibility: collapse;
>  }
>  
> -photonpanelmultiview panelview:not([current]):not([in-transition]) {
> +panelview:not([current]):not([in-transition]) {

Shouldn't we just remove the rule above this one, and remove `transition: none` from this one? Or is this still doing something useful? It's not clear to me that it is, though they got modified in bug 1401991, and the second one, confusingly, even prior to your change AFAICT didn't have the specificity to override the previous one...

::: browser/components/customizableui/CustomizableUI.jsm:4326
(Diff revision 1)
> -      let photonView = this._panel.querySelector("photonpanelmultiview");
> +      let multiview = this._panel.querySelector("panelmultiview");
>        let contextMenu;
> -      if (photonView) {
> -        let mainViewId = photonView.getAttribute("mainViewId");
> +      if (multiview) {
> +        let mainViewId = multiview.getAttribute("mainViewId");
>          let mainView = doc.getElementById(mainViewId);
>          contextMenu = doc.getElementById(mainView.getAttribute("context"));
>        } else {
>          contextMenu = doc.getElementById(this._panel.getAttribute("context"));
>        }

The else block is dead now. Please outdent and remove the if/else check here, and collapse the declaration and assignment to `contextMenu`.

::: browser/themes/osx/customizableui/panelUI.css:19
(Diff revision 1)
> -photonpanelmultiview .toolbaritem-combined-buttons > spacer {
> +panelmultiview .toolbaritem-combined-buttons > spacer {
>    width: 42px; /* 18px toolbarbutton padding + 16px icon + 8px label padding start */
>  }

Here and elsewhere, can we get rid of the descendant tagname selector? That would really be better for both perf and readability / number of overriding selectors. I bet for some of these, there are other rules that are now always overridden that we can get rid of.

For this particular rule, I don't see any rules that don't have the `photonpanelmultiview` prefix, nor do I see anything on macOS or Windows that would override this rule if it was made less specific.

::: browser/themes/shared/customizableui/panelUI.inc.css:940
(Diff revision 1)
>  .subviewbutton[checked="true"]:-moz-locale-dir(rtl) {
>    background-position: center right 7px;
>  }

Uh, this rule is dead now, right? Like, even deader than before? :-)

::: browser/themes/shared/customizableui/panelUI.inc.css:952
(Diff revision 1)
>    border: 0;
>    border-radius: 0;
>    margin-inline-end: 8px;
>  }
>  
> -photonpanelmultiview .toolbaritem-combined-buttons > label {
> +panelmultiview .toolbaritem-combined-buttons > label {

In these rules too I would prefer to get rid of the descendant tag selector.

::: browser/themes/shared/customizableui/panelUI.inc.css:1012
(Diff revision 1)
> -photonpanelmultiview .addon-banner-item::after,
> -photonpanelmultiview .panel-banner-item::after {
> +panelmultiview .addon-banner-item::after,
> +panelmultiview .panel-banner-item::after {

Here we can definitely drop `panelmultiview`

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 1)
>  
>  #widget-overflow > .panel-arrowcontainer > .panel-arrowcontent {
>    padding: 0;
>  }
>  
> -.cui-widget-panelview,

This still applies to the bookmarks menu button's popup. Is there some other rule that makes this selector unnecessary that's not immediately obvious from here? Otherwise, this might need to just become an ID selector for that view, or something. Or, possibly, this is doing more harm than good because that view is supposed to be an autoscroller menu list, rather than having "normal" scrollbars...

::: browser/themes/shared/customizableui/panelUI.inc.css:1545
(Diff revision 1)
>    /* !important to override .cui-widget-panel toolbarbutton:not([wrap]) > .toolbarbutton-text
>     * selector further down. */
>    display: none !important;
>  }
>  
> -photonpanelmultiview .PanelUI-subView toolbarseparator {
> +panelmultiview #panelMenu_pocket {

Hmm. This is dead-ish. It's for the item that gets inserted next to this one: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#499 but that's always in a panelmultiview... So just remove the `panelmultiview` bit of the selector here.

I think the item is supposed to no longer exist in photon. Please could you file a followup bug to rm this rule and the JS that inserts it in the pocket system add-on.
Attachment #8927844 - Flags: review?(gijskruitbosch+bugs)

Comment 17

a year ago
mozreview-review
Comment on attachment 8927845 [details]
Bug 1414244 - Part 7 - Remove unused code paths from PanelMultiview.jsm.

https://reviewboard.mozilla.org/r/199134/#review206598

Thanks for working on this! :-)

(and apologies again about review slowness)
Attachment #8927845 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 18

a year ago
(In reply to :Gijs from comment #12)
> > -.panel-viewcontainer[panelopen]:-moz-any(:not([viewtype="main"]),[transitioning]) {
> > +.panel-viewcontainer[panelopen] {
> 
> I think we should keep the `[transitioning]` selector here? Or is there some
> reason it is no longer necessary?

The :not([viewtype="main"]) selector always matches Photon panels regardless of what view is shown, so the new rule should be equivalent, if I read the old one correctly.

Maybe this should have had the selector, but I wouldn't make these fixes in the same changeset as the refactoring. While removing the non-Photon code, things that could be fixed or optimized will become more apparent, and we can tackle those separately.
(Assignee)

Comment 19

a year ago
(In reply to :Gijs from comment #16)
> ::: browser/themes/shared/customizableui/panelUI.inc.css
> > -.cui-widget-panelview,
> 
> This still applies to the bookmarks menu button's popup.

I removed a rule below that reset the effects of this one for the "cui-widget-panelview" class.

Comment 20

a year ago
(In reply to :Paolo Amadini from comment #19)
> (In reply to :Gijs from comment #16)
> > ::: browser/themes/shared/customizableui/panelUI.inc.css
> > > -.cui-widget-panelview,
> > 
> > This still applies to the bookmarks menu button's popup.
> 
> I removed a rule below that reset the effects of this one for the
> "cui-widget-panelview" class.

Sure, but that rule only applied to `photonpanelmultiview` descendants, which the bookmarks menu button's popup isn't.
(Assignee)

Updated

a year ago
Blocks: 1419408
(Assignee)

Comment 21

a year ago
(In reply to :Gijs from comment #20)
> Sure, but that rule only applied to `photonpanelmultiview` descendants,
> which the bookmarks menu button's popup isn't.

Ah, missed that.

(In reply to :Gijs from comment #16)
> Or, possibly, this is doing more harm than good because
> that view is supposed to be an autoscroller menu list, rather than having
> "normal" scrollbars...

It seems to have no effect on Mac. I'll test on other platforms, but the original rule sounds unnecessary.
(Assignee)

Updated

a year ago
Blocks: 1420128
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
(In reply to :Gijs from comment #16)
> > -photonpanelmultiview .toolbaritem-combined-buttons > label {
> > +panelmultiview .toolbaritem-combined-buttons > label {
> 
> In these rules too I would prefer to get rid of the descendant tag selector.

I agree the descendant selector is overused here, and there are several other rules where it can be worked around, but I'm spending more time making sense of this styling than I'm comfortable with. So, I split this issue into its own bug, in the interest of having the main refactoring land soon.

Comment 30

a year ago
mozreview-review
Comment on attachment 8927844 [details]
Bug 1414244 - Part 6 - Fold the "photonpanelmultiview" binding into "panelmultiview".

https://reviewboard.mozilla.org/r/199132/#review207850

r=me, OK, let's leave the combined buttons for bug 1420128.
Attachment #8927844 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 32

a year ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4312855b1107
Part 1 - Remove the "panelid" attribute. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f6b3152c7a
Part 2 - Remove the "viewtype" attribute. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/29ed6af31c62
Part 3 - Remove the "panel-subviews" container. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5eb229c2dd
Part 4 - Remove the "panel-clickcapturer" element. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49d25e5c124
Part 5 - Remove the "panel-mainview" container. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9a0be10b65
Part 6 - Fold the "photonpanelmultiview" binding into "panelmultiview". r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee02ed83f71
Part 7 - Remove unused code paths from PanelMultiview.jsm. r=Gijs

Updated

a year ago
Depends on: 1421853
Depends on: 1446800
You need to log in before you can comment on or make changes to this bug.