Update the 'History' view in the Library for photon

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
4 months ago
9 days ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 2 bugs)

52 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
This button opens a subview when clicked, which contains:

* A button called 'View History Sidebar'
* A button called 'Clear Recent History'
* A button called 'Restore Previous Session' (disabled if there's no session)
* A separator
* A button called 'Recently Closed Tabs', which references another subview
* A button called 'Recently Closed Windows', which references another subview
* A separator
* A list of items visited today
* A separator
* A list of items visited yesterday

It is recommended to implement the complete subview in a separate bug, per-spec.

The title of the subview reads 'History', the same as the button label.
(Assignee)

Updated

4 months ago
Blocks: 1354534

Updated

4 months ago
No longer depends on: 1354532

Updated

4 months ago
No longer blocks: 1354534

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Updated

2 months ago
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]

Updated

2 months ago
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]

Updated

2 months ago
Summary: Add a 'History' button to the Library panel → Update the 'History' view in the Library for photon
(Assignee)

Updated

a month ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED

Updated

a month ago
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a month ago
mozreview-review
Comment on attachment 8888274 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/159222/#review164608

::: browser/themes/shared/menupanel.inc.css:261
(Diff revision 2)
>  #appMenu-library-history-button {
>    list-style-image: url(chrome://browser/skin/history.svg);
>  }
>  
> +#appMenuRecentlyClosedTabs,
> +#appMenuRecentlyClosedWindows,

I have to ask Aaron for a better icon here. The one in the spec also isn't very, well, good.

::: browser/themes/shared/menupanel.inc.css:276
(Diff revision 2)
>    list-style-image: url("chrome://browser/skin/device-mobile.svg");
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
>  #appMenuViewHistorySidebar,
> +#appMenuRestoreLastSession,

I don't really get why the sidebar icon is used for this button in the spec. I'll ask Aaron for a new icon here too.
(Assignee)

Comment 4

a month ago
Aaron, when you look at https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229252061, you'll see what I mention in comment 3: the icons used for 'Restore Previous Session' and 'Recently Closed Windows' are rather odd. Are you able to provide ones that are more related to what the action describes?
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
Perhaps new-window.svg would be a good fit for 'Recently Closed Windows'?
(Assignee)

Updated

a month ago
Flags: needinfo?(abenson)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Flags: needinfo?(abenson)

Comment 8

29 days ago
mozreview-review
Comment on attachment 8888274 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/159222/#review164906

r=me assuming Marco OKs the marker stuff, though if you want me to look at the ViewShowing stuff and/or the footer extraction if you decide to implement that, that is also OK.

::: browser/components/customizableui/CustomizableWidgets.jsm:198
(Diff revision 4)
>        // Populate our list of history
>        const kMaxResults = 15;
>        let doc = aEvent.target.ownerDocument;
>        let win = doc.defaultView;
>  
> +      if (win.gPhotonStructure) {

Nit: here and elsewhere, use AppConstants.MOZ_PHOTON_THEME, because the markup is wrong otherwise. Or maybe even && those. I don't know. We have 2 things, it is not going to work right one way or another if you try to flip this pref to true on a non-photon-theme thing, or if you flip the pref to false on a photon theme thing (because we'll use a non-photon panelmultiview for the history panel when it's opened standalone). Not sure if there's much we can do about this.

::: browser/components/customizableui/CustomizableWidgets.jsm:314
(Diff revision 4)
> +      // When either of these sub-subviews show, populate them with recently closed
> +      // objects data.
> +      document.getElementById(this.recentlyClosedTabsPanel).addEventListener("ViewShowing", this);
> +      document.getElementById(this.recentlyClosedWindowsPanel).addEventListener("ViewShowing", this);

TIL, you can't add the same listener (JS object identity) more than once. I thought we'd have to remove the listener in viewhiding as well, but I guess panelmultiviewhidden alone is enough, then.

::: browser/components/customizableui/CustomizableWidgets.jsm:328
(Diff revision 4)
>        // Middle clicking recently closed items won't close the panel - cope:
>        let onRecentlyClosedClick = function(aEvent) {

FWIW, this seems relevant to https://bugzilla.mozilla.org/show_bug.cgi?id=1377967 , maybe. Does this fix the history part of that bug?

::: browser/components/customizableui/CustomizableWidgets.jsm:364
(Diff revision 4)
> +      let utils = RecentlyClosedTabsAndWindowsMenuUtils;
> +      let fragment = utils[`get${viewType}Fragment`](window, "toolbarbutton",
> +        true, `menuRestoreAll${viewType}Subview.label`);

Nit: split the method name and the string id into temp vars for readability's sake, then line up the args with the starting ( ? :-)

::: browser/components/customizableui/CustomizableWidgets.jsm:373
(Diff revision 4)
> +      this._panelMenuView._setEmptyPopupStatus(panelview, !elementCount);
> +      while (--elementCount >= 0) {
> +        let element = fragment.children[elementCount];
> +        CustomizableUI.addShortcut(element);
> +        element.classList.add("subviewbutton");
> +        if (!element.classList.contains("restoreallitem"))

The invision spec has the 'restore all' items as footers. Could we split them out of the fragment here?

Note that their strings (in the mockup) don't really match. Probably good to check with UX what we should be using.

::: browser/components/customizableui/PanelMultiView.jsm:687
(Diff revision 4)
> +    let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id);
> +    if (custWidget)
> +      custWidget["on" + eventName]({ target: viewNode });

It's a little ugly that we're then not using this for ViewShowing. Is that hard to do?

Also, the onFoo version can't cancel anything. Given it's for ViewHiding, I guess it doesn't matter... but it will matter for ViewShowing if we try to use it for that too.

::: browser/components/places/content/browserPlacesViews.js:2118
(Diff revision 4)
> -      if (!panelview._startMarker.previousSibling &&
> -          !panelview._endMarker.nextSibling)
> +      if (!panelview._startMarker ||
> +          (!panelview._startMarker.previousSibling && !panelview._endMarker.nextSibling)) {

This makes me nervous because I'm not sure off-hand what this does. Can Marco maybe sanity-check that this won't break the assumptions of other places views?

::: browser/components/places/content/browserPlacesViews.js:2174
(Diff revision 4)
>      if (!this.controllers.getControllerCount() && this._controller)
>        this.controllers.appendController(this._controller);
>    }
>  }
> +
> +this.PlacesPanelview = PlacesPanelview;

Why do we need this now?
Attachment #8888274 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 9

29 days ago
mozreview-review-reply
Comment on attachment 8888274 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/159222/#review164906

> TIL, you can't add the same listener (JS object identity) more than once. I thought we'd have to remove the listener in viewhiding as well, but I guess panelmultiviewhidden alone is enough, then.

Nice, eh!

> FWIW, this seems relevant to https://bugzilla.mozilla.org/show_bug.cgi?id=1377967 , maybe. Does this fix the history part of that bug?

Ermz, maybe? I don't have a middle click handy atm.

> The invision spec has the 'restore all' items as footers. Could we split them out of the fragment here?
> 
> Note that their strings (in the mockup) don't really match. Probably good to check with UX what we should be using.

Oooh, I hadn't even checked the specs for those :/ Thanks for the heads-up!

> It's a little ugly that we're then not using this for ViewShowing. Is that hard to do?
> 
> Also, the onFoo version can't cancel anything. Given it's for ViewHiding, I guess it doesn't matter... but it will matter for ViewShowing if we try to use it for that too.

Good point; it resulted in a very nice cleanup as well!

> Why do we need this now?

TYL: standalone class expressions are block-scoped. I did, however, adjust the code somewhat to use `var PlacesPanelview = ` to make the scoping effort a bit clearer.
Comment hidden (mozreview-request)
(Assignee)

Updated

29 days ago
Attachment #8888274 - Flags: review+ → review?(gijskruitbosch+bugs)

Comment 11

29 days ago
mozreview-review
Comment on attachment 8888274 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/159222/#review165112

This looks OK besides the below, but also note that on try, the webextension popup tests are unhappy. I expect it's the messing with the event listeners and how they relate to the timing of the panelmultiview's `_viewShowing` prop. :-(

::: browser/components/customizableui/CustomizableWidgets.jsm:370
(Diff revisions 4 - 5)
> -      let fragment = utils[`get${viewType}Fragment`](window, "toolbarbutton",
> -        true, `menuRestoreAll${viewType}Subview.label`);
> +      let method = `get${viewType}Fragment`;
> +      let fragment = utils[method](window, "toolbarbutton");
>        let elementCount = fragment.childElementCount;
>        this._panelMenuView._setEmptyPopupStatus(panelview, !elementCount);
> +      if (elementCount) {
> +        let body = fragment.insertBefore(document.createElement("vbox"), fragment.children[0]);

Wouldn't it be easier to just create the vbox with the class, and append the fragment to the vbox, and then take out the restoreallitem individually, finally appending the footer and the vbox? That's fewer DOM manipulations from JS. :-)

::: browser/components/customizableui/PanelMultiView.jsm:690
(Diff revisions 4 - 5)
> +      }
> +    }
> +
> +    let evt = new this.window.CustomEvent(eventName, {
> +      bubbles: true,
> +      cancelable: eventName != "ViewShown"

I don't think viewhiding is cancelable... should this be eventName == "ViewShowing" ?
Attachment #8888274 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 13

25 days ago
mozreview-review
Comment on attachment 8888274 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/159222/#review166138

I trust Gijs on the general review, I'm just leaving a comment about the markers code.

::: browser/components/customizableui/CustomizableWidgets.jsm:362
(Diff revision 6)
> +      let viewType = panelview.id == this.recentlyClosedTabsPanel ? "Tabs" : "Windows";
> +
> +      while (panelview.firstChild) {
> +        panelview.firstChild.remove();
> +      }
> +      panelview._emptyMenuitem = panelview._startMarker = panelview._endMarker = null;

I'm not extremely happy about this pollution, where Customizeable widgets needs to play with the code internals of the view... Can we provide a cleaner abstraction for it, a method on the view to clear it completely, includind the markers?
Something like resetContents or clearAllContents that would include both the removal and resetting the internal details.
Attachment #8888274 - Flags: review?(mak77)

Updated

25 days ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1

Comment 14

25 days ago
(In reply to Marco Bonardo [::mak] from comment #13)
> Comment on attachment 8888274 [details]
> Bug 1354533 - Update the History panelview when it's shown inside the new
> Library panel.
> 
> https://reviewboard.mozilla.org/r/159222/#review166138
> 
> I trust Gijs on the general review, I'm just leaving a comment about the
> markers code.
> 
> ::: browser/components/customizableui/CustomizableWidgets.jsm:362
> (Diff revision 6)
> > +      let viewType = panelview.id == this.recentlyClosedTabsPanel ? "Tabs" : "Windows";
> > +
> > +      while (panelview.firstChild) {
> > +        panelview.firstChild.remove();
> > +      }
> > +      panelview._emptyMenuitem = panelview._startMarker = panelview._endMarker = null;
> 
> I'm not extremely happy about this pollution, where Customizeable widgets
> needs to play with the code internals of the view... Can we provide a
> cleaner abstraction for it, a method on the view to clear it completely,
> includind the markers?
> Something like resetContents or clearAllContents that would include both the
> removal and resetting the internal details.

Because Mike's out, I'm going to be pushing this over the line. Where do you want this method to live? On some kind of places view object?
Flags: needinfo?(mak77)
I'd expect it to exist at the view level (same level as ._startMarker and friends)
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)

Comment 17

24 days ago
mozreview-review
Comment on attachment 8889897 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/160964/#review166276
Attachment #8889897 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 18

24 days ago
mozreview-review
Comment on attachment 8889897 [details]
Bug 1354533 - Update the History panelview when it's shown inside the new Library panel.

https://reviewboard.mozilla.org/r/160964/#review166278

yes, it's a little bit more abstracted away now.
Note I didn't do a full review, just checked the previous issue.
If it works, it will be fine :)
Attachment #8889897 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 20

24 days ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/58b73242f3ef
Update the History panelview when it's shown inside the new Library panel. r=mak

Comment 21

24 days ago
(The earlier trypush was green bar some webextension issues, which I fixed locally by always passing the `detail` parameter. So I pushed with that updated.)

Comment 22

24 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58b73242f3ef
Status: ASSIGNED → RESOLVED
Last Resolved: 24 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Verified on Windows, Mac and Ubuntu with the specs in Comment 1.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
^ Meant to be Comment 0.

Updated

22 days ago
Depends on: 1385083
Blocks: 1387512
(Assignee)

Updated

9 days ago
Duplicate of this bug: 1374692
You need to log in before you can comment on or make changes to this bug.