941457 - Defect - Overlay buttons disappear when clearing History & Other Data

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: overlay buttons missing

Clearing the "History" & "Other data" from the "Options" flyout will remove both of the overlay buttons on the page that's being currently viewed.

- Attached a screenshot of a webpage with the overlay's missing after I cleared the "History" & "Other Data"

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to any website (clicked on one of the Mozilla websites that are under favorites in this instance)
3) Once the website has been loaded, slide in the Windows Charm and select "Settings -> Options"
4) Ensure that both "History" & "Other Data" are selected and select "Clear"

Current Behavior:

- Overlays being removed when clearing the "History" & "Other data" from the "Options" flyout

Expected Behavior:

- When a user clears the data, the overlays shouldn't be cleared

Found the issue using the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-11-20-06-22-58-mozilla-central/

Comment 1

[Jim Mathies [:jimm]]

[JavaScript Error: "aOwner is null" {file: "chrome://browser/content/browser-ui.js" line: 458}]
[JavaScript Error: "aOwner is null" {file: "chrome://browser/content/browser-ui.js" line: 458}]

Comment 2

[Jim Mathies [:jimm]]

ahhh wrong bug!

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Investigated and found the following cause.

Whenever it is not possible for the browser to "go back" [1], which includes the case that we remove browsing history [2], we hide both [4] of the overlay buttons [3].

Requesting input from UX. After the user clears her/his history, the browser is not able to "go back," and so the overlay buttons get hidden. Does it make sense for us to just hide the "back" button (and keep the "new tab" button visible)? Or maybe we should do something else, like close all the user's tabs when he/she clears history? That way the user will be brought back to a start page where the world makes sense again (and the overlay buttons are correctly hidden). Though that brings up another point... why do we hide the "new tab" button on the start page?

[1] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js?rev=b87726cc3c4b#661
[2] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.js?rev=1bc33fa19b24#1206
[3] https://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/browser.css?rev=9968e99bbdf6#381
[4] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul?rev=d9107edc306a#214

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)
> why do we hide the "new tab" button on the start page?

I believe the rationale is that you are already on the "new tab" page, so pressing the "new tab" button will just open the same page again and is unlikely to be what you want.  And of course, it makes the start page look cleaner.

Comment 5

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Matt covered the rationale pretty well. I think you can still add a new tab from the tab strip if another new tab is needed.

I think closing current tabs and bringing the users back to Start Page after cleaning private data is a smart suggestion.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

This patch closes all open tabs when clearing browser history.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8348962 [details] [diff] [review]
Patch v1

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

::: browser/metro/base/content/browser.js
@@ +1219,5 @@
> +
> +    // Clear last URL of the Open Web Location dialog
> +    try {
> +      Services.prefs.clearUserPref("general.open_location.last_url");
> +    } catch (e) {

I don't think we actually need this; that pref is set only by desktop Firefox code.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Comment on attachment 8348962 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8348962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/browser.js
> @@ +1219,5 @@
> > +
> > +    // Clear last URL of the Open Web Location dialog
> > +    try {
> > +      Services.prefs.clearUserPref("general.open_location.last_url");
> > +    } catch (e) {
> 
> I don't think we actually need this; that pref is set only by desktop
> Firefox code.

Removed!

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/integration/fx-team/rev/f472bfe87b49

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f472bfe87b49

Comment 12

[Kamil Jozwiak [:kjozwiak]]

Went through the following defect for iteration #20 without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-23-03-02-03-mozilla-central/

- Went through the original test case from comment #0 without any issues
- Ensured that the about:start screen doesn't display both overlay buttons (Back/New Tab)
- Ensured that the behaviour matches comment #5
- Ensured that once you clear the history through the "Options" flyout, the overlay buttons re-appear when creating new tabs
- Ensured clearing the History closes all current tabs and returns the user to the about:start window without the overlay buttons visible
- Ensured that both of the overlays are working as expected after removing the history
- Ensured both overlay buttons are still movable, sliding them up and down works correctly
- Ensured that if you only clear "Browsing History", that the passwords and other information isn't being removed
- Ensured that if you only clear "Private Data", it doesn't remove the browsing history (keeping all the current tabs opened)
- Ensured if both "Browsing History" and "Private Data" are selecting, everything is removed including all the current opened tabs
- Ensured that all of the above test cases also worked under filled view

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8349066 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 835628
User impact if declined: The "back" and "new tab" buttons that always overlay metroFx will disappear whenever a user clears her/his browsing history.
Testing completed (on m-c, etc.): This has been on m-c since 
Risk to taking this patch (and alternatives if risky): low-risk. This is a metro-only change.
String or IDL/UUID changes made by this patch: none.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f7e41fb4caf

Comment 15

[u279076]

Verified fixed for Firefox 29 based on comment 12.

Kamil, can you please verify this is fixed in Firefox 28?

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Went through the following defect without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-11-00-40-04-mozilla-aurora/

- Went through the original test case from comment #0 without any issues
- Went through the test cases added under comment #12 without any issues