History button (and history panel in library, after bug 1354117) doesn't show the Recent History

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: smartfon.reddit, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {regression})

56 Branch
Firefox 57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170727162801

Steps to reproduce:

Visit sites. Click on the History button. The "Recent History" section is always empty. History is shown properly when it's accessed via Sidebar and Ctrl+Shift+H.

Tested with July 26 and July 27 (latest) builds, and a fresh profile.
Component: Untriaged → Bookmarks & History

Comment 1

2 years ago
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2039119836eafbe19e47a9db193141e5f1da355d&tochange=58b73242f3ef211bdac77612e8cd797b49a22a35

Regressed by: 58b73242f3ef	Mike de Boer — Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. r=mak


no automate test?
Blocks: 1354533
status-firefox55: --- → unaffected
status-firefox56: --- → affected
Flags: needinfo?(mdeboer)
Flags: needinfo?(mak77)
Keywords: regression

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Please stop needinfo-ing a bunch of people to ask about automated tests, it's quite annoying.
Flags: needinfo?(mak77)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Flags: needinfo?(mdeboer)
Priority: -- → P1
Summary: History button doesn't show the Recent History. Available on Sidebar and Ctrl+Shift+H. → History button doesn't show the Recent History (but does when opened from the library)
Whiteboard: [photon-structure]
Flags: qe-verify?
(Assignee)

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(Assignee)

Comment 3

2 years ago
I tested, and this doesn't reproduce on cedar, so we'll be good for the 56 release, just need to fix for photon.

It seems we run the right places view initialization when this happens, so I don't understand off-hand why it doesn't work. Marco, would you know why the places view doesn't successfully render places nodes in this case? It seems like popup._placesNode is null in the single panel case, but not in the case where the popup is shown from the library. What's supposed to instantiate that?
Flags: needinfo?(mak77)
how do I reproduce this? both the history button and Library/History are WFM...
(Assignee)

Comment 5

2 years ago
(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #4)
> how do I reproduce this? both the history button and Library/History are
> WFM...

0. move history button to the toolbar
1. open a new window
2. open history button (don't open the library entrypoint first).
(Assignee)

Comment 6

2 years ago
You also need the photon structure pref set to the nightly default (ie photon structure enabled).
OK, I needed to open the new window. I'll into this tomorrow morning.
(Assignee)

Comment 9

2 years ago
(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #8)
> Could it be because of this?
> http://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/base/content/browser-places.
> js#1998

Maybe, though copying that to https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#312 doesn't seem to be fixing things.
the .place setter creates the result, and adds the view as an observer
that will set view.result to the result.
the result setter sets _rootElt._placesNode

_onPopupShowing does
let popup = aEvent.originalTarget;
and then uses popup._placesNode

BUT the PlacesPanelview constructor doe:
this._onPopupShowing({ originalTarget: this._viewElt });

Instead of passing _rootElt to _onPopupShowing, it's passing _viewElt.
_viewElt._placesNode is undefined, since it's set on _rootElt.

Just changing this._viewElt to this._rootElt seems to fix the problem for me.
Flags: needinfo?(mak77)
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15

Updated

2 years ago
Blocks: 1387512
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1387851
(Assignee)

Updated

2 years ago
Summary: History button doesn't show the Recent History (but does when opened from the library) → History button (and history panel in library, after bug 1354117) doesn't show the Recent History
Assignee: gijskruitbosch+bugs → mdeboer
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8894520 [details]
Bug 1385083 - Name and use the event argument appropriately by fixing a typo.

https://reviewboard.mozilla.org/r/165692/#review170768

This isn't sufficient, please see the patch I raced with.
Attachment #8894520 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 15

2 years ago
My precious!
Assignee: mdeboer → gijskruitbosch+bugs

Comment 16

2 years ago
mozreview-review
Comment on attachment 8894519 [details]
Bug 1385083 - history panel should actually list history,

https://reviewboard.mozilla.org/r/165688/#review170770

This looks good to me! r=me with an explaination about the change in browserPlacesViews.js.

::: browser/components/customizableui/CustomizableWidgets.jsm:194
(Diff revision 1)
>          "&maxResults=42&excludeQueries=1";
> +
> +      // XPCOMUtils.defineLazyScriptGetter can't return class constructors, so
> +      // trigger the getter once without using the result before calling
> +      // PlacesPanelview as a constructor.
> +      window.PlacesPanelview;

I've run into this before and I think changing `var PlacesPanelview = class extends PlacesViewBase {` to `this.PlacesPanelview = class extends PlacesViewBase {` in browserPlacesViews.js fixed it for me.

::: browser/components/places/content/browserPlacesViews.js:2015
(Diff revision 1)
>      options.viewElt = panelview;
>      super(place, options);
>      this._viewElt._placesView = this;
>      // We're simulating a popup show, because a panelview may only be shown when
>      // its containing popup is already shown.
> -    this._onPopupShowing({ originalTarget: this._viewElt });
> +    this._onPopupShowing({ originalTarget: this._rootElt });

Why this change? I see the bookmarks panelview still works, so was this a bug I introduced?
Attachment #8894519 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)

Comment 18

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87152019960e
history panel should actually list history, r=mikedeboer

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87152019960e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 20

2 years ago
I have reproduced this bug with Nigtly 56.0a1 (2017-07-27) on Windows 8.1 (64 bit).

This bug's fix is verified on latest Nightly 57.0a1.

Build ID : 20170808114032
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170809]

Comment 21

2 years ago
Managed to reproduce the issue on an affected build (Firefox 57.0a1), using the steps to reproduce from Comment 0, on Windows 10 X 64 bit. Bug 1385083 is fixed & verified.
build ID: 20170809100326
[bugday-20170809]
status-firefox57: fixed → verified
(Assignee)

Updated

2 years ago
Depends on: 1388753
status-firefox56: affected → disabled
status-firefox-esr52: --- → unaffected

Comment 22

2 years ago
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.