Closed Bug 1414244 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, task, P1)

Product:
Firefox
Component:
General
Type:
task

Tracking

()

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

People

(Reporter: bgrins, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [xbl-remove-unused])

Attachments

(7 files)

Bug 1414244 - Part 1 - Remove the "panelid" attribute.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1414244 - Part 2 - Remove the "viewtype" attribute.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1414244 - Part 3 - Remove the "panel-subviews" container.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1414244 - Part 4 - Remove the "panel-clickcapturer" element.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1414244 - Part 5 - Remove the "panel-mainview" container.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1414244 - Part 6 - Fold the "photonpanelmultiview" binding into "panelmultiview".
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1414244 - Part 7 - Remove unused code paths from PanelMultiview.jsm.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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)
Depends on: 1416192
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)
That was an artifact of cloning the bug. Thanks for spotting it!
No longer depends on: 1416192
Flags: needinfo?(paolo.mozmail)
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1
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 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 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 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 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 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 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 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+
(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.
(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.
(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.
Blocks: 1419408
(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.
Blocks: 1420128
(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 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+
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
Depends on: 1421853
Depends on: 1446800
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: