widget placement not available during onWindowOpened

VERIFIED FIXED in Firefox 46

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

({regression})

46 Branch
Firefox 47
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox46+ verified, firefox47+ verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
[Tracking Requested - why for this release]: regression from bug 1236605.  pocket menu items may be hidden when they shouldn't be.
Blocks: 1236605
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
(Assignee)

Comment 2

3 years ago
Created attachment 8725478 [details] [diff] [review]
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)
Assignee: nobody → mixedpuppy
Attachment #8725478 - Flags: review?(mconley)
Attachment #8725478 - Flags: review?(gijskruitbosch+bugs)
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 ?
Attachment #8725478 - Flags: review?(mconley)
Attachment #8725478 - Flags: review?(gijskruitbosch+bugs)
Attachment #8725478 - Flags: feedback+
(Assignee)

Comment 4

3 years ago
(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.
(Assignee)

Comment 5

3 years ago
Created attachment 8725878 [details] [diff] [review]
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.
Attachment #8725478 - Attachment is obsolete: true
Attachment #8725878 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8725878 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 7

3 years ago
> ::: 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.
(Assignee)

Comment 8

3 years ago
Created attachment 8726931 [details] [diff] [review]
fix menu item visibility
Attachment #8725878 - Attachment is obsolete: true
Attachment #8726931 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8726931 [details] [diff] [review]
fix menu item visibility

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

Nice, r=me
Attachment #8726931 - Flags: review?(gijskruitbosch+bugs) → review+
Regression from 46, tracking. We should uplift this before the merge...
tracking-firefox46: ? → +
tracking-firefox47: ? → +

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/50f911179445
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

3 years ago
Duplicate of this bug: 1254013

Updated

3 years ago
Keywords: regression
(Assignee)

Comment 14

3 years ago
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
Attachment #8726931 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(lhenry)
Attachment #8726931 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(Assignee)

Updated

3 years ago
Depends on: 1255824
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.
Flags: needinfo?(lhenry)
Attachment #8726931 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/775cb20a61fd
status-firefox46: affected → fixed
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
status-firefox47: fixed → verified
You need to log in before you can comment on or make changes to this bug.