Remove the "ctrlTab-preview" binding

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

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

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

This is a required step of bug 1450823.
Comment on attachment 8976483 [details]
Bug 1462297 - Remove the contents of the "ctrlTab-preview" binding.

https://reviewboard.mozilla.org/r/244608/#review250686

::: browser/base/content/browser-ctrlTab.js:256
(Diff revision 1)
> +    let previewInner = document.createElement("vbox");
> +    previewInner.setAttribute("class", "ctrlTab-preview-inner");
> +    preview.appendChild(previewInner);

It might be worth checking if the inner box is still needed. I think it might be possible to set -moz-box-orient: vertical; on the parent button, and remove the child vbox.

::: browser/base/content/browser-ctrlTab.js:267
(Diff revision 1)
> +      canvas.setAttribute("style", "max-width:" + canvasWidth + "px;" +
> +                                   "min-width:" + canvasWidth + "px;" +
> +                                   "max-height:" + canvasHeight + "px;" +
> +                                   "min-height:" + canvasHeight + "px;");

canvas.style.maxWidth = 
canvas.style.maxHeight = ...


might be more elegant.
Comment on attachment 8976483 [details]
Bug 1462297 - Remove the contents of the "ctrlTab-preview" binding.

https://reviewboard.mozilla.org/r/244608/#review250922

If we were starting this feature from scratch today I think we could simplify how this is done quite a bit, but given that it's not even on by default it's not worth spending a bunch of time refactoring it as we go.

::: browser/base/content/browser-ctrlTab.js:238
(Diff revision 1)
>    },
>    observe(aSubject, aTopic, aPrefName) {
>      this.readPref();
>    },
>  
> +  makePreview: function ctrlTab_makePreview(aIsShowAllButton) {

I guess we are safe from the footguns that  `parseXULToFragment` around XBL construction that is meant to avoid in this particular case, since we don't call/set anything on the XBL-bound elements being constructed here (button and label)?

::: browser/base/content/browser-ctrlTab.js:264
(Diff revision 1)
> +
> +    if (!aIsShowAllButton) {
> +      let canvasWidth = this.canvasWidth;
> +      let canvasHeight = this.canvasHeight;
> +
> +      let canvas = preview._canvas = document.createElement("hbox");

I don't love setting these expando properties on the nodes, but if we can figure out the focus/tabbing issues in Bug 1450823 then we should be able to convert this to a CE (registered only for this document and when the feature is enabled). Which would then let us move the DOM setup and getters onto that class.

::: browser/base/content/browser-tabPreviews.xml:10
(Diff revision 1)
> -
>  <bindings id="tabPreviews"
>            xmlns="http://www.mozilla.org/xbl"
>            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>            xmlns:xbl="http://www.mozilla.org/xbl">
>    <binding id="ctrlTab-preview" extends="chrome://global/content/bindings/button.xml#button-base">

Would it be possible to directly bind these elements to button-base and remove this file instead of having a pretty much empty binding? Or do we need this for `<content/>`?
Attachment #8976483 - Flags: review?(bgrinstead) → review+
(In reply to :ntim (busy until May 25th) from comment #2)
> It might be worth checking if the inner box is still needed. I think it
> might be possible to set -moz-box-orient: vertical; on the parent button,
> and remove the child vbox.

It looks like it's needed for providing the correct spacing. It may be possible to remove it and do thing differently, but as Brian mentioned it's probably not worth the effort right now. I explored a few refactoring opportunities like this, but they would result in too many changes.

I updated the code to use the style properties instead of the attribute.

> I guess we are safe from the footguns that  `parseXULToFragment` around XBL
> construction that is meant to avoid in this particular case, since we don't
> call/set anything on the XBL-bound elements being constructed here (button
> and label)?

Right, we only set attributes on the elements with bindings, so we should be fine.

> Would it be possible to directly bind these elements to button-base and
> remove this file instead of having a pretty much empty binding? Or do we
> need this for `<content/>`?

I thought we wouldn't want to bind to button-base, but that actually works fine here.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84a09a4a0dc2
Remove the "ctrlTab-preview" binding. r=bgrins
Summary: Remove the contents of the "ctrlTab-preview" binding → Remove the "ctrlTab-preview" binding
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a22438592c
Fix leftover reference to "browser-tabPreviews.xml". rs=Aryx on a CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/84a09a4a0dc2
https://hg.mozilla.org/mozilla-central/rev/e1a22438592c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.