1252661 - widget placement not available during onWindowOpened

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Placement of widgets is not necessarily available via getPlacementOfWidget during the first onWindowOpened.  Subsequent onWindowOpened calls (for 2nd/etc window) works fine.  

This may be affected by timing somehow, perhaps number of addons making widgets, etc. since my regular profile doesn't have the problem whereas nightly and fresh profiles on aurora do show the problem.

If I listen for onWidgetAfterDOMChange I can get placement.

Since placement is used in the pocket addon to determine visibility of the menu's, this issue caused the menu's to remain hidden on the fist opened window.

STR:
start firefox with a fresh profile, look at bookmarks menus (button and menu bar)

expected:
see pocket menu item

actual:
you don't see the pocket menu item

Comment 1

[Shane Caraveo (:mixedpuppy)]

[Tracking Requested - why for this release]: regression from bug 1236605.  pocket menu items may be hidden when they shouldn't be.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: fix menu item visibility

I think gijs is pto, so asking for alternate reviewers.

this patch:
- makes onWindowOpen only add dom elements for menu items, removing the visibility update which didn't work on first window opened
- replaces widget added/removed with onWidgetAfterDOMChange and relies on that for updating menu item visibility.
- fixes updating reader mode buttons (prevents multiple additions, potential multiple message listeners in PocketReader class, etc)

Comment 3

[:Gijs (he/him)]

Comment on attachment 8725478 [details] [diff] [review]
fix menu item visibility

Review of attachment 8725478 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pocket/bootstrap.js
@@ +362,5 @@
>      this._cachedSheet = styleSheetService.preloadSheet(gPocketStyleURI,
>                                                         this._sheetType);
>      AboutSaved.register();
>      AboutSignup.register();
> +    CustomizableUI.addListener(this);

Why was this moved to happen before the pocket widget is created?

@@ +469,2 @@
>    },
> +  onWidgetAfterDOMChange: function(aWidgetNode) {

This will get called with an actual DOM node, and so if there are multiple windows, it will get called multiple times, so I don't think you need to loop over windows here.

It's not really clear to me why you switched to this over onWidgetAdded/onWidgetRemoved. Can you expand on the reasoning for that change?

@@ +484,5 @@
>          doc.getElementById(prefix + "pocketSeparator").hidden = hidden;
>        }
>      }
>      // enable or disable reader button
> +    PocketReader.update(hidden);

This will call update() multiple times if there are multiple windows, because then this method gets called multiple times.

AIUI there's now only 1 callsite for updatePocketItemVisibility, so maybe this line and its comment should just go there, after the for loop in onWidgetAfterDOMChange ?

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 8725478 [details] [diff] [review]
> fix menu item visibility
> 
> Review of attachment 8725478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/pocket/bootstrap.js
> @@ +362,5 @@
> >      this._cachedSheet = styleSheetService.preloadSheet(gPocketStyleURI,
> >                                                         this._sheetType);
> >      AboutSaved.register();
> >      AboutSignup.register();
> > +    CustomizableUI.addListener(this);
> 
> Why was this moved to happen before the pocket widget is created?

To catch another notification I was testing with, iirc onwidgetadded also was not called on the first window.

> @@ +469,2 @@
> >    },
> > +  onWidgetAfterDOMChange: function(aWidgetNode) {
> 
> This will get called with an actual DOM node, and so if there are multiple
> windows, it will get called multiple times, so I don't think you need to
> loop over windows here.
> 
> It's not really clear to me why you switched to this over
> onWidgetAdded/onWidgetRemoved. Can you expand on the reasoning for that
> change?

Yeah, getPlacementOfWidget is not returning a placement until after DOM has changed, thus the menu items were hidden even though the button was then placed.

> 
> @@ +484,5 @@
> >          doc.getElementById(prefix + "pocketSeparator").hidden = hidden;
> >        }
> >      }
> >      // enable or disable reader button
> > +    PocketReader.update(hidden);
> 
> This will call update() multiple times if there are multiple windows,
> because then this method gets called multiple times.
> 
> AIUI there's now only 1 callsite for updatePocketItemVisibility, so maybe
> this line and its comment should just go there, after the for loop in
> onWidgetAfterDOMChange ?

I'll look at that, I'm not certain if the reader is adding to all windows or now.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: fix menu item visibility

addressed calls to reader update.

onWidgetAdded was not called on first startup because the widget was added prior to the listener.  Nonetheless, placement is not available yet during onWidgetAdded or onWindowOpened for the first window on startup.  Waiting for the onWidgetAfterDOMChange notification allows us to determine the placement properly, thus properly show/hide the menu items.  Adding the listener addition before widget creation is defensive at this point since it works either way with onWidgetAfterDOMChange.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8725878 [details] [diff] [review]
fix menu item visibility

Review of attachment 8725878 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pocket/bootstrap.js
@@ +296,5 @@
>      mm.removeMessageListener("Reader:OnSetup", this);
>      mm.removeMessageListener("Reader:Clicked-pocket-button", this);
> +    this.update(true);
> +  },
> +  update: function(hidden) {

Nit: I think it would make more sense to have the param name and meaning be "visible" and inverse the logic. Feels odd to startup(false) and shutdown(true), but maybe that's just me?

@@ +477,1 @@
>      }

It is still the case that this loop is not necessary; you'll get notified for each dom node, so for each window, and you can get the document off the widget node using aWidgetNode.ownerDocument.

@@ +477,3 @@
>      }
> +    // enable or disable reader button
> +    PocketReader.update(hidden);

This will now run N times if there are N open windows, and twice on startup (once from startup(), once from onWidgetAfterDOMChange!). We should probably persist the current hidden state on this object and only run code in PocketReader.update if the new state differs from the old one.

Comment 7

[Shane Caraveo (:mixedpuppy)]

> ::: browser/extensions/pocket/bootstrap.js
> @@ +296,5 @@
> >      mm.removeMessageListener("Reader:OnSetup", this);
> >      mm.removeMessageListener("Reader:Clicked-pocket-button", this);
> > +    this.update(true);
> > +  },
> > +  update: function(hidden) {
> 
> Nit: I think it would make more sense to have the param name and meaning be
> "visible" and inverse the logic. Feels odd to startup(false) and
> shutdown(true), but maybe that's just me?

next patch will help reduce that weirdness, but I like to keep it "hidden" since that is the attr on the elements.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: fix menu item visibility

Comment 9

[:Gijs (he/him)]

Comment on attachment 8726931 [details] [diff] [review]
fix menu item visibility

Review of attachment 8726931 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, r=me

Comment 10

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/50f9111794457fb3d992a0c517dbbe1057dbdd0a
Bug 1252661 fix visibility state of pocket menu items, r=gijs

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Regression from 46, tracking. We should uplift this before the merge...

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50f911179445

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8726931 [details] [diff] [review]
fix menu item visibility

Approval Request Comment
[Feature/regressing bug #]:pocket addon
[User impact if declined]:missing menu items on startup
[Describe test coverage new/current, TreeHerder]:manual
[Risks and why]: larger patch than I'd like to uplift, but I still feel it's relatively low risk, impact would be limited to pocket
[String/UUID change made/needed]:none

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8726931 [details] [diff] [review]
fix menu item visibility

Switching aurora flag to beta:? nom since this is already in 47. Liz, fyi.

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8726931 [details] [diff] [review]
fix menu item visibility

We want the pocket menus to work, let's uplift to beta. This should make it into 46 beta 2.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/775cb20a61fd

Comment 18

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

This bug is verified fixed on:
- 46.0b6-build1 (20160328182534),
- 47.0a2 (2016-03-31),
- 48.0a1 (2016-03-30),
using Ubuntu 14.04 x86, Mac OS X 10.11 Beta and Windows 10 x64.