Closed
Bug 1252661
Opened 6 years ago
Closed 6 years ago
widget placement not available during onWindowOpened
Categories
(Firefox :: Pocket, defect)
Tracking
()
VERIFIED
FIXED
Firefox 47
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
6.89 KB,
patch
|
Gijs
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•6 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•6 years ago
|
||
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 3•6 years ago
|
||
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•6 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•6 years ago
|
||
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 6•6 years ago
|
||
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•6 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•6 years ago
|
||
Attachment #8725878 -
Attachment is obsolete: true
Attachment #8726931 -
Flags: review?(gijskruitbosch+bugs)
Comment 9•6 years ago
|
||
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+
Assignee | ||
Comment 10•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/50f9111794457fb3d992a0c517dbbe1057dbdd0a Bug 1252661 fix visibility state of pocket menu items, r=gijs
Regression from 46, tracking. We should uplift this before the merge...
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50f911179445
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Keywords: regression
Assignee | ||
Comment 14•6 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?
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•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/775cb20a61fd
Updated•6 years ago
|
Flags: qe-verify+
Comment 18•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•