Closed Bug 1401991 Opened 7 years ago Closed 7 years ago

The "Library" toolbarbutton opens an empty popup if the toolbarbutton was clicked with the "Library" submenu opened in Firefox's main menu.

Categories

(Firefox :: Menus, defect, P1)

Product:
Firefox
Component:
Menus
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + verified
firefox58 + verified

People

(Reporter: johan.charlez, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

Firefox-Toolbarbutton-Library-empty-popup.png
4.25 KB, image/png
Details
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.
59 bytes, text/x-review-board-request
Gijs
: review+
Sylvestre
: approval-mozilla-beta+
Details 
1. Put the "Library" toolbarbutton on the main toolbar or the tab-bar.
2. Open the main menu (hamburger button).
3. Open the "Library" submenu.
4. Click the "Library" toolbarbutton on the main toolbar/tab-bar.

Actual result:
1. The main menu closes.
2. The toolbarbutton's popup opens, shows its contents ("Bookmarks", "History", "Downloads", "Synced Tabs" etc.) for a split second, and then changes to an empty popup.

Expected result:
1. The main menu closes.
2. The toolbarbutton's menu opens with its contents  ("Bookmarks", "History", "Downloads", "Synced Tabs" etc.).

Tested and reproduced on Beta Dev Edition 57 and Nightly 57
Blocks: photon-visual
[Tracking Requested - why for this release]:
Empty popups aren't very shippable.
Blocks: photon-structure, 1374749
No longer blocks: photon-visual
tracking-firefox57: --- → ?
Whiteboard: [photon-structure][triage]
Depends on: 1401383
This should be fixed in the _next_ Nightly.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
I guess you mean this is fixed by 1401383, updating status.
status-firefox58: affected → fixed
Target Milestone: --- → Firefox 58
(In reply to Julien Cristau [:jcristau] from comment #3)
> I guess you mean this is fixed by 1401383, updating status.

Indeed, thanks Julien.
This is still NOT fixed in the Nightly 20170924100550.
It's even worse than initial STR. You don't even need to go to the Hamburger submenu, just open Hamburger menu and then Library on the toolbar. For a split second it shows a content and then changes to an empty popup.
Should be filed another bug or can be reopened this ?
Flags: needinfo?(mdeboer)
I can still reproduce the problem with the STR on 58.0a1 windows10.

Built from https://hg.mozilla.org/mozilla-central/rev/ff40c5dcaa41261b39a4e9795e02a9d51dd30ced
Build ID 	20170924100550
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Status: RESOLVED → REOPENED
status-firefox58: fixed → affected
tracking-firefox58: --- → ?
Resolution: FIXED → ---
Status: REOPENED → NEW
Priority: P1 → P3
This is indeed a different problem and has to do with the reparenting of the <panelview> node. Will take a look soon.
Flags: needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: P3 → P1
Flags: qe-verify? → qe-verify+
Target Milestone: Firefox 58 → Firefox 57
QA Contact: gwimberly
It didn't strike me to test the other "library toolbarbuttons" (history, bookmarks, downloads, synced tabs).
Strangely I can only reproduce the bug with the history toolbarbutton, and the synced tabs toolbarbutton, but not not with the bookmarks and downloads toolbarbuttons.

"History"-toolbarbutton STR:
1. Click 'Hamburger->Library->History'.
2. Click the "History"-toolbarbutton.
Result: empty popup.

"Bookmarks"-toolbarbutton STR:
1. Click 'Hamburger->Library->Bookmarks'.
2. Click the "Bookmarks"-toolbarbutton.
Result: Works as it should.

"Synced Tabs"-toolbarbutton STR:
1. Click 'Hamburger->Library->Synced Tabs'.
2. Click the "Synced Tabs"- toolbarbutton.
Result: empty popup.

"Downloads"-toolbarbutton STR:
0. I made sure the downloads button was visible on the toolbar by downloading a file.
1. Click 'Hamburger->Library->Downloads'.
2. Click the "Downloads"-toolbarbutton.
Result: Works as it should.

Does your patch fix this as well Mike, or would you like me to file another bug?
Flags: needinfo?(mdeboer)
Thanks, Johan, for thoroughly looking at this!

I just confirmed that my patch here fixes the cases you mentioned as well. (pfew!)
Flags: needinfo?(mdeboer)
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

https://reviewboard.mozilla.org/r/183224/#review188406

::: browser/components/customizableui/PanelMultiView.jsm
(Diff revision 1)
> -        if (viewNode.id == this._mainViewId) {
> -          this.node.setAttribute("viewtype", "main");
> -        } else {
> -          this.node.setAttribute("viewtype", "subview");

I'm concerned about this moving until after the `await this._cleanupTransitionPhase()` code, and thus causing issues with flickering / flash of not quite right-styled content. Is that possible? (haven't had a chance to test it yet)
No, the attributes are mainly/ only used for styling purposes in the non-photon style panels. I could add them back where they were without issue to be absolutely sure.
(In reply to Mike de Boer [:mikedeboer] from comment #13)
> No, the attributes are mainly/ only used for styling purposes in the
> non-photon style panels. I could add them back where they were without issue
> to be absolutely sure.

OK, no, I think we're OK - I was confused with https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#93-101 - will review the rest after my 1:1.
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Thanks, Johan, for thoroughly looking at this!
> 
> I just confirmed that my patch here fixes the cases you mentioned as well.
> (pfew!)

Sweet, thanks for confirming! :)
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

https://reviewboard.mozilla.org/r/183224/#review188462

r=me because this fixes things. I can see at least 2 more issues that could still be racy. Maybe we can/should fix them in another bug, or here if you prefer?

(and one simple nit about `else` vs. `else if`)

::: browser/components/customizableui/PanelMultiView.jsm:544
(Diff revision 2)
> -      if (this.panelViews && playTransition) {
> -        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, aAnchor);
> -
>          this._dispatchViewEvent(viewNode, "ViewShown");
>          this._updateKeyboardFocus(viewNode);
>        } else if (!this.panelViews) {

This can just be `else` now.

::: browser/components/customizableui/PanelMultiView.jsm:589
(Diff revision 2)
>      if (this._autoResizeWorkaroundTimer)
>        window.clearTimeout(this._autoResizeWorkaroundTimer);

Should we do this in the non-transition case, too?

AIUI, right now the following sequence is possible:

1. open, transition to subview
2. close popup before timeout fires. We never hit the clearTimeout because we don't transition when re-showing the main view
3. reopen the popup. We remove width/height attributes too early.

Is that plausible? Is there a better place to do this?

::: browser/components/customizableui/PanelMultiView.jsm:753
(Diff revision 2)
>        previousViewNode.style.display = "none";
>        await BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => {});
>        previousViewNode.style.removeProperty("display");

I'm nervous about this in relation to the reparenting, too. Right now, I think the following sequence is (in theory) possible:

1. open panel
2. transition to library subview
3. attempt to show library in its own popup while the  layout promise here hasn't resolved yet
4. end up with a display: none reparented node.

Is there something we can do better here?
Attachment #8911795 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #16)
> Should we do this in the non-transition case, too?
> 
> AIUI, right now the following sequence is possible:
> 
> 1. open, transition to subview
> 2. close popup before timeout fires. We never hit the clearTimeout because
> we don't transition when re-showing the main view
> 3. reopen the popup. We remove width/height attributes too early.
> 
> Is that plausible? Is there a better place to do this?

I don't think so; this place makes sense, because we're setting the width/ height on the viewContainer again below - so the code that runs after the timeout would interfere. If we `clearTimeout()` earlier, we run the risk of not removing the fixed width/ height which would look odd, especially when other views are shown.
I think it's safe to keep this workaround as-is, until the auto-sizing problem is fixed.

> I'm nervous about this in relation to the reparenting, too. Right now, I
> think the following sequence is (in theory) possible:
> 
> 1. open panel
> 2. transition to library subview
> 3. attempt to show library in its own popup while the  layout promise here
> hasn't resolved yet
> 4. end up with a display: none reparented node.
> 
> Is there something we can do better here?

Well, since we're _expecting_ a layout flush, regardless of the user action, the promise will always resolve. Apart from that, we're waiting for a layout flush for the _entire_ document, so even if my previous statement were to be invalid, showing the library in another popup and the reparenting will cause a layout flush anyways, causing the `display: none` to be reset.
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

I had to do a whole bunch of things to make the tests happy, but I'm glad that we have these integration tests to start with.
So, I had to:

 * Harden the new `hideAllViewsButOne()` to not do erroneous things if called when the binding is already gone.
 * Generalize things into `hideAllViewsButOne()` some more by clearing `_viewShowing` in there and do the descriptionHeightWorkaround thing in there too,
 * Then I had to make sure not to over-eagerly dispatch 'ViewShowing' events, because that confuses some,
 * Keep a local copy of `_transitionDetails` to ensure it's still there after transition
 * Harden `_cleanupTransitionPhase()` to only clear the phase that belongs to a specific transition, _if_ that's passed in as an argument. This resolves any potential raciness that might occur when `showSubView()` is called again mid-transition.
Attachment #8911795 - Flags: review+ → review?(gijskruitbosch+bugs)
Depends on: 1402176
Blocks: 1402384
Blocks: 1402845
Attachment #8911795 - Flags: review?(gijskruitbosch+bugs)
Still working on the failing 'browser_987640_charEncoding.js' test, which should be the last one remaining.
It doesn't fail when run standalone, mind you, but only in suite-mode.
I've narrowed it down to the following command:

./mach test browser/components/customizableui/test/browser_981418-widget-onbeforecreated-handler.js browser/components/customizableui/test/browser_987640_charEncoding.js

I'm now looking at what 'browser_981418-widget-onbeforecreated-handler.js' might be doing to interfere with said test.
Attachment #8911795 - Flags: review?(gijskruitbosch+bugs)
Try looks good/ green enough!
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

https://reviewboard.mozilla.org/r/183224/#review189818

I have some nits in terms of naming etc., but on the whole the code looks OK to me. Unfortunately, if I follow these steps on OS X:

1. open hamburger menu
2. open library in hamburger
3. click on library button (hamburger panel closes)
4. click on library button again

the library panel opens but the back button is visible (and nonfunctional).

So I think something is still going wrong.

::: browser/components/customizableui/PanelMultiView.jsm:513
(Diff revision 11)
>  
>        let reverse = !!aPreviousView;
>        let previousViewNode = aPreviousView || this._currentSubView;
> -      let playTransition = (!!previousViewNode && previousViewNode != viewNode &&
> -        this._panel.state == "open");
> +      // If the panelview to show is the same as the previous one, the 'ViewShowing'
> +      // event has already been dispatched. Don't do it twice.
> +      let showingSameView = (viewNode == previousViewNode);

Nit: don't need braces around this.

::: browser/components/customizableui/PanelMultiView.jsm:543
(Diff revision 11)
>  
>        this._viewShowing = viewNode;
>  
>        // Make sure that new panels always have a title set.
>        if (this.panelViews && aAnchor) {
> -        if (!viewNode.hasAttribute("title"))
> +        if (!viewNode.hasAttribute("title") && aAnchor.hasAttribute("label"))

This seems like an orthogonal fix, can you mention in the commit message why this was needed?

::: browser/components/customizableui/PanelMultiView.jsm:585
(Diff revision 11)
> -        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, aAnchor);
> +          await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, aAnchor);
> -
> -        this._dispatchViewEvent(viewNode, "ViewShown");
> -        this._updateKeyboardFocus(viewNode);
> +          this._updateKeyboardFocus(viewNode);
> -      } else if (!this.panelViews) {
> +        } else {
> +          this.hideAllViews(viewNode);

Unfortunately, this is hard to read. I understand that if we renamed the method again, it would also be hard to read this version:
```js
this.hideAllViewsExcept();
```

but perhaps that would still be better in that we could call it as:
```js
this.hideAllViewsExcept(null);
```
which does kind of make sense? More sense than `hideAllViews(viewNode)`, that is.

::: browser/components/customizableui/PanelMultiView.jsm:761
(Diff revision 11)
>      let {phase, previousViewNode, viewNode, reverse, resolve, listener, anchor} = this._transitionDetails;
>      this._transitionDetails = null;
>  
>      // Do the things we _always_ need to do whenever the transition ends or is
>      // interrupted.
> -    this._dispatchViewEvent(previousViewNode, "ViewHiding");
> +    this.hideAllViews(viewNode);

It's also not obvious this does the post-transition stuff like calling descriptionHeightWorkaround. Maybe we can split that out to a separate method so it's more obvious at the callsite? Or have one named method that hides all the views except 1, and a second method that calls the first and then does the other stuff, with a more descriptive name (`finishSwitchingTo(viewNode)` or something).

::: browser/components/customizableui/PanelMultiView.jsm:1077
(Diff revision 11)
> -        if (this._mainView.hasAttribute("blockinboxworkaround")) {
> -          this._mainView.style.removeProperty("height");
> -          this._mainView.removeAttribute("exceeding");
> +        let mainView = this._mainView;
> +        if (mainView && mainView.hasAttribute("blockinboxworkaround")) {
> +          mainView.style.removeProperty("height");
> +          mainView.removeAttribute("exceeding");
>          }

In the `popupshowing` block this makes sense because there's a callback involved, but here I don't see the point of creating the local variable. Can we simply keep this as-is to get patch size down a little to help with uplift?

::: browser/components/customizableui/content/panelUI.js:430
(Diff revision 11)
>        tempPanel.classList.toggle("cui-widget-panelWithFooter",
>                                   viewNode.querySelector(".panel-subview-footer"));
>  
> +      let viewShown = false;
> +      let listener;
> +      viewNode.addEventListener("ViewShown", listener = () => viewShown = true, {once: true});

Nit: might as well define the listener on the previous line and pass it by reference here, instead of having the assignment in this line.

::: browser/components/customizableui/test/browser_remote_tabs_button.js:38
(Diff revision 11)
> -  await viewShown;
> +  let remoteTabsPanel = document.getElementById("PanelUI-remotetabs");
> +  await BrowserTestUtils.waitForEvent(remoteTabsPanel, "ViewShown");

Can you clarify why moving this is OK / fixes anything? Looks like there shouldn't be a behavioural difference.

::: browser/components/customizableui/test/head.js:316
(Diff revision 11)
>  }
>  
>  function subviewShown(aSubview) {
>    return new Promise((resolve, reject) => {
>      let win = aSubview.ownerGlobal;
>      let timeoutId = win.setTimeout(() => {

Followup to kill all this "did not <whatever> within 20 seconds" timeout malarky?

::: browser/components/uitour/UITour.jsm:19
(Diff revision 11)
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +  "resource://gre/modules/BrowserUtils.jsm");

Not quite clear why you added this?
Attachment #8911795 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #30)
> Comment on attachment 8911795 [details]
> Bug 1401991 - Ensure that we don't hide panelviews that are already
> reparented to another multi-view and ensure to hide other panels
> consistently.
> 
> https://reviewboard.mozilla.org/r/183224/#review189818
> 
> I have some nits in terms of naming etc., but on the whole the code looks OK
> to me. Unfortunately, if I follow these steps on OS X:
> 
> 1. open hamburger menu
> 2. open library in hamburger
> 3. click on library button (hamburger panel closes)
> 4. click on library button again
> 
> the library panel opens but the back button is visible (and nonfunctional).
> 
> So I think something is still going wrong.


And in fact, if I follow these steps but insert:

2a. open bookmarks subview for library in the hamburger panel

then on step 3, the library panel does open immediately instead of only closing the hamburger panel first (but it still ends up with a back button). So it feels like there's at least 2 issues, and both of those are regressions compared to the status quo, I believe. :-(
Ah yes, easy fix... coming up!
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

https://reviewboard.mozilla.org/r/183224/#review189818

> It's also not obvious this does the post-transition stuff like calling descriptionHeightWorkaround. Maybe we can split that out to a separate method so it's more obvious at the callsite? Or have one named method that hides all the views except 1, and a second method that calls the first and then does the other stuff, with a more descriptive name (`finishSwitchingTo(viewNode)` or something).

I actually think it makes perfect sense, because this way unecessary reflows are prevented at all cost. Having this processing in one place helps the thought process and perhaps with the `hideAllViewExcept` change it suits you better?

> In the `popupshowing` block this makes sense because there's a callback involved, but here I don't see the point of creating the local variable. Can we simply keep this as-is to get patch size down a little to help with uplift?

the `this._mainView` getter is expensive as in it does a `document.getElementById(this._mainViewId)` for every invocation. I don't think the patch size grows much due to this change, but moreso due to the unit test changes.
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

https://reviewboard.mozilla.org/r/183224/#review189960

This fixes the 'main view' appearance (so no more back button + title), but not the other issue I flagged up. More precise STR that more clearly show something fishy is still going on:

1. open hamburger
2. open library within hamburger
3. click library button (Note: if you were to open any other subview than the library in step 2, clicking the library button in the toolbar opens the library immediately. It not opening immediately here is a bug, which then causes the next confusion, I think... we must be exiting early somewhere and leaving things in a broken state). This closes the hamburger panel and does nothing else.
4. DO NOT click the library button again
5. instead, open the hamburger panel
6. open the library subview

Now the animation is broken. At the end of the animation's completion time, the subview shows up, but before that the panel gets way too narrow.

::: browser/components/customizableui/PanelMultiView.jsm:543
(Diff revisions 11 - 12)
>  
>        this._viewShowing = viewNode;
> +      // Because the 'mainview' attribute may be out-of-sync, due to view node
> +      // reparenting in combination with ephemeral PanelMultiView instances,
> +      // this is the best place to correct it (just before showing).
> +      if (viewNode == this._mainView)

So if the `_mainView` getter is expensive, should this compare ids instead of references?
Attachment #8911795 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #35)
> 1. open hamburger
> 2. open library within hamburger
> 3. click library button (Note: if you were to open any other subview than
> the library in step 2, clicking the library button in the toolbar opens the
> library immediately. It not opening immediately here is a bug, which then
> causes the next confusion, I think... we must be exiting early somewhere and
> leaving things in a broken state). This closes the hamburger panel and does
> nothing else.
> 4. DO NOT click the library button again
> 5. instead, open the hamburger panel
> 6. open the library subview
> 
> Now the animation is broken. At the end of the animation's completion time,
> the subview shows up, but before that the panel gets way too narrow.

Nice find! (BTW, I love how all these errors have become so explicit and nice to debug. Odd, perhaps, but that's how I feel.)
This indeed has to do with the fact we return early in PanelUI.js and I see why :)

> So if the `_mainView` getter is expensive, should this compare ids instead
> of references?

Cute :) Of course, will change this.
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

https://reviewboard.mozilla.org/r/183224/#review190082

Woop! Ship it! Thanks and sorry for the loooong slog here.

::: browser/components/customizableui/PanelMultiView.jsm:448
(Diff revisions 12 - 13)
> -    if (!this._mainView)
> -      return;
> +    if (!this._mainViewId)
> +      return Promise.resolve();
>  
>      if (this.panelViews) {
> -      this.showSubView(this._mainView);
> +      return this.showSubView(this._mainView);
>      } else {

Nit: No else after return (this is also coming up on eslint on your trypush). :-)

::: browser/components/customizableui/PanelMultiView.jsm:460
(Diff revisions 12 - 13)
>            this.node.setAttribute("viewtype", "main");
>          });
>        }
>  
>        this._shiftMainView();
> +      return Promise.resolve();

Shouldn't this resolve after the `_transitionHeight` method returns? I don't think I really mind given that (AIUI) the identity popup is the only remaining thing here so we won't really have issues here anyway, but in terms of correctness...

::: browser/components/customizableui/PanelMultiView.jsm:1052
(Diff revisions 12 - 13)
> +        if (!this.panelViews)
> -        this.descriptionHeightWorkaround();
> +          this.descriptionHeightWorkaround();

Can you elaborate on why this change was needed? It looks correct to me off-hand, but I'm still curious how it relates to the last bug you're fixing here. :-)

::: browser/components/customizableui/content/panelUI.js:430
(Diff revisions 12 - 13)
> +      let multiView = viewNode.panelMultiView;
> +      if (multiView && multiView.current == viewNode) {
> +        await multiView.showMainView();
> +      }

I think I slightly prefer using a new variable here, explicitly called "oldMultiView" or something like that, rather than 'reusing' the `multiView` variable later on when we create a completely new one...
Attachment #8911795 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #38)
> Woop! Ship it! Thanks and sorry for the loooong slog here.

Please don't worry about it! I'm actually happier this way, because since you took your time to review thoroughly I think we can feel more confident that uplifting this patch is more than OK.
I do think we should let it bake in Nightly for a couple of days.

> Shouldn't this resolve after the `_transitionHeight` method returns? I don't
> think I really mind given that (AIUI) the identity popup is the only
> remaining thing here so we won't really have issues here anyway, but in
> terms of correctness...

The callback passed into `_transitionHeight` is synchronous and it doesn't return a Promise. So returning Promise.resolve() here unconditionally for the time being seemed to strike the right balance.

> Can you elaborate on why this change was needed? It looks correct to me
> off-hand, but I'm still curious how it relates to the last bug you're fixing
> here. :-)

Yeah, not really related to this bug - but since all this gajingle is neatly folded into `hideAllViewsExcept(x)` for Photon panels from here on out, this is superfluous processing. I thought it might shave off a sync reflow for us, but that test doesn't cover this case.
But if the 'Synced Tabs' panelview was the main view, it would have! :-P
Depends on: 1400259
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35acf942df34
Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently. r=Gijs
Hi Mike, should we uplift this fix to Beta57 once it has landed in central and baked for a bit? See https://bugzilla.mozilla.org/show_bug.cgi?id=1400259#c13
Flags: needinfo?(mdeboer)
Absolutely! This is an important patch to get into 57.
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/35acf942df34
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Hi Mike, please nominate a patch for uplift to Beta57.
Flags: needinfo?(mdeboer)
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1374749.
[User impact if declined]: Empty panels when the Library view is clicked, but also for WebExtension browser actions of type 'view' can show an empty panel when placed inside the Overflow menu. Absolutely jarring and not something we can ship with.
[Is this code covered by automated tests?]: Yes, primarily in WebExtension browser tests.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, STR can be found in comment 0.
[List of other uplifts needed for the feature/fix]: None, standalone patch.
[Is the change risky?]: Moderately.
[Why is the change risky/not risky?]: Because the the code refactor that was necessary is larger than, for example, a 'make that label bold and make its color GrayText' patch. However, the code is contained to a single JSM and updates to consumers on the API level, so that's why I'd label it moderate at the _highest_.
[String changes made/needed]: n/a.
Flags: needinfo?(mdeboer)
Attachment #8911795 - Flags: approval-mozilla-beta?
Comment on attachment 8911795 [details]
Bug 1401991 - Ensure that we don't hide panelviews that are already reparented to another multi-view and ensure to hide other panels consistently.

Photon work, taking it.
Attachment #8911795 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
It seems that this fix may have introduced the regression found in Bug 1406892.

Because of that regression I can't verify this fix on Linux.
Depends on: 1406892
Depends on: 1405942
Depends on: 1407591
Depends on: 1408390
On Nightly 58 as of this date - 

On Windows/OS X, I am seeing the Library menu open on the toolbar when it is clicked while the Hamburger menu.
For Linux, nothing is opened and the Library button on the toolbar needs to be clicked twice.
Which behavior in Comment 50 is intended?
Flags: needinfo?(sledru)
302 to the dev
Flags: needinfo?(sledru) → needinfo?(mdeboer)
Emil noticed the same thing (comment 49), as in on Linux this behavior seems to manifest - see bug 1406892.

However, the most important thing that this bug fixes is mentioned in comment 46: the menu showed up _empty_ after following the STR in comment 0.
The 'regression' as implied is far less harmful than what this bug intended and did fix.
Flags: needinfo?(mdeboer)
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) with today's build.
status-firefox58: fixed → verified
Verified on Windows 10 64bit, macOS 10.12 and Ubuntu 16.04 64bit using Firefox 57.0b11 (BuildId:20171023180840).
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Depends on: 1408613
Depends on: 1413159
Depends on: 1435473
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: