1069300 - (Forget-button) Implement privacy button and subview

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Design: bug 1064524

This explicitly doesn't cover dealing with placement or telling the user we added this button, but purely deals with adding the button to the palette and getting the subview hooked up and implemented.

I'll also split using final imagery here off into its own bug, but I'd like to get the functionality and any styling that doesn't need images fixed as part of this bug. I think for the icon, I'll just scaleX(-1) the reload icon for now, or something.

Comment 1

[Dão Gottwald [:dao]]

For maintenance and to ensure keyboard and mouse users have access to the same feature set, it would be nice if most of the code could be shared with History > Clear Recent History.

The design is close to that existing UI but also different in some ways for reasons that aren't obvious to me. It's basically the same use case, right? I suspect that, while the styling may differ, we should end up with the same basic structure for both UIs.

Comment 2

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #1)
> For maintenance and to ensure keyboard and mouse users have access to the
> same feature set, it would be nice if most of the code could be shared with
> History > Clear Recent History.

The UI design is so different that I don't think sharing the XUL will be helpful.

Regarding access issues: we can add functionality to close all open windows and tabs to the existing dialog, and add the 5 minute timeframe. I don't think that should be done in this bug.

> The design is close to that existing UI but also different in some ways for
> reasons that aren't obvious to me. It's basically the same use case, right?

I don't think so, but I'll leave answering this up to UX. I'm not sure why these concerns are being raised after the design was done. While they are certainly not any less valid for that, it makes it feel an awful lot like pure stop energy, especially when there are other people advocating for an expedited release of the feature. I've been in this situation once before with bug 951627, and I have no appetite for repeating the experience.

> I suspect that, while the styling may differ, we should end up with the same
> basic structure for both UIs.

It's not clear to me what you mean by "basic structure" here. The UI is different enough (radio button vs. menulist, details aren't available in the subview, pref-based dialog vs. subview panel, persists pref vs. resets to 5 minutes every time) that I don't think there's much opportunity to share code beyond sanitize.js.

Comment 3

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (Mostly gone until 22/9) from comment #2)
> > The design is close to that existing UI but also different in some ways for
> > reasons that aren't obvious to me. It's basically the same use case, right?
> 
> I don't think so, but I'll leave answering this up to UX. I'm not sure why
> these concerns are being raised after the design was done.

Because I'm watching Toolbars & Customization and bug 1064524 was filed in the wrong component.

> > I suspect that, while the styling may differ, we should end up with the same
> > basic structure for both UIs.
> 
> It's not clear to me what you mean by "basic structure" here. The UI is
> different enough (radio button vs. menulist, details aren't available in the
> subview, pref-based dialog vs. subview panel, persists pref vs. resets to 5
> minutes every time) that I don't think there's much opportunity to share
> code beyond sanitize.js.

I said we /should/ end up with the same basic structure. Clearly, like you said and like I mentioned earlier, with the current mockup and the current state of sanitize.xul, there /are/ structural differences, but they seem largely arbitrary. Only dialog vs. (subview) panel is inevitable, which is why I said the styling may differ.

Comment 4

[Justin Dolske [:Dolske]]

Strict consistency or UI reuse with the existing CRH feature is not a goal of this project. If there are opportunities to do so without causing other difficulties, fine. But in general this was designed to be straightforward to implement on a tight schedule, and that's what we should do.

Comment 5

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch (Mostly gone until 22/9) from comment #2)

> Regarding access issues: we can add functionality to close all open windows
> and tabs to the existing dialog, and add the 5 minute timeframe. I don't
> think that should be done in this bug.

Correct. If adding something like this to the backend is the most expedient route to finishing, fine. But changing the existing CRH feature should be considered out-of-scope.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Strict consistency or UI reuse with the existing CRH feature is not a goal
> of this project.

Having two close but different UIs for the same purpose seems like a source for user confusion that should be avoided in principal. Maybe my perception is wrong and somebody can clarify in how far the purposes are different (and whether there's a plan for exposing the new design keyboard to keyboard users if it does cover a different use case).

As for tight schedule, the new design doesn't look a lot simpler to implement than the existing dialog, especially if we could have reused the UI code, let alone that I wasn't even saying that the new design should follow the old dialog -- a possible outcome could be to have the dialog updated to the new design in a followup.

Comment 7

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

I still need to tweak the styling some more, but I believe that the behaviour here is otherwise correct. Can you have an early feedback pass about how this works? Note that I've not yet looked at implementing the 'success' panel; not sure yet if that will need to be a followup, it kind of depends on how hard the styling here turns out to be.

Comment 8

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

This still needs finalized radio button styles (Stephen, does the border from the mockup just appear on hover/active, or also for selected things? How does that work with the adjacency, which AFAICT from the mockup leaves 0 margin between elements?), I need to adjust the wrapping of text around the big logo at the top of the panel in both the 'thank you' and main subview, probably with a replacement logo for now. Otherwise, I think this is ready for review - feedback? for now because of the remaining issues.

Comment 9

[:Gijs (he/him)]

Attachment: update toolbar and menuPanel sprites,

Splitting off the updating of all the existing assets, which is kind of boring.

Comment 10

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

Apart from the size of the icon at the top, this is now complete including the use of the correct icons, AFAICT. I'll leave a needinfo for shorlander for some of the UI considerations I'm not 100% sure about yet.

Comment 11

[:Gijs (he/him)]

So I have the following remaining concerns:

1) what exactly do we want this dialog to clear? Obviously:
- cookies
- history
- windows

but then there's less obvious stuff. Particularly:

formdata - form/search history. To take the canonical example from the design, if I search for "justin bieber" and then use the button, the google history result will disappear, but the search autocomplete history will still have that input listed.

passwords - probably not

cache - can only be deleted wholesale, so I'm leaning towards "no"

siteSettings - (e.g. zoom, allowing images, etc.) ditto

offlineApps - ditto

downloads - ?? not listed, but I guess for the usecase of this button, users might expect this to work?

sessions - these are open http auth sessions and the like, and we can only nuke all of them. I'd lean towards leaving these.


2) the mockups have different sizes of the red icon thingy in the header of the subview when it is in the panel vs. when it's on the toolbar. The alignment of the other items in the top portion is different, too. Is this really important or can we just leave it the same?

I'm not sure why the two are different, and if I'd be meant to use the toolbar "open" state icon for the smaller version of the header image (the assets in bug 1069303 only supply the 48px version). I'd prefer to just leave them the same unless there's a compelling reason to differentiate.

3) the radio button styles - see comment #8. I think what I've implemented is what the spec means, but I'm not 100% sure. Can I get a basic ui-review? :-)

(builds will appear at https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2082cd4b83d9 when https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2082cd4b83d9 finishes)

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> downloads - ?? not listed, but I guess for the usecase of this button, users
> might expect this to work?

... by which I mean, they might expect that dodgy (for whatever meaning of the word) download they just did to disappear from the list?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8494602 [details] [diff] [review]
update toolbar and menuPanel sprites,

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

This doesn't update browser/themes/window/Toolbar-aero.png

Comment 14

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> Comment on attachment 8494602 [details] [diff] [review]
> update toolbar and menuPanel sprites,
> 
> Review of attachment 8494602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't update browser/themes/window/Toolbar-aero.png

Ugh, that doesn't seem to be in attachment 8494206 [details]. Stephen?

Comment 15

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #11)

> 1) what exactly do we want this dialog to clear?

Hmm, I think we settled on the same as CRH's defaults -- everything except "Offline Data" and "Site Preferences". Gavin, Philipp, and I talked though this a couple times, generally around the issue that some of these don't actually clear by timespan (bug 771630) and the tradeoff between clearing too much vs not enough.

> 2) the mockups have different sizes of the red icon thingy in the header of
> the subview when it is in the panel vs. when it's on the toolbar. The
> alignment of the other items in the top portion is different, too. Is this
> really important or can we just leave it the same?

Steven's call, but I assume both are intentional to keep the subview narrow.

> 3) the radio button styles - see comment #8. I think what I've implemented
> is what the spec means, but I'm not 100% sure. Can I get a basic ui-review?
> :-)

Seems ok to me, although the bold->normal transition is a little weird because the text size (width) changes... It grabs my eye away from the item that is actually being clicked. To fix we'd have to either get rid of the style change, or make it only a color change (black = selected, 90% opaque = unselected). Again, Steven's call.

Two other nits:

* Seems like the red Forget button should have a mousedown state? Especially since there can be a delay between clicking and getting to a new empty window, during which it's unclear if something is happening.

* The "Your recent history is cleared!" arrowpanel should dismiss itself when clicking elsewhere. Currently it persists until you click "Thanks!".

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8494603 [details] [diff] [review]
add a privacy/forget/panic button,

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

Just some feedback until the next patch comes around.

::: browser/base/content/browser.js
@@ +7374,5 @@
> +      let popup = document.getElementById("panic-button-success-notification");
> +      popup.hidden = false;
> +      let widget = CustomizableUI.getWidget("panic-button").forWindow(window);
> +      let anchor = widget.anchor;
> +      popup.openPopup(anchor, popup.getAttribute("position"));

The anchor for this panel (the toolbarbutton) is different than the anchor for the Forget panel (the toolbarbutton's icon).

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +937,5 @@
> +      }
> +    },
> +    _getSanitizeRange: function(aDocument) {
> +      let group = aDocument.getElementById("PanelUI-panic-timeSpan");
> +      return this._Sanitizer.getClearRange(1 * group.value);

return this._Sanitizer.getClearRange(+group.value);

The unary plus operator handles string to integer coercion as well.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +203,5 @@
> +          <label id="PanelUI-panic-warning" value="&panicButton.view.undoWarning;"/>
> +        </vbox>
> +        <toolbarbutton id="PanelUI-panic-view-button"
> +                       label="&panicButton.view.forgetButton;"
> +                       />

nit, move the /> up to the end of the previous line.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +775,5 @@
>  
> +<!ENTITY panicButton.view.mainDesc                "Forget the last:">
> +<!ENTITY panicButton.view.5min                    "Five minutes">
> +<!ENTITY panicButton.view.2hr                     "Two hours">
> +<!ENTITY panicButton.view.day                     "All Day">

"Forget the last: All Day" is awkward phrasing. Maybe "Forget the last: Day" is better.

We should get UX input here. The code here is stopping at the previous midnight, which is confusing to me since if someone starts browsing questionable websites at 10pm and hits this button at 1am, then there is no choice here that would satisfy their requirements. 2 hours in this case goes back to 11pm, while "All Day" goes back just one hour to midnight.

We should introduce a new amount that is 24 hours and use that for the 'Day' setting.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +1112,5 @@
> +  min-width: 280px;
> +}
> +
> +#PanelUI-panic-timeframe {
> +  padding: 10px 10px 10px 70px;

RTL support needed.

@@ +1170,5 @@
> +  font-size: 0.9em;
> +}
> +
> +.PanelUI-panic-actionlist {
> +  padding-left: 20px;

RTL support.

@@ +1217,5 @@
> +  text-align: center;
> +  text-shadow: none;
> +}
> +
> +#panic-button-success-closebutton {

This button is missing :hover and :hover:active support (:hover needed on Windows + Linux, :hover:active needed on all platforms).

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> (builds will appear at
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-2082cd4b83d9 when
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2082cd4b83d9
> finishes)

The test failures here are caused by bug 1065450. I'll need to rewrite that array comprehension because whenever that fix lands, it won't be on branches where this needs uplift...

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8494603 [details] [diff] [review]
add a privacy/forget/panic button,

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

::: browser/base/content/sanitize.js
@@ +487,5 @@
> +        // accidentally close the app by closing all the windows.
> +        let handler = Cc["@mozilla.org/browser/clh;1"].getService(Ci.nsIBrowserHandler);
> +        let defaultArgs = handler.defaultArgs;
> +        let newWindow = existingWindow.openDialog("chrome://browser/content/", "_blank",
> +                                                  "chrome,all,dialog=no,non-private", defaultArgs);

This should probably inherit the opener's "non-private"/"private" setting. When coming from a private window, this ends up opening a non-private window. Kinda weird, since no history is being remembered for private windows, but maybe this feature will be used to quickly restart a session without having to close down windows and reopen.

If permanent private browsing is enabled, this will still open a private window, so no bug there.

Comment 19

[:Gijs (he/him)]

Attachment: Interdiff

New patch in a sec with this interdiff applied, to do the following:

(In reply to Justin Dolske [:Dolske] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #11)
> 
> > 1) what exactly do we want this dialog to clear?
> 
> Hmm, I think we settled on the same as CRH's defaults -- everything except
> "Offline Data" and "Site Preferences". Gavin, Philipp, and I talked though
> this a couple times, generally around the issue that some of these don't
> actually clear by timespan (bug 771630) and the tradeoff between clearing
> too much vs not enough.

This should now match the implementation (minus passwords which CRH also doesn't support, it seems).

> > 2) the mockups have different sizes of the red icon thingy in the header of
> > the subview when it is in the panel vs. when it's on the toolbar. The
> > alignment of the other items in the top portion is different, too. Is this
> > really important or can we just leave it the same?
> 
> Steven's call, but I assume both are intentional to keep the subview narrow.

*** IMPORTANT ***
Not done this yet because I realized there's no appropriately sized asset for this.
*** IMPORTANT ***


> * Seems like the red Forget button should have a mousedown state? Especially
> since there can be a delay between clicking and getting to a new empty
> window, during which it's unclear if something is happening.

Fixed.

> * The "Your recent history is cleared!" arrowpanel should dismiss itself
> when clicking elsewhere. Currently it persists until you click "Thanks!".

Fixed.

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (builds will appear at
> > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> > [e-mail]-2082cd4b83d9 when
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2082cd4b83d9
> > finishes)
> 
> The test failures here are caused by bug 1065450. I'll need to rewrite that
> array comprehension because whenever that fix lands, it won't be on branches
> where this needs uplift...

Fixed.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> The anchor for this panel (the toolbarbutton) is different than the anchor
> for the Forget panel (the toolbarbutton's icon).

Should be fixed. I find it hard to see this difference on Windows, but YMMV.

> ::: browser/components/customizableui/CustomizableWidgets.jsm
> return this._Sanitizer.getClearRange(+group.value);

Done.

> ::: browser/components/customizableui/content/panelUI.inc.xul
> nit, move the /> up to the end of the previous line.

Fixed.

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> > +<!ENTITY panicButton.view.day                     "All Day">
> 
> "Forget the last: All Day" is awkward phrasing. Maybe "Forget the last: Day"
> is better.
> 
> We should get UX input here. The code here is stopping at the previous
> midnight, which is confusing to me since if someone starts browsing
> questionable websites at 10pm and hits this button at 1am, then there is no
> choice here that would satisfy their requirements. 2 hours in this case goes
> back to 11pm, while "All Day" goes back just one hour to midnight.
> 
> We should introduce a new amount that is 24 hours and use that for the 'Day'
> setting.

I'm assuming UX goes along with this interpretation and have implemented this. Shouldn't be hard to change, but really needs ux/ui-review.

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> > +#PanelUI-panic-timeframe {
> > +  padding: 10px 10px 10px 70px;
> 
> RTL support needed.

Fixed.

> @@ +1170,5 @@
> > +.PanelUI-panic-actionlist {
> > +  padding-left: 20px;
> 
> RTL support.

Fixed.
 
> @@ +1217,5 @@
> > +  text-align: center;
> > +  text-shadow: none;
> > +}
> > +
> > +#panic-button-success-closebutton {
> 
> This button is missing :hover and :hover:active support (:hover needed on
> Windows + Linux, :hover:active needed on all platforms).

Fixed for all platforms - we provide hover feedback on mac inside panels, AFAICT - just not for toplevel toolbarbuttons.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Comment on attachment 8494603 [details] [diff] [review]
> add a privacy/forget/panic button,
> 
> Review of attachment 8494603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/sanitize.js
> @@ +487,5 @@
> > +        // accidentally close the app by closing all the windows.
> > +        let handler = Cc["@mozilla.org/browser/clh;1"].getService(Ci.nsIBrowserHandler);
> > +        let defaultArgs = handler.defaultArgs;
> > +        let newWindow = existingWindow.openDialog("chrome://browser/content/", "_blank",
> > +                                                  "chrome,all,dialog=no,non-private", defaultArgs);
> 
> This should probably inherit the opener's "non-private"/"private" setting.
> When coming from a private window, this ends up opening a non-private
> window. Kinda weird, since no history is being remembered for private
> windows, but maybe this feature will be used to quickly restart a session
> without having to close down windows and reopen.
> 
> If permanent private browsing is enabled, this will still open a private
> window, so no bug there.

Also fixed, I believe.

I also removed some leftover debugging code, fixed the scrollbar showing in the panel on Windows, fixed the clear button not appearing on Windows, a focus issue Jared found on Windows 7, making the button in the subview a <button> instead of a <toolbarbutton>, the radio button styles on Windows, and the 'open' style of the button on non-mac (wrong rect offset into the sprite).

This still needs testing on Linux, and needs the issues 2 & 3 from comment 11 and the sprite issue from comment 13/14 addressed.

Comment 20

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

Comment 21

[:Gijs (he/him)]

Renewed try push:


Alternatively, view them on Treeherder (experimental):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d15a25ed4058

Once completed, builds and logs will be available at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d15a25ed4058

Comment 22

[Justin Dolske [:Dolske]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)

> This should probably inherit the opener's "non-private"/"private" setting.
> When coming from a private window, this ends up opening a non-private
> window. Kinda weird, since no history is being remembered for private
> windows, but maybe this feature will be used to quickly restart a session
> without having to close down windows and reopen.

Hmm. I'm not sure we really thought about what should happen when using this from a Private Window. It's kind of a weird case -- you could sorta just close the window and not explicitly clear anything, but what if the time frame extends earlier, or you have a non-private window open too? Bleh. Could disable the button or show a "Hey, you don't need to use this from a Private window!" info string, but that's also weird and the same time frame / extra window issue is there. Double bleh.

My first thought was that it should open a normal/default window, just like if you had quit and restarted. But I could see it being a little unexpected to change from Private to Normal. Matt also noted that as an actual "panic" button it could seem suspicious to end up with a private window. But if you're worried about that, a blank new window could be suspicious in and of itself.

So... Doesn't seem like either approach is obviously right or wrong, and we can always change our mind in a followup or based on user feedback. Perhaps Steven/Philipp have further thoughts.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8494840 [details] [diff] [review]
add a privacy/forget/panic button,

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY panicButton.view.mainLabel               "Proceding will:">

Typo: "proceeding"

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Justin Dolske [:Dolske] from comment #15)
> > 1) what exactly do we want this dialog to clear?
> 
> Hmm, I think we settled on the same as CRH's defaults -- everything except
> "Offline Data" and "Site Preferences". Gavin, Philipp, and I talked though
> this a couple times, generally around the issue that some of these don't
> actually clear by timespan (bug 771630) and the tradeoff between clearing
> too much vs not enough.

In sanitize.js terms, I think we should clear:
cache, cookies, history, formdata, downloads, sessions

As far as I can tell these all properly support time-based clearing (except for cache/sessions, which isn't a big deal, and some other minor exceptions e.g. the formdata case where it always clears all searchbar undo history). 

I was a bit on the fence about "offlineApps". We can only clear it completely, which is sub-optimal, and CRH doesn't clear it by default, so let's omit it.

(In reply to Justin Dolske [:Dolske] from comment #22)
> Hmm. I'm not sure we really thought about what should happen when using this
> from a Private Window.

I think disabling the button in private windows would be fine.

Comment 25

[Francesco Lodolo [:flod]]

Comment on attachment 8494840 [details] [diff] [review]
add a privacy/forget/panic button,

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +780,5 @@
> +<!ENTITY panicButton.view.mainLabel               "Proceding will:">
> +<!ENTITY panicButton.view.deleteCookies           "Delete Recent <html:strong>Cookies</html:strong>">
> +<!ENTITY panicButton.view.deleteHistory           "Delete Recent <html:strong>History</html:strong>">
> +<!ENTITY panicButton.view.deleteTabsAndWindows    "Close all <html:strong>Tabs</html:strong> and <html:strong>Windows</html:strong>">
> +<!ENTITY panicButton.view.openNewWindow           "Open a new clean Window">

I think this should use the same case of other items ("Open a New Clean Window").

Comment 26

[Francesco Lodolo [:flod]]

Sorry, I commented in bug 1064524 without realizing there was an implementation bug already open. Same question though: how does the design cope with very long text for the 3 options, and the last 4 explanations? I'm particularly worried about the panel embedded in the Australis menu.

Comment 27

[:Gijs (he/him)]

See also bug 1064524 comment #12 and bug 1064524 comment #13. I'd really like to keep the discussion here if possible.

I have no opinion on the capitalization and/or "Day" and/or "forget the last:" sentence-formulation bikeshedding - that's something for UX to resolve, as far as I'm concerned, but I would really like that to happen soon so the patch can be finished off.

(In reply to Francesco Lodolo [:flod] from comment #26)
> Sorry, I commented in bug 1064524 without realizing there was an
> implementation bug already open. Same question though: how does the design
> cope with very long text for the 3 options, and the last 4 explanations? I'm
> particularly worried about the panel embedded in the Australis menu.

The labels wrap and the radio button and action list imagery centers inside the box (good!), but cause a scrollbar when displayed as a standalone panel because XUL's height calculation code somehow doesn't get things right, for whatever reason (bad!).

I worked around the XUL calculation stuff by specifying a height for the one-line case, which fixes the one-line case, but doesn't avoid the scrollbar if the labels start wrapping. I can try to find an alternative solution for this but am not sure to what extent that's feasible.

Comment 28

[:Gijs (he/him)]

CC'ing Chris here so we can really keep the discussion here...

Comment 29

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch from comment #27)
> I have no opinion on the capitalization and/or "Day" and/or "forget the
> last:" sentence-formulation bikeshedding - that's something for UX to
> resolve, as far as I'm concerned, but I would really like that to happen
> soon so the patch can be finished off.
I agree.

> The labels wrap and the radio button and action list imagery centers inside
> the box (good!), but cause a scrollbar when displayed as a standalone panel
> because XUL's height calculation code somehow doesn't get things right, for
> whatever reason (bad!).
Specifying a localizable min-height in the DTD could be an option?

Comment 30

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #29)
> (In reply to :Gijs Kruitbosch from comment #27)
> > I have no opinion on the capitalization and/or "Day" and/or "forget the
> > last:" sentence-formulation bikeshedding - that's something for UX to
> > resolve, as far as I'm concerned, but I would really like that to happen
> > soon so the patch can be finished off.
> I agree.
> 
> > The labels wrap and the radio button and action list imagery centers inside
> > the box (good!), but cause a scrollbar when displayed as a standalone panel
> > because XUL's height calculation code somehow doesn't get things right, for
> > whatever reason (bad!).
> Specifying a localizable min-height in the DTD could be an option?

min-height doesn't fix this, oddly, just setting height does (and doesn't cut off stuff if the content is higher - you just get the scrollbar back)

However, I'd (a) want to just fix this (seems like we're running into bug 451997, so I'm trying the workaround there...) and (b) I'm not sure I want to rely on localizers dealing appropriately with CSS units, font sizes, and maths (I *think* it boils down to something like 1.5em * number of lines + 4px for padding, but even so).

Comment 31

[:Gijs (he/him)]

Attachment: update toolbar and menuPanel sprites,

This s/proceding/proceeding/, and fixes the height issue by using the workaround from bug 451997 (setting the computed height as a style attribute - and no, I also don't know why that's necessary and I think we should fix it properly too, but that's unlikely to happen for 33). I also just realized (note to self for when Toolbar-aero.png materializes) that the toolbar images patch doesn't include the yosemite images, which didn't have the same name as the existing one and therefore slipped through my copying routine from the zipfile...

Comment 32

[:Gijs (he/him)]

Comment on attachment 8495194 [details] [diff] [review]
update toolbar and menuPanel sprites,

Ugh, qref'd into the wrong patch. Sigh.

Comment 33

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

Now with the right patch...

Comment 34

[Stephen Horlander [:shorlander]]

(In reply to Justin Dolske [:Dolske] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #11)
> 
> > 1) what exactly do we want this dialog to clear?
> 
> Hmm, I think we settled on the same as CRH's defaults -- everything except
> "Offline Data" and "Site Preferences". Gavin, Philipp, and I talked though
> this a couple times, generally around the issue that some of these don't
> actually clear by timespan (bug 771630) and the tradeoff between clearing
> too much vs not enough.

I wasn't in those early meetings, but that aligns with my notes from Philipp.


> > 2) the mockups have different sizes of the red icon thingy in the header of
> > the subview when it is in the panel vs. when it's on the toolbar. The
> > alignment of the other items in the top portion is different, too. Is this
> > really important or can we just leave it the same?
> 
> Steven's call, but I assume both are intentional to keep the subview narrow.

Indeed, it is intentional because of the narrower area. I think I left the 32 x 32 icon out of the asset. Will attach that here.


> > 3) the radio button styles - see comment #8. I think what I've implemented
> > is what the spec means, but I'm not 100% sure. Can I get a basic ui-review?
> > :-)
> 
> Seems ok to me, although the bold->normal transition is a little weird
> because the text size (width) changes... It grabs my eye away from the item
> that is actually being clicked. To fix we'd have to either get rid of the
> style change, or make it only a color change (black = selected, 90% opaque =
> unselected). Again, Steven's call.

I agree, the normal to bold is odd in usage. How about non-bold, but could we get the depressed selected area to be persistent? http://cl.ly/image/1k2o120o3U03

Styling looks good to me, except the blue selected dot still seems oddly jaggy.


> Two other nits:
> 
> * Seems like the red Forget button should have a mousedown state? Especially
> since there can be a delay between clicking and getting to a new empty
> window, during which it's unclear if something is happening.

Yeah, needs a hover and active state. Let me work out what that should be :)
 

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> > Comment on attachment 8494602 [details] [diff] [review]
> > update toolbar and menuPanel sprites,
> > 
> > Review of attachment 8494602 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This doesn't update browser/themes/window/Toolbar-aero.png
> 
> Ugh, that doesn't seem to be in attachment 8494206 [details]. Stephen?

Oops. Too many assets, will attach here.

Two other nits:

- The large icon should be top aligned with the text: http://cl.ly/image/0x1X3V0j051F
- "Thanks!" button is two pixels too tall: http://cl.ly/image/03173Z3d1M3K
-- Looks like from the border

Comment 35

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #34)
> > > 3) the radio button styles - see comment #8. I think what I've implemented
> > > is what the spec means, but I'm not 100% sure. Can I get a basic ui-review?
> > > :-)
> > 
> > Seems ok to me, although the bold->normal transition is a little weird
> > because the text size (width) changes... It grabs my eye away from the item
> > that is actually being clicked. To fix we'd have to either get rid of the
> > style change, or make it only a color change (black = selected, 90% opaque =
> > unselected). Again, Steven's call.
> 
> I agree, the normal to bold is odd in usage. How about non-bold, but could
> we get the depressed selected area to be persistent?
> http://cl.ly/image/1k2o120o3U03

As I noted earlier in this bug, that'd mean that the hover and depressed state would be completely adjacent, which looks odd, because the design leaves (AFAICT) no margin between the items otherwise, in an effort to get nice spacing between the radio items... Am I missing something?

> Styling looks good to me, except the blue selected dot still seems oddly
> jaggy.

I'm reusing bits of the in-content styles which have layered radii with a 0.5px difference. I'm not sure why that is, but could get rid of it. Jared, any idea what that's about?

Comment 36

[Axel Hecht [:Pike]]

Should some of the visuals be flipped for RTL? Honest question, I don't know what we're doing on the desktop UI usually.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495199 [details] [diff] [review]
add a privacy/forget/panic button,

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

Close, but r- for the elements-within-entities and the RTL arrow. Also still need an updated patch that has the aero icons.

::: browser/base/content/browser.js
@@ +7369,5 @@
> +      window.PanicButtonNotifierShouldNotify = true;
> +      return;
> +    }
> +    // Display notification panel here...
> +    try { 

nit, whitespace

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +990,5 @@
> +      let variableHeightItems = view.querySelectorAll("radio, label");
> +      let win = aContainer.ownerDocument.defaultView;
> +      for (let item of variableHeightItems) {
> +        if (aSetHeights) {
> +          item.style.height = win.getComputedStyle(item, null).getPropertyValue("height");

This is just... yeah... we should get bug 451997 fixed. In saner worlds, this should be basically a no-op (with lingering side effects).

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +775,5 @@
>  
> +<!ENTITY panicButton.view.mainDesc                "Forget the last:">
> +<!ENTITY panicButton.view.5min                    "Five minutes">
> +<!ENTITY panicButton.view.2hr                     "Two hours">
> +<!ENTITY panicButton.view.day                     "Day">

Since this was switched to 24 hours, I think we should change the wording here to "24 hours" to make it more clear

@@ +779,5 @@
> +<!ENTITY panicButton.view.day                     "Day">
> +<!ENTITY panicButton.view.mainLabel               "Proceeding will:">
> +<!ENTITY panicButton.view.deleteCookies           "Delete Recent <html:strong>Cookies</html:strong>">
> +<!ENTITY panicButton.view.deleteHistory           "Delete Recent <html:strong>History</html:strong>">
> +<!ENTITY panicButton.view.deleteTabsAndWindows    "Close all <html:strong>Tabs</html:strong> and <html:strong>Windows</html:strong>">

I couldn't find any embedded elements within string definitions when searching MXR. We should probably build these strings using stringbundle.getFormattedString.
panicButton.view.cookies=Cookies
panicButton.view.deleteCookies=Delete Recent $1
And then use getFormattedString to place the <html:strong> and </html:strong> around $1 when building the string.

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +70,5 @@
>      background-image: url("chrome://global/skin/menu/[e-mail]");
>    }
>  
> +  #PanelUI-panic-timeframe {
> +    background-image: url(chrome://browser/skin/panic-panel/[e-mail]);

This image needs to be flipped horizontally when in RTL, since the arrow head should always be pointing in the same direction as the Back button (not mirroring the Reload button).

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +1163,5 @@
> +.subviewradio > .radio-check[selected] {
> +  background-image: radial-gradient(circle, rgb(23,146,229),
> +                    rgb(76,177,255) 4px, rgba(76,177,255,0.2) 4.5px,
> +                    transparent 4.5px),
> +                    linear-gradient(rgb(255,255,255), rgba(255,255,255,0.8));

As for the jaggeds, I had always assumed that it was a rendering bug with Gecko that small circles had lots of jaggeds. If we could get this to look smooth that would be awesome! But also fine to move to a follow-up bug.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Axel Hecht [:Pike] from comment #36)
> Should some of the visuals be flipped for RTL? Honest question, I don't know
> what we're doing on the desktop UI usually.

Yes, directional UI should be flipped. That is mainly things like arrows. We don't normally flip other visuals. For example, the Bookmarks button's menu-button has an asymmetrical icon which does not flip in RTL mode. This is most likely a bug, but not something that we have paid extra attention to historically.

Comment 39

[Stephen Horlander [:shorlander]]

Attachment: more-forget-assets.zip

Attaching the missing assets.

(In reply to :Gijs Kruitbosch from comment #35)
> (In reply to Stephen Horlander [:shorlander] from comment #34)
> > > > 3) the radio button styles - see comment #8. I think what I've implemented
> > > > is what the spec means, but I'm not 100% sure. Can I get a basic ui-review?
> > > > :-)
> > > 
> > > Seems ok to me, although the bold->normal transition is a little weird
> > > because the text size (width) changes... It grabs my eye away from the item
> > > that is actually being clicked. To fix we'd have to either get rid of the
> > > style change, or make it only a color change (black = selected, 90% opaque =
> > > unselected). Again, Steven's call.
> > 
> > I agree, the normal to bold is odd in usage. How about non-bold, but could
> > we get the depressed selected area to be persistent?
> > http://cl.ly/image/1k2o120o3U03
> 
> As I noted earlier in this bug, that'd mean that the hover and depressed
> state would be completely adjacent, which looks odd, because the design
> leaves (AFAICT) no margin between the items otherwise, in an effort to get
> nice spacing between the radio items... Am I missing something?

We could add 2px to the bottom margin: http://cl.ly/image/2j1D0b353o3P


> > Styling looks good to me, except the blue selected dot still seems oddly
> > jaggy.
> 
> I'm reusing bits of the in-content styles which have layered radii with a
> 0.5px difference. I'm not sure why that is, but could get rid of it. Jared,
> any idea what that's about?

Oh, I see, radial-gradient doesn't appear to render well in that situation for some reason. Might be better off using an image.



Styles for the Forget button states:

#PanelUI-panic-view-button:hover {
  background-color: #bf1f13;
  border-color: #b81d12;
}

#PanelUI-panic-view-button:hover:active {
  background-color: #99180f;
  border-color: #91170f;
}

Comment 40

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> @@ +779,5 @@
> > +<!ENTITY panicButton.view.day                     "Day">
> > +<!ENTITY panicButton.view.mainLabel               "Proceeding will:">
> > +<!ENTITY panicButton.view.deleteCookies           "Delete Recent <html:strong>Cookies</html:strong>">
> > +<!ENTITY panicButton.view.deleteHistory           "Delete Recent <html:strong>History</html:strong>">
> > +<!ENTITY panicButton.view.deleteTabsAndWindows    "Close all <html:strong>Tabs</html:strong> and <html:strong>Windows</html:strong>">
> 
> I couldn't find any embedded elements within string definitions when
> searching MXR. We should probably build these strings using
> stringbundle.getFormattedString.
> panicButton.view.cookies=Cookies
> panicButton.view.deleteCookies=Delete Recent $1
> And then use getFormattedString to place the <html:strong> and
> </html:strong> around $1 when building the string.

MXR sucks at doing regexes with no ascii in them. Here's some random examples of other cases where we do this:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#686
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/app-manager.dtd#97
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/aboutHome.dtd#25

etc. So I'd like to keep this, please. I'll add l10n notes.

Comment 41

[:Gijs (he/him)]

(found with ack-grep, but:

find browser -iname '*.dtd' | xargs grep '<[^!]'

will quickly find you all the cases in browser/ )

Comment 42

[:Gijs (he/him)]

Attachment: update toolbar and menuPanel sprites,

Incorporates yosemite and aero assets this time.

Comment 43

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #34)
> - "Thanks!" button is two pixels too tall: http://cl.ly/image/03173Z3d1M3K
> -- Looks like from the border

Which build did you use for this, and on what OS (looks like OS X hidpi?)? I can't reproduce this myself :-\


I'll put up a new patch + try builds in a few minutes, which should address everything else comments #34-39 mentioned (except as addressed in comment #40 re: strings). I'll also put up a strings patch for uplift.

Comment 44

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

This also adds the icon to the thanks panel, and fixes the layout when opening as a subview by using image elements everywhere - thanks to Blair for that idea, without which this patch probably wouldn't be done yet...

Comment 45

[:Gijs (he/him)]

Attachment: add a privacy/forget/panic button,

Now with pref to enable/disable.

Comment 46

[:Gijs (he/him)]

Attachment: strings for panic/privacy/forget-button for beta,

Strings for beta. Stephen and Jared, can you doublecheck that this is what we want? I've changed to '24 hours' for what started out as 'All day', and I made up the tooltiptext all by myself, so yeah. See also the capitalization concerns in comment #25 (there was some IRC discussion where several of us said we think the current version makes sense, but I'd like to make extra sure we're OK with what we're landing on beta, because of the timing constraints, so fixing this later isn't really an option).

Comment 47

[Stephen Horlander [:shorlander]]

Comment on attachment 8495972 [details] [diff] [review]
strings for panic/privacy/forget-button for beta,

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

Looks good to me. Thanks!

Comment 48

[:Gijs (he/him)]

Comment on attachment 8495972 [details] [diff] [review]
strings for panic/privacy/forget-button for beta,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: no strings for localizers to translate in time for the release plans for this feature, so no feature on the released versions of beta/aurora
[Describe test coverage new/current, TBPL]: n/a, strings only
[Risks and why]: n/a
[String/UUID change made/needed]: strings! :-)

Comment 49

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #43)
> (In reply to Stephen Horlander [:shorlander] from comment #34)
> > - "Thanks!" button is two pixels too tall: http://cl.ly/image/03173Z3d1M3K
> > -- Looks like from the border
> 
> Which build did you use for this, and on what OS (looks like OS X hidpi?)? I
> can't reproduce this myself :-\

(talked to Stephen, seems the latest patch is fine)

Comment 50

[Axel Hecht [:Pike]]

(In reply to :Gijs Kruitbosch from comment #48)
> [String/UUID change made/needed]: strings! :-)

Yes, please approve.

Comment 51

[Justin Dolske [:Dolske]]

Comment on attachment 8495972 [details] [diff] [review]
strings for panic/privacy/forget-button for beta,

This feature and rapid uplift has been previously discussed with lmandel and pike, so I'll wield my approval+ rubberstamp and mark it so.

For the record: we're landing strings on Beta ASAP, so that when we release this feature (i.e., uplift the code) in a special 33.0.x release, there will be no L10N impact. Localizers will have the remaining ~week between now and 33's last-beta-before-release to have an opportunity to localize. We're landing strings on Aurora too, even though code will follow shortly, for consistency with L10N.

Intent is to land+upift the code on Nightly/35 and Aurora/34 within the next few days. No code uplift to 33/Beta or 33.0 Release, but there will be a future uplift to a 33.0.x update (exact details currently being worked with the relevant folks).

Comment 52

[:Gijs (he/him)]

Comment on attachment 8495972 [details] [diff] [review]
strings for panic/privacy/forget-button for beta,

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1bbe4e97f87
https://hg.mozilla.org/releases/mozilla-beta/rev/16e19b9cec72

Comment 53

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495965 [details] [diff] [review]
add a privacy/forget/panic button,

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

>> +  #PanelUI-panic-timeframe {
>> +    background-image: url(chrome://browser/skin/panic-panel/[e-mail]);
>
> This image needs to be flipped horizontally when in RTL, since the arrow head should always be pointing in the same direction as the Back button (not mirroring the Reload button).

This still needs to be done for the toolbar icon and the menu panel icon.

r+ with the above fixed.

Comment 54

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Justin Dolske [:Dolske] from comment #51)
> Comment on attachment 8495972 [details] [diff] [review]
> strings for panic/privacy/forget-button for beta,
> 
> This feature and rapid uplift has been previously discussed with lmandel and
> pike, so I'll wield my approval+ rubberstamp and mark it so.

I will confirm this and thank Dolske for getting this landed quickly to support l10n activities.

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495965 [details] [diff] [review]
add a privacy/forget/panic button,

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

BrowserUITelemetry.jsm needs to have the PALETTE_ITEMS list updated.

Comment 56

[:Gijs (he/him)]

with RTL + BrowserUITelemetry addressed,

remote:   https://hg.mozilla.org/integration/fx-team/rev/2e42ec08a4c1
remote:   https://hg.mozilla.org/integration/fx-team/rev/dbd49d378c9b

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2e42ec08a4c1
https://hg.mozilla.org/mozilla-central/rev/dbd49d378c9b

Comment 58

[:Gijs (he/him)]

Attachment: Patch for Aurora (folds in bug 1074498)

Comment 59

[:Gijs (he/him)]

Comment on attachment 8497745 [details] [diff] [review]
Patch for Aurora (folds in bug 1074498)

Approval Request Comment
[Feature/regressing bug #]: this bug.
[User impact if declined]: bug 1073292 will be sad
[Describe test coverage new/current, TBPL]: nope!
[Risks and why]: there is some risk, but this comes with a pref to turn it off completely, so I'm ultimately not too worried about this.
[String/UUID change made/needed]: were landed earlier.

Basically, I think we should increase testing audience for this button to Aurora so we catch any remaining issues sooner rather than later.

Comment 60

[Justin Dolske [:Dolske]]

Comment on attachment 8497745 [details] [diff] [review]
Patch for Aurora (folds in bug 1074498)

Same rationale for a+ as comment 51.

Comment 61

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ea0c9321f1ad

Comment 62

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Bogdan will test and monitor this feature.

Comment 63

[Bogdan Maris, Desktop QA]

Did some extensive testing around this feature and all the issues we found are tracked separately and added to dependencies so I am going to mark this as verified on Aurora and Nightly and wait for the push to 33.

Comment 64

[Justin Dolske [:Dolske]]

Landed in alder: https://hg.mozilla.org/projects/alder/rev/aea178b2ec0c

Based on https://hg.mozilla.org/releases/mozilla-aurora/rev/ea0c9321f1ad from comment 61.

Also included the fix for bug 1074498, as did that Aurora landing.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/aea178b2ec0c

Comment 66

[Bogdan Maris, Desktop QA]

Tested on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.8.5 using Firefox 33.1 as well, no new issues were found so I will go ahead and mark this verified on 33.

Comment 67

[Sylvestre Ledru [:Sylvestre]]

I added this to the release notes with "Panic button added" as wording.
Don't hesitate to propose a better wording

An URL would be appreciated!

Comment 68

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Sylvestre Ledru [:sylvestre] from comment #67)
> I added this to the release notes with "Panic button added" as wording.
> Don't hesitate to propose a better wording
> 
> An URL would be appreciated!

I think the official name is »Forget Button«
Needinfo'ing Winston to confirm and provide a URL.

Comment 69

[Sylvestre Ledru [:Sylvestre]]

OK. I updated it. Thanks

Comment 70

[Winston Bowden]

Forget button.  We will be updating https://www.mozilla.org/en-US/firefox/desktop/trust/ with info about Forget.  There's also a sumo page with more info.  https://support.mozilla.org/kb/forget-button-quickly-delete-your-browsing-history.