Closed Bug 895921 Opened 7 years ago Closed 7 years ago

[MP] Defect - Tab bar not appearing when selecting links from flyouts

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 26

People

(Reporter: kjozwiak, Assigned: ally)

References

Details

(Whiteboard: [preview] feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3)

Attachments

(1 file, 2 obsolete files)

When selecting "Help (online)", the "Tabs App Bar" isn't appearing making it seam like a new tab hasn't been created.

Steps to reproduce the issue:

1) Open Firefox Metro
2) Swipe in the Windows Charm flyout and select "Settings"
3) Select "Help (online)" and you'll notice that the "Tabs App Bar" will not slide in indicating a new tab has been opened

Current Behavior:

- Selecting "Help (online)" will not slide in the "Tabs" app bar indicating that a new tab has been created

Expected Behavior:

- When a user selects "Help (online)" from the "Settings" flyout, the "Tabs" app bar should slide in showing the user that a new tab has been created
Selecting "Read the Nightly privacy policy online" from the "About" flyout has the same issue.
Summary: Defect - Tab bar not appearing when selecting "Help (online)" → Defect - Tab bar not appearing when selecting links from flyouts
Assignee: nobody → ally
Assignee: ally → nobody
Priority: -- → P2
Assignee: nobody → ally
the privacy policy handler calls a function that has Browser.addTab()'s aBringFront hardcoded to true..this might be more tricky than we thought in triage, but I won't know until I can get a build standing again.
(In reply to :Ally Naaktgeboren from comment #2)
> the privacy policy handler calls a function that has Browser.addTab()'s
> aBringFront hardcoded to true..this might be more tricky than we thought in
> triage, but I won't know until I can get a build standing again.

I think all that's needed is an added call to peekTabs.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/ContextUI.js#151
(In reply to :Ally Naaktgeboren from comment #2)
> the privacy policy handler calls a function that has Browser.addTab()'s
> aBringFront hardcoded to true.

I think we still want the new tab to appear in the foreground, so this should not be a problem.  (But if we did want it to appear in the background, we could just switch to calling Browser.addTab(url, false).)
Whiteboard: feature=defect status=for_sprint c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 status=for_sprint
Blocks: metrov1it12
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 status=for_sprint → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3 [preview]
Attached patch bringing in the tabstrip (obsolete) — Splinter Review
When I picked up the bug, the newtab was not foregrounded and the tabstrip was not sliding in. The newtab was foregrounded at some point (yay), and this brings the tabstrip in.

That said, I wonder whether I should refactor this code so that BrowserUI also controls the privacy policy link. (I don't think it really belongs in browser.js). It won't make a functional difference, but I think it will make a maintainability/code sanity difference. Worthwhile, or am I over rotating?
Attachment #786620 - Flags: feedback?(mbrubeck)
Comment on attachment 786620 [details] [diff] [review]
bringing in the tabstrip

I agree; onAboutPolicyClick should move out of browser.js, maybe into AboutFlyoutPanel or BrowserUI.  We should use the same constant as the argument to peekTabs in both of these calls -- maybe even move the peekTabs call inside of BrowserUI.newTab.
Attachment #786620 - Flags: feedback?(mbrubeck) → feedback+
Attached patch peektabs (obsolete) — Splinter Review
I feel better much better. :)

turns out that peekTabs() isn't called near newTab as often as I thought. I left most of the incantations of newTab() alone since the 3rd parameter defaults to false.
Attachment #786620 - Attachment is obsolete: true
Attachment #786686 - Flags: review?(mbrubeck)
Attached patch peektabsSplinter Review
Attachment #786686 - Attachment is obsolete: true
Attachment #786686 - Flags: review?(mbrubeck)
Attachment #786690 - Flags: review?(mbrubeck)
Attachment #786690 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/85c1aed7e525
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Went through the following "Defect" for iteration #12 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-08-14-03-02-04-mozilla-central/

- Went through the original test case in comment #0 without any issues (went through it 15 different times)
- Ensured all the flyouts where still working without any issues (sliding into view)
- Ensured that you can go back and forth between the different flyouts without any issues
- Ensured that all the flyouts where formatted and appear correctly
- Ensured that both links from "Help" & "About" had the same behavior (when opening into a new tab)
- Ensured that all of the above cases also worked using Filled View
Summary: Defect - Tab bar not appearing when selecting links from flyouts → [MP] Defect - Tab bar not appearing when selecting links from flyouts
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3 [preview] → [preview] feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3
Went through the following "Defect" for iteration #13 without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-08-31-03-02-24-mozilla-central/

- Went through the original test case in comment #0 without any issues
- Went through the test cases added in comment #11 without any issues
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0

Tested this on latest nightly (build ID: 20131030030201) during iteration #17 testing.
No issues encountered, marking as verified.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.