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

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Bookmarks & History
P1
normal
VERIFIED FIXED
a year ago
11 months 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8891047 [details]
firefox nightly history.png

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

a year 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

a year 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

a year 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

a year ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(Assignee)

Comment 3

a year 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

a year 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

a year 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

a year 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
(Assignee)

Updated

a year ago
Duplicate of this bug: 1387851
(Assignee)

Updated

a year 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

a year 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

a year ago
My precious!
Assignee: mdeboer → gijskruitbosch+bugs

Comment 16

a year 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

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

Comment 19

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

Comment 20

11 months 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

11 months 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

11 months ago
Depends on: 1388753
status-firefox56: affected → disabled
status-firefox-esr52: --- → unaffected
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.