Closed Bug 941457 Opened 6 years ago Closed 6 years ago

Defect - Overlay buttons disappear when clearing History & Other Data

Categories

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

x86_64
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: kjozwiak, Assigned: TimAbraldes)

References

Details

(Whiteboard: [beta28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=3)

Attachments

(2 files, 1 obsolete file)

Attached image 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/
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → [triage] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
[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}]
Assignee: nobody → jmathies
Whiteboard: [triage] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → [triage] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
Whiteboard: [triage] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1 → [block28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
ahhh wrong bug!
Assignee: jmathies → nobody
Whiteboard: [block28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1 → [triage] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
Whiteboard: [triage] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1 → [release28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
Whiteboard: [release28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1 → [beta28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
Assignee: nobody → tabraldes
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → [beta28] feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=3
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
Flags: needinfo?(ywang)
(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.
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.
Flags: needinfo?(ywang)
Attached patch Patch v1 (obsolete) — Splinter Review
This patch closes all open tabs when clearing browser history.
Attachment #8348962 - Flags: review?(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.
Attachment #8348962 - Flags: review?(mbrubeck) → review+
Duplicate of this bug: 877047
Attached patch Patch v2Splinter Review
(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!
Attachment #8348962 - Attachment is obsolete: true
Attachment #8349066 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f472bfe87b49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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 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.
Attachment #8349066 - Flags: approval-mozilla-aurora?
Attachment #8349066 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed for Firefox 29 based on comment 12.

Kamil, can you please verify this is fixed in Firefox 28?
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
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
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.