Closed Bug 1354141 Opened 7 years ago Closed 7 years ago

Allow for panelmultiview subviews to be multiple levels deep

Categories

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

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.5 - May 15
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(2 files, 4 obsolete files)

Panel recording
1.40 MB, video/ogg
Details
Bug 1354141 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
In practice, this will mean that it's best to not impose a limit on the subview depth.
A breadcrumb will need to kept around to allow for backward-navigation, using the back button that will always be visible.
Blocks: photon-structure
No longer blocks: 1353360
Depends on: 1353360
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] → [photon-structure]
Since I am tackling the design bits in bug 1353360, I was experimenting with this bug to get to a neat implementation here.

I'll provide a PoC patch today for the team to look at, because this is one of the more risky areas of the project re. unknown unknowns.
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review134810

::: browser/components/customizableui/PanelMultiView.jsm:252
(Diff revision 1)
> +      container.removeEventListener("transitionend", onTransitionEnd);
> +      this._transitioning = false;
> +    };
> +
> +    container.addEventListener("transitionend", onTransitionEnd);
> +    container.style.height = `${aHeight}px`;

dive-by nit: this isn't shorter and seems less readable than:

aHeight + "px";

Maybe the JS engine can optimize \`$\{\}\` better? I doubt it though.
Comment on attachment 8859971 [details]
Bug 1354141 - Part 1 - move the panelmultiview JS implementation to a separate module.

https://reviewboard.mozilla.org/r/132014/#review134822

::: browser/components/customizableui/PanelMultiView.jsm:56
(Diff revision 1)
> +
> +  set currentPanel(panel) {
> +    let idx = -1;
> +    // Since this is a sliding window, it's most efficient to start looking for
> +    // the panel to update from the current index, forwards.
> +    for (let i = this.current, l = this.length; i < l; ++i) {

This *might* be a premature optimization and the alternative
```js
this.current = this.indexOf(panel);
```
Might work just as fast. My thought here was that with the knowledge we have is that in the most common case we'd only look ahead one panel. It may actually be faster to pre-empt that using an `if (this[this.current + 1] == panel) { ... }`, but ifs are not necessarily faster/ easier to JIT compile as well.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Comment on attachment 8859971 [details]
> Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows
> for more granular control in behavior and to fork the styles entirely.
> 
> https://reviewboard.mozilla.org/r/132014/#review134822
> 
> ::: browser/components/customizableui/PanelMultiView.jsm:56
> (Diff revision 1)
> > +
> > +  set currentPanel(panel) {
> > +    let idx = -1;
> > +    // Since this is a sliding window, it's most efficient to start looking for
> > +    // the panel to update from the current index, forwards.
> > +    for (let i = this.current, l = this.length; i < l; ++i) {
> 
> This *might* be a premature optimization and the alternative
> ```js
> this.current = this.indexOf(panel);
> ```
> Might work just as fast. My thought here was that with the knowledge we have
> is that in the most common case we'd only look ahead one panel. It may
> actually be faster to pre-empt that using an `if (this[this.current + 1] ==
> panel) { ... }`, but ifs are not necessarily faster/ easier to JIT compile
> as well.

Another drive-by nit: I think that code is referring to views or something like that, rather than <panel> elements? If so, please rename this stuff accordingly.

Regarding your perf optimization, I think its usefulness depends on how many items you're usually dealing with here. If it's a reasonably small number, the above is likely a not so useful micro-optimization and I'd err on the side of making this code neat and brief, and just use indexOf.
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review134856

I'm kind of assuming this is effectively a copy-paste... it would be useful to see if using 'hg cp' gives us a better diff here. Otherwise I'll review this the hard way when push comes to shove, but I would prefer not to if the tools can help us (I guess this also depends a bit on whether you kept the ordering of the binding the same in the JSM...).

::: browser/components/customizableui/PanelMultiView.jsm:1
(Diff revision 1)
> +/* 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/. */

If you `hg cp --after` this from the binding, do we get a more usable diff -w? That'd help with reviewing this changeset...

::: browser/components/customizableui/PanelMultiView.jsm:47
(Diff revision 1)
> +    if (this.__subViews)
> +      return this.__subViews;
> +    return this.__subViews =
> +      this.document.getAnonymousElementByAttribute(this.node, "anonid", "subViews");

This will keep being expensive for the photon binding, because we falsy-check the 'null' that we'll get for this element. We should probably check `this.hasOwnProperty("__subViews")` instead.

Same concern for other getters that are specific to one of the bindings, of course.

Maybe it'd be simpler to just define these as properties in the constructor, which is more or less what the binding does today?

::: browser/components/customizableui/content/panelUI.xml:41
(Diff revision 1)
> -        this._panel.addEventListener("popupshown", this);
> -        this._panel.addEventListener("popuphidden", this);
> -        this._subViews.addEventListener("overflow", this);
> -        this._mainViewContainer.addEventListener("overflow", this);
> -
> -        // Get a MutationObserver ready to react to subview size changes. We
> +        // Proxy these public properties and methods, as used elsewhere by various parts of
> +        // the browser, to the JS instance.
> +        ["_mainView", "ignoreMutations", "showingSubView"].forEach(property => {
> +          this.__defineGetter__(property, () => this.instance[property]);
> +          this.__defineSetter__(property, val => this.instance[property] = val);
> +        });

Please use the standards version:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty

I also wonder if this could/should live in the PanelMultiView jsm instead, and if we should have a followup bug to stop accessing these properties directly on the element.
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8859971 [details]
Bug 1354141 - Part 1 - move the panelmultiview JS implementation to a separate module.

https://reviewboard.mozilla.org/r/132014/#review134858

::: browser/components/customizableui/PanelMultiView.jsm:273
(Diff revision 1)
> +  goBack(target) {
> +    this.showSubView(this.panelViews.back().id, target);
> +  }

This looks odd. If this is the only thing that calls back(), maybe we should just factor out whatever back() is doing into here...

::: browser/components/customizableui/PanelMultiView.jsm:288
(Diff revision 1)
> +      if (aNewMainView.parentNode != this._viewStack && this._viewStack.firstChild != aNewMainView) {
> +        this._viewStack.insertBefore(aNewMainView, this._viewStack.firstChild);
> +      }

Uh... What is this doing, and why do we need it?

::: browser/components/customizableui/PanelMultiView.jsm:396
(Diff revision 1)
> +          if (this._dir == "rtl") {
> +            viewNode.style.marginLeft = `-${viewRect.width}px`;

I'm pretty sure this should be using the same rects regardless of RTL/LTR setting, but different margin properties - or you can just use the logical property instead (margin-inline-start, I expect).

::: browser/components/customizableui/PanelMultiView.jsm:399
(Diff revision 1)
> +
> +          //this._viewStack.style.width = previousRect.width + "px";
> +          if (this._dir == "rtl") {
> +            viewNode.style.marginLeft = `-${viewRect.width}px`;
> +          } else {
> +            viewNode.style.marginLeft = `${previousRect.width}px`;

I'm with Dão here and elsewhere. I also believe that straightforward concats are actually still faster today (ie template strings are slow).

::: browser/components/customizableui/PanelMultiView.jsm:403
(Diff revision 1)
> +          } else {
> +            viewNode.style.marginLeft = `${previousRect.width}px`;
> +          }
> +        }
> +        // Take a breather for the next view to render.
> +        yield new window.Promise(resolve => window.addEventListener("MozAfterPaint", resolve, {once: true}));

Why do we need this? This looks hacky. :-)

::: browser/components/customizableui/PanelMultiView.jsm:415
(Diff revision 1)
> +            // this._viewStack.style.removeProperty("width");
> +            viewNode.style.removeProperty("left");
> +            viewNode.style.removeProperty("translateX");

This looks off? I don't think you're setting `left`

::: browser/components/customizableui/PanelMultiView.jsm:425
(Diff revision 1)
> +          this._viewChangeObserver.observe(viewNode, {
> +            attributes: true,
> +            characterData: true,
> +            childList: true,
> +            subtree: true
> +          });

I think if we can, we should stop doing this. I doubt it makes anything faster...

::: browser/components/customizableui/PanelMultiView.jsm:436
(Diff revision 1)
> +          viewNode.addEventListener("transitionend", onTransitionEnd, { once: true });
> +          if (this._dir == "rtl") {
> +            viewNode.style.transform = `translateX(${viewRect.width}px)`;
> +          } else {
> +            viewNode.style.transform = `translateX(-${previousRect.width}px)`;
> +          }

So are you setting both margin and translate? What am I missing? :-)

::: browser/components/customizableui/PanelMultiView.jsm:594
(Diff revision 1)
> -        height = this._heightOfSubview(this._mainView);
> +          height = this._heightOfSubview(this._mainView);
> -      } else {
> +        } else {
> -        height = this._mainView.scrollHeight;
> +          height = this._mainView.scrollHeight;
> -      }
> +        }
> +      } else {
> +        height = this._currentSubView.scrollHeight;

Looks to me like this might want to use heightOfSubview.

::: browser/components/customizableui/PanelMultiView.jsm:638
(Diff revision 1)
> +    if (this._shouldSetPosition()) {
> +      this._panel.adjustArrowPosition();
> +    }
> +
> +    if (!this.ignoreMutations && this.showingSubView) {
> +      let newHeight = this._heightOfView(this._currentSubView, this._viewContainer);

Did you mean heightOfSubview?

::: browser/components/customizableui/content/panelUI.inc.xul:510
(Diff revision 1)
> +                       label="Go to no. 2"
> +                       onclick="document.getBindingParent(this).showSubView('appMenu-second', this)"/>

For testing only, I guess? :-)

::: browser/components/customizableui/content/panelUI.inc.xul:514
(Diff revision 1)
> -  </panelmultiview>
> +    <panelview id="appMenu-second">
> +      <vbox class="panel-subview-body">
> +        <toolbarbutton id="appMenu-goback"
> +                       class="subviewbutton subviewbutton-iconic"
> +                       label="Go Back"
> +                       onclick="document.getBindingParent(this).showSubView('appMenu-mainView', this)"/>

As discussed on IRC, we should probably make the panelview binding have this button and the header, and hide them based on whether the header attribute (TBD) is or isn't set on the panelview.

Though, I'm a bit confused because I thought we wanted to switch to that style for all the subviews... in particular, what will we end up doing for the overflow panel? Should I just file a separate bug to 'fix' styling / a back button there until we replace it with this stuff, or do you think we'll get this in for 55?
Attachment #8859971 - Flags: review?(gijskruitbosch+bugs)
This looks like it's generally on the right track! Comments in the review comments; I'll have a "proper" look once this is no longer WIP. :-)

(In reply to :Gijs from comment #8)
> ::: browser/components/customizableui/content/panelUI.inc.xul:514
> (Diff revision 1)
> > -  </panelmultiview>
> > +    <panelview id="appMenu-second">
> > +      <vbox class="panel-subview-body">
> > +        <toolbarbutton id="appMenu-goback"
> > +                       class="subviewbutton subviewbutton-iconic"
> > +                       label="Go Back"
> > +                       onclick="document.getBindingParent(this).showSubView('appMenu-mainView', this)"/>
> 
> As discussed on IRC, we should probably make the panelview binding have this
> button and the header, and hide them based on whether the header attribute
> (TBD) is or isn't set on the panelview.
> 
> Though, I'm a bit confused because I thought we wanted to switch to that
> style for all the subviews... in particular, what will we end up doing for
> the overflow panel? Should I just file a separate bug to 'fix' styling / a
> back button there until we replace it with this stuff, or do you think we'll
> get this in for 55?


Ni for this.
Flags: needinfo?(mdeboer)
Gijs, please consider these two patches as feedback requests as well - the animation will be less choppy once we get rid of the panel padding everywhere.
Flags: needinfo?(mdeboer)
Comment on attachment 8859971 [details]
Bug 1354141 - Part 1 - move the panelmultiview JS implementation to a separate module.

https://reviewboard.mozilla.org/r/132014/#review135290

Pretty high-level comments here, hope this is helpful.

::: browser/components/customizableui/PanelMultiView.jsm:39
(Diff revisions 1 - 2)
>    set current(index) {
> +    if (index == this._marker) {

So, I'm sure this is me, but I find this very confusing. AIUI, the `current` setter gets called with an index, ie `slidingPanelView.current = 5` or whatever, but after that, `slidingPanelView.current` might not be 5 but 3 or 8? That's... confusing. I wouldn't know what would happen if someone did something like:

`let foo = slidingPanelView.current = 5;`

Would foo be 5 or the return value of the `current` getter after executing the `current` setter?

Do consumers outside this class actually need to know about indices? Can we just abstract it into only "switch to panelview X" and "switch to the previous panelview" ? That seems like it would be less complex - we could just keep a stack in here instead of a linked list. It should be possible to push things onto the stack, and pop things off it, but not to arbitrarily juggle the stack around...

::: browser/components/customizableui/PanelMultiView.jsm:68
(Diff revisions 1 - 2)
>    get previous() {
>      return this._marker > 0 ? this._marker - 1 : this._marker;
>    }
>  
> -  back() {
> -    return this[this.current = this.previous];
> +  get previousView() {
> +    return this[this.previous];
>    }

I would expect this to return -1 and null, respectively, if there was only 1 panelview on the stack (ie nowhere to go back to).

::: browser/components/customizableui/PanelMultiView.jsm:81
(Diff revisions 1 - 2)
> +  toJSON() {
> +    return `[ ${this.map((view, idx) => idx == this._marker ? `<${view.id}>` : view.id).join(", ")} ]`;
> +  }

What do we need this for?

::: browser/components/customizableui/PanelMultiView.jsm:434
(Diff revisions 1 - 2)
> +          nodeToAnimate.style.width = (aReverse ? viewRect : previousRect).width + "px";
> +
> +          let rectToAnimate = (aReverse ? previousRect : viewRect);
> +          let moveToLeft = (this._dir == "rtl" && !aReverse) || (this._dir == "ltr" && aReverse);
> +          let moveX = (moveToLeft ? "" : "-") + rectToAnimate.width;
> +          nodeToAnimate.style.transform = "translateX(" + moveX + "px)";

There seems to be a 'jump' at the end of the transition for me when the second view comes in. Is there missing styling / items there?

::: browser/components/customizableui/content/panelUI.inc.xul:523
(Diff revisions 1 - 2)
>      </panelview>
> -    <panelview id="appMenu-second">
> +    <panelview id="appMenu-second" title="Second View" class="cui-widget-panelview">
>        <vbox class="panel-subview-body">
>          <toolbarbutton id="appMenu-goback"
>                         class="subviewbutton subviewbutton-iconic"
> -                       label="Go Back"
> +                       label="Placeholder button"/>

Something's going wrong with the subview height calculations because this isn't visible for me - the panel-subview-body element gets 0 height, presumably because it or its parent has overflow set and we resize the panel such that there's no room for it to be any bigger.

::: browser/components/customizableui/content/panelUI.xml:65
(Diff revisions 1 - 2)
> +    <content>
> +      <xul:box class="panel-title">
> +        <xul:toolbarbutton id="appMenu-goback"
> +                           class="subviewbutton subviewbutton-iconic subviewbutton-back"
> +                           label="&backCmd.label;"
> +                           onclick="document.getBindingParent(this).panelMultiView.goBack()"/>
> +        <xul:label xbl:inherits="value=title"/>

The more I think about this, the more I think we should do this bit as part of bug 1353360, and try and get that done (for the existing panels) first. Then we can also pretty easily add an item with a subview to the main photon hamburger panel, which will make it easier to test the stuff in this bug, and would potentially simplify some of the animation/transition work in here (for the multi-nested panels), too. Would that work?
Attachment #8859971 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review135294

Neat! I found 1 issue, but otherwise this looks pretty good to me.

::: browser/components/customizableui/PanelMultiView.jsm:183
(Diff revision 2)
> -        <parameter name="aAnchor"/>
> -        <body><![CDATA[
> +    window.Task.spawn(function*() {
> +      let viewNode = this.node.querySelector("#" + aViewId);
> -          Task.spawn(function*() {
> -            let viewNode = this.querySelector("#" + aViewId);
> -            if (!viewNode) {
> +      if (!viewNode) {
> -              viewNode = document.getElementById(aViewId);
> +        viewNode = document.getElementById(aViewId);

I don't know why eslint doesn't pick this up, but `document` is not defined here, and this breaks in my newly added subviews test for the overflow panel.
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8859971 - Attachment is obsolete: true
Attachment #8859971 - Flags: review?(gijskruitbosch+bugs)
Attachment #8861487 - Attachment is obsolete: true
Attachment #8861487 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8861509 [details]
Bug 1354141 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/133482/#review136876

OK, I'm struggling to get to actual reviews today, but let's just at least fix the flags... I already r+'d this, this looks fine.
Attachment #8861509 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review136878

... whereas I still need to look at this.
Attachment #8859970 - Flags: review+ → review?(gijskruitbosch+bugs)
Just learned about this bug, I'll have to update the patch in bug 1009116 once this lands. The other bug is currently dependent on bug 1293242 as well.
Blocks: 1009116
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review136880

I haven't had time to take this for a spin, but I think there's enough feedback here that it's probably better to hand it back to you now rather than sit on the review until later tomorrow. Thanks!

::: browser/components/customizableui/PanelMultiView.jsm:39
(Diff revision 4)
> +  set current(index) {
> +    if (index == this._marker) {
> +      // Never change a winning team.
> +      return;
> +    }

You don't seem to have changed anything here after my previous feedback about the index changing here. Can you elaborate on why we need to use indices as we do here, as an externally-exposed API?

::: browser/components/customizableui/PanelMultiView.jsm:165
(Diff revision 4)
> +  get panelViews() {
> +    if (this._panelViews)
> +      return this._panelViews;
> +
> +    this._panelViews = new SlidingPanelViews();
> +    this._panelViews.push(...Array.from(this.node.getElementsByTagName("panelview")));

You don't need the Array.from here, as far as I can tell, just the spread operator works.

::: browser/components/customizableui/PanelMultiView.jsm:172
(Diff revision 4)
> +  get _dir() {
> +    return this.window.getComputedStyle(this.node).direction;
> +  }

This flushes layout. Can we use one of the UI locale APIs instead? The actual direction of the panel won't change at runtime, I would have thought, so another option would be to cache it.

::: browser/components/customizableui/PanelMultiView.jsm:179
(Diff revision 4)
> +  }
> +  get _currentSubView() {
> +    return this._panelViews ? this._panelViews.currentView : this.__currentSubView;
> +  }
> +  set _currentSubView(panel) {
> +    if (this._panelViews)

Here and in the getter we check the underscored property, which only gets set if someone accesses the non-underscored property. Is that deliberate? It feels a bit fragile...

::: browser/components/customizableui/PanelMultiView.jsm:272
(Diff revision 4)
> +      this._subViews = this._viewStack = null;
> +  }
>  
> -    this.node = this.__clickCapturer = this.__viewContainer = this.__mainViewContainer =
> -      this.__subViews = this.__viewStack = null;
> +  goBack(target) {
> +    let [current, previous] = this.panelViews.back();
> +    this.showSubView(current, target, previous);

The third param here is a boolean, but you're passing an object, and this is coming from new API stuff you're adding (.back()) for which this is the only consumer. Can we tidy that up? It looks to me like right now, `previous` is always non-null (it will either be the previous item or the current one, and the current one clearly exists...), and so really we're just always passing `true` here, for the third param.

::: browser/components/customizableui/PanelMultiView.jsm:317
(Diff revision 4)
> +    } else {
> +      this.showSubView(this._mainViewId);
> +    }
>    }
>  
> -  showSubView(aViewId, aAnchor) {
> +  showSubView(aViewId, aAnchor, aReverse = false) {

Can we get a followup to convert this code to async/await?

::: browser/components/customizableui/PanelMultiView.jsm:428
(Diff revision 4)
> +          // Wait until after the first paint to ensure setting 'current=true'
> +          // has taken full effect; we want to correctly measure rects using
> +          // `dwu.getBoundsWithoutFlushing`.
> +          window.addEventListener("MozAfterPaint", () => {
> +            let viewRect = dwu.getBoundsWithoutFlushing(viewNode);

Can this race with the transitionend handler? We should probably catch that.

::: browser/components/customizableui/PanelMultiView.jsm:428
(Diff revision 4)
> +          // Wait until after the first paint to ensure setting 'current=true'
> +          // has taken full effect; we want to correctly measure rects using
> +          // `dwu.getBoundsWithoutFlushing`.

Under what circumstances does the current=true thing change the bounds of the subview? Can we be more explicit in this comment?

::: browser/components/customizableui/PanelMultiView.jsm:433
(Diff revision 4)
> +            // Due to the views being inside a stack, the next view is stretched
> +            // to be be same size as the currently visible view, except when it's
> +            // larger. In other words: smaller views are reported to be a different

IIRC if there is a stack you can use the equalsize attribute, set it to never, and then items shouldn't all end up being the same size. Does that help here?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/equalsize

Of course, if we then use flex everywhere that might still change things...

::: browser/components/customizableui/PanelMultiView.jsm:437
(Diff revision 4)
> +            let viewRect = dwu.getBoundsWithoutFlushing(viewNode);
> +            // Due to the views being inside a stack, the next view is stretched
> +            // to be be same size as the currently visible view, except when it's
> +            // larger. In other words: smaller views are reported to be a different
> +            // size, regardless whether we flush layout.
> +            // We can at least make sure that this only happens to use once and

"happens to us", I think?

::: browser/components/customizableui/PanelMultiView.jsm:460
(Diff revision 4)
> +            // Set the transition style and listen for its end to clean up and
> +            // make sure the box sizing becomes dynamic again.
> +            nodeToAnimate.style.transition = "transform ease-" + (aReverse ? "in" : "out") +
> +              " var(--panelui-subview-transition-duration)";

Can/should we do any of this before the initial paint event?

::: browser/components/customizableui/PanelMultiView.jsm:485
(Diff revision 4)
> +                if (!aReverse)
> +                  viewNode.style.removeProperty("margin-inline-start");

This looks duplicated with 6 lines up?

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

The addition of `width` here also affects non-photon panels, right? Is that intentional?

I think this has other effects that are a bit problematic, in particular, do we already check what property finishes transitioning when using transitionend events in the binding / .jsm ?

::: browser/components/customizableui/content/panelUI.inc.xul:542
(Diff revision 4)
> +        <toolbarbutton id="appMenu-subiew-button"
> +                       class="subviewbutton subviewbutton-nav"
> +                       label="Go to no. 2"
> +                       onclick="document.getBindingParent(this).showSubView('appMenu-second', this)"/>

We should probably not include this (and the other test views) when we land).

::: browser/components/customizableui/content/panelUI.js:444
(Diff revision 4)
>      if (!aAnchor) {
>        Cu.reportError("Expected an anchor when opening subview with id: " + aViewId);
>        return;
>      }
>  
> -    let container = aAnchor.closest("panelmultiview");
> +    let container = aAnchor.closest("panelmultiview") || aAnchor.closest("photonpanelmultiview");

This can be one selector, right?

```js
let container = aAnchor.closest("panelmultiview,photonpanelmultiview");
```
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs)
Iteration: 55.4 - May 1 → 55.5 - May 15
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review136880

> Can we get a followup to convert this code to async/await?

Of course. Will do upon landing this work.

> Can this race with the transitionend handler? We should probably catch that.

The transitionend listener is set inside this MozAfterPaint handler. I don't think it can race.

> IIRC if there is a stack you can use the equalsize attribute, set it to never, and then items shouldn't all end up being the same size. Does that help here?
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/equalsize
> 
> Of course, if we then use flex everywhere that might still change things...

This didn't work, unfortunately :(

> We should probably not include this (and the other test views) when we land).

Indeed. I will remove them upon landing.
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review138486
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

Too early!
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs)
Attached video Panel recording — 
I see artifacts when switching between views. The height 'jumps' at the end of the transition, and sometimes (when switching from the initial panel to the first subview) I see the menu contents outside of the main menu, somehow...
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review138568

Clearing review for now given the behavioural hiccups I'm still seeing.
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review139566

When switching views ("forward"), I still see the previous view appear momentarily to the left of the new view. I also see a (transitioned) extra height change after the new view is selected. :-(

Maybe we just *need* an actual sync reflow to do some of the measurements to make this work correctly? At least for a first pass? We can always iterate later.

::: browser/components/customizableui/PanelMultiView.jsm:478
(Diff revisions 4 - 7)
> -              // We set the margin here to make sure the view is positioned next
> +          // We set the margin here to make sure the view is positioned next
> -              // to the view that is currently visible. The animation is taken
> +          // to the view that is currently visible. The animation is taken
> -              // care of by transitioning the `transform: translateX()` property
> +          // care of by transitioning the `transform: translateX()` property
> -              // instead.
> +          // instead.
> -              // Once the transition finished, we clean both properties up.
> +          // Once the transition finished, we clean both properties up.
> -              viewNode.style.marginInlineStart = `${previousRect.width}px`;
> +          viewNode.style.marginInlineStart = `${previousRect.width}px`;

It would be clearer to use nodeToAnimate here (which is guaranteed to be == viewNode because !reverse).

::: browser/components/customizableui/PanelMultiView.jsm:509
(Diff revisions 4 - 7)
> -            // Set the viewContainer dimensions to make sure only the current view
> +          // Set the viewContainer dimensions to make sure only the current view
> -            // is visible.
> +          // is visible.
> -            this._viewContainer.style.height = viewRect.height + "px";
> +          this._viewContainer.style.height = viewRect.height + "px";
> -            this._viewContainer.style.width = viewRect.width + "px";
> +          this._viewContainer.style.width = viewRect.width + "px";

Effectively, doing this after the MozAfterPaint seems to mean that the two views get displayed next to each other (horizontally) for a short space of time. Can we avoid that?

::: browser/components/customizableui/PanelMultiView.jsm:511
(Diff revisions 4 - 7)
> +            }
> +          }
> +
> -            // Set the viewContainer dimensions to make sure only the current view
> +          // Set the viewContainer dimensions to make sure only the current view
> -            // is visible.
> +          // is visible.
> -            this._viewContainer.style.height = viewRect.height + "px";
> +          this._viewContainer.style.height = viewRect.height + "px";

I suspect that doing this after the MozAfterPaint means that the two views get displayed next to each other (horizontally) for a short space of time. Can we avoid that?

::: browser/components/customizableui/PanelMultiView.jsm:518
(Diff revisions 4 - 7)
> +              this._viewContainer.style.removeProperty("height");
> +              this._viewContainer.style.removeProperty("width");

This feels really hacky. I also see a kind of "extra jump" transition after the animation ends where the height of the subview changes. Could that be a result of doing this off a timeout?
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs)
> I suspect that doing this after the MozAfterPaint means that the two views get displayed next to each other (horizontally) for a short space of time. Can we avoid that?

This comment got duplicated. Mozreview doesn't seem to do well with intermittent connectivity. :-\
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review139816

This is much better. I still have 1 more issue (besides the item below). http://imgur.com/a/DC00q - at the end of the animation when going from the 2nd to the 3rd subview, I see the header and footer 'jump'. I expect their containers are somehow being resized to a much wider thing and then back, or something. My understanding is that we're fixing the width of these panelviews so they're always the same width. This is also how the current panelmultivews work. Is that not the case?

When I tried to investigate this, and changed the main transition length (in content/browser.css) to be much longer, the 'jump' I saw in my earlier review reappeared (but now without a transition) - so after the transition to the second view is complete, the height of the panel changes to have whitespace at the bottom, and then changes back after a second or so. It feels like we're fighting with something that we shouldn't need to be fighting (like XUL panel/stack layout, maybe?) which doesn't like the position: relative on the stack, or at least expects the stack to be resized, too. Could we just set a height/width on that at the end of the transition when we clear the height/width off the viewcontainer, and then remove those height/width before starting the next transition?

I also experimented with -moz-stack-sizing: ignore, which seems to at least help with the aforementioned 'jump', and was always set on the panel-subviews but somehow not on the stack you're using here (intentional?), but doesn't seem to solve the first width-related issue. Unless you have a flash of insight, let's work this out in Toronto, I guess? :-(

::: browser/components/customizableui/PanelMultiView.jsm:509
(Diff revisions 7 - 9)
>  
>            // Set the viewContainer dimensions to make sure only the current view
>            // is visible.
>            this._viewContainer.style.height = viewRect.height + "px";
>            this._viewContainer.style.width = viewRect.width + "px";
>            this._viewContainer.addEventListener("transitionend", () => {

There's a transitionend listener here and another one a few lines down. Transitions bubble, so either:

1) make this one listener
2) if there are multiple transitions we're waiting for, they need to check what the original target and the transitioned properties are.

::: browser/components/customizableui/PanelMultiView.jsm:517
(Diff revisions 7 - 9)
>              // to be sure we don't flicker annoyingly.
> +            // NB: HACK!
>              window.setTimeout(() => {
>                this._viewContainer.style.removeProperty("height");
>                this._viewContainer.style.removeProperty("width");
> -            }, this._transitionDuration);
> +            }, 500);

Can we file a followup for this, if we really need this? :-(

In my testing, waiting for MozAfterPaint instead (again) works here, instead of the arbitrary timeout...
Attachment #8859970 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #35)
> Maybe we just *need* an actual sync reflow to do some of the measurements to
> make this work correctly? At least for a first pass? We can always iterate
> later.

This is what I found while working on bug 1009116. We need one synchronous layout so we can calculate the actual final height of the views without painting anything on screen yet:

https://reviewboard.mozilla.org/r/136888/diff/1#6.38

We should probably join forces here, as I've made progress on that bug in the past three days. The generic 50-line function linked above replaces something like 300 lines and is compatible with any layout, so you can fork the styles but won't need to fork the JavaScript logic, likely saving another 100 lines of code.

You can try the build from this changeset, make sure to rebuild the C++ binaries, as Neil's layout fixes are included:

hg pull -r d5ae125068bc89d6f56c4db9994160dcf8a59c50 https://reviewboard-hg.mozilla.org/gecko

The other two views still have issues, but the main view should be 90% working. Most of the bugs mentioned here, like race conditions and jumps if a transition is stopped in the middle, should be fixed. Also the footer of the main view is always aligned to the bottom, including while the view is closing.

The patch includes the timing change from 150ms to 1s for easier testing.
(In reply to :Paolo Amadini from comment #40)
> The other two views still have issues, but the main view

To clarify, I'm referring to the Identity Block and the Downloads Panel versus the Application Menu.
Comment on attachment 8859970 [details]
Bug 1354141 - Part 2 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/132012/#review139816

> There's a transitionend listener here and another one a few lines down. Transitions bubble, so either:
> 
> 1) make this one listener
> 2) if there are multiple transitions we're waiting for, they need to check what the original target and the transitioned properties are.

In my testing, it doesn't seem to be bubbling at all...?

> Can we file a followup for this, if we really need this? :-(
> 
> In my testing, waiting for MozAfterPaint instead (again) works here, instead of the arbitrary timeout...

I used MozAfterPaint as well, but that didn't work in few edge cases when switching between panels really quickly and often. I don't remember how I triggered it exactly, but I simply settled on a longer timeout, since it's merely cleanup.
Depends on: 1363178
Attachment #8859970 - Attachment is obsolete: true
Attachment #8861509 - Attachment is obsolete: true
Comment on attachment 8865966 [details]
Bug 1354141 - Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely.

https://reviewboard.mozilla.org/r/137564/#review140780

So, I think this can land given it's all behind a pref, with the following proviso, besides the nits below:

- (obviously) please remove the test markup
- please file followups for:
-- the width setting of the panel and its kiddos
-- figuring out the setTimeout... hopefully Neil can help with that.

Please make sure there are comments in the code that reference the bug numbers for the followups.

Thanks so much for all the crazy work this patch was! :-)

::: browser/components/customizableui/PanelMultiView.jsm:136
(Diff revision 1)
> +   * currently selected view is denoted by '<' and '>'.
> +   *
> +   * @return {String}
> +   */
> +  toJSON() {
> +    return `[ ${this.map((view, idx) => idx == this._marker ? `<${view.id}>` : view.id).join(", ")} ]`;

Let's remove this until we need it.

::: browser/components/customizableui/PanelMultiView.jsm:266
(Diff revision 1)
> +      this._subViews.addEventListener("overflow", this);
> +      this._mainViewContainer.addEventListener("overflow", this);
> -    this._subViews.addEventListener("overflow", this);
> +      this._subViews.addEventListener("overflow", this);
> -    this._mainViewContainer.addEventListener("overflow", this);
> +      this._mainViewContainer.addEventListener("overflow", this);

Probably only need 1 of each of these.

::: browser/components/customizableui/PanelMultiView.jsm:417
(Diff revision 1)
>          return;
>        }
>  
> +      let reverse = !!aPreviousView;
> +      let previousViewNode = aPreviousView || this._currentSubView;
>        this._currentSubView = viewNode;

In the photon case, we may need to reorder nodes? It's not really clear because the test case in this commit has the panels in the 'right' order anyway, so we can't tell.

We should probably fix one of the things that actually needs this (the "more" thing in the hamburger panel probably being the easiest) to see how this works in practice and make any necessary adjustments.
Attachment #8865966 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1363753
Blocks: 1363756
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e704f6d9476
Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely. r=Gijs
Backed out for failing mochitest browser_ext_browserAction_popup_resize.js:

https://hg.mozilla.org/integration/autoland/rev/3b02ca42da3aeff49828a6486a0370fcf8ec928d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3e704f6d9476c810d336f132ec8a5bf6211521cf&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=98050429&repo=autoland

[task 2017-05-10T16:30:39.974847Z] 16:30:39     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window width should not change - 
[task 2017-05-10T16:30:39.977345Z] 16:30:39     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window height should increase (448 >= 174) - 
[task 2017-05-10T16:30:39.981878Z] 16:30:39     INFO - Buffered messages finished
[task 2017-05-10T16:30:39.984524Z] 16:30:39     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Document should not be vertically scrollable - Got 10, expected 0
[task 2017-05-10T16:30:39.987778Z] 16:30:39     INFO - Stack trace:
[task 2017-05-10T16:30:39.989635Z] 16:30:39     INFO -     chrome://mochikit/content/browser-test.js:test_is:928
[task 2017-05-10T16:30:39.992059Z] 16:30:39     INFO -     chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:testPopupSize:213
[task 2017-05-10T16:30:39.995362Z] 16:30:39     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-05-10T16:30:39.997911Z] 16:30:39     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-05-10T16:30:40.000266Z] 16:30:39     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-05-10T16:30:40.006026Z] 16:30:40     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-05-10T16:30:40.008382Z] 16:30:40     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-05-10T16:30:40.010521Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-05-10T16:30:40.012748Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.015416Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.017426Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.021601Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.023566Z] 16:30:40     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-05-10T16:30:40.025654Z] 16:30:40     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-05-10T16:30:40.029629Z] 16:30:40     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-05-10T16:30:40.031523Z] 16:30:40     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-05-10T16:30:40.033353Z] 16:30:40     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-05-10T16:30:40.037581Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-05-10T16:30:40.040779Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.042554Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.044402Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.046318Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.048203Z] 16:30:40     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-05-10T16:30:40.050220Z] 16:30:40     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-05-10T16:30:40.052355Z] 16:30:40     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-05-10T16:30:40.054420Z] 16:30:40     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-05-10T16:30:40.059915Z] 16:30:40     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-05-10T16:30:40.061772Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-05-10T16:30:40.063678Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.065394Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.067414Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.069151Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.070998Z] 16:30:40     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-05-10T16:30:40.072892Z] 16:30:40     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-05-10T16:30:40.075179Z] 16:30:40     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-05-10T16:30:40.077391Z] 16:30:40     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-05-10T16:30:40.081908Z] 16:30:40     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-05-10T16:30:40.083733Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-05-10T16:30:40.085547Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.087458Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.089385Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.091188Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-05-10T16:30:40.094319Z] 16:30:40     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-05-10T16:30:40.096184Z] 16:30:40     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-05-10T16:30:40.098140Z] 16:30:40     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-05-10T16:30:40.101675Z] 16:30:40     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-05-10T16:30:40.103636Z] 16:30:40     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-05-10T16:30:40.105423Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-05-10T16:30:40.107533Z] 16:30:40     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-05-10T16:30:40.109426Z] 16:30:40     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
Flags: needinfo?(mdeboer)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9919aa12372d
Introduce a new binding for Photon panels that allows for more granular control in behavior and to fork the styles entirely. r=Gijs
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/9919aa12372d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1364738
Depends on: 1369407
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: