989194 - Add captive portal notification bar to all tabs when detected

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P3)

Description

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Login prompt with a status bar (designed by vtsatskin)

Note: Implementation followup from design Bug 878566
Spec: https://github.com/vtsatskin/FX-Captive-Portals-Design/blob/master/design/spec.md

As part of Firefox's Captive Portal Design, this bug displays a drop-down status bar on all windows and all tabs when a captive portal is detected.  The bar includes a link to the captive portal.  If the user dismisses this bar, it is removed from all open windows and tabs.

Acceptance criteria:

- Drop-down notification displays on all windows and tabs when captive portal is detected
- Drop-down notification contains link to captive portal
- Drop-down notification, when dismissed, is dismissed globally for all windows and tabs and does not reappear unless another captive portal is detected
- If the captive portal is logged into, the drop-down notification disappears from all tabs

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Here is a useful logic diagram for the user interaction covered by this bug:

https://bug1202680.bmoattachments.org/attachment.cgi?id=8658485

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP

After much experimentation, I've arrived at the following compromise between UX/implementation complexity:

- When a portal is detected, the tab is still opened automatically (and maybe focused) as already discussed in bug 989193.

- With this patch, a notification is added to the global notificationbox of the *same window as the captive portal tab* (which is the most recently active one), saying that the network might require you to log in and that Firefox has opened the page for you.

- When a tab that is not the captive portal tab is selected, a button is shown in the notification. When clicked, it selects the captive portal tab, opening one if necessary - e.g. if the user has closed it already by mistake or otherwise.

Adding a notification to all windows is rather complex and obfuscates the code unnecessarily (in my opinion). In the rare case that the user ignores the notification that just popped up in their face and opens a new window and attempts to browse, either they will try to access a http site and be redirected to the login page, or they will encounter a cert error. At that point, work in a future bug will cause them to see a "Network Portal Interference" page which will have a link to open the login page. This will likely be *separate* from anything CaptivePortalWatcher does. I think CaptivePortalWatcher should aim to optimize the experience in the *most recent* window only. If we really want, IMO we can extend it to work with multiple windows in a future bug. FWIW, if the user logs in via a browser client that is not managed by CaptivePortalWatcher (including a tab in a new Firefox window), Watcher will still pick it up and maintain its state correctly.

I'm still working on tests for this patch, and it has a few rough edges that need smoothing out. I'll upload more patches soon.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

I think this is complete, requesting review. Updated tests will come in a separate patch. I'm fixing bug 1279491 first, I think the patch in bug 1282611 should help with the issue detecting focus. Try push will reveal if I'm right.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8766600 [details] [diff] [review]
Patch

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

Here are some preliminary comments

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +761,5 @@
> +captivePortal.infoMessage=This network may require you to login to use the internet. %1$S has opened the login page for you.
> +
> +# captivePortal.openLoginPage is the label for a button in the info bar in all tabs
> +# except the login page tab, which shows the portal login page tab when clicked.
> +captivePortal.showLoginPage=Show Login Page

There is an ID mismatch and this doesn't follow the comment convention for properties files.

::: browser/modules/CaptivePortalWatcher.jsm
@@ +31,5 @@
>    // don't leak it if the user closes it.
>    _captivePortalTab: null,
>  
> +  // This holds a weak reference to the captive portal notification.
> +  _captivePortalNotification: null,

I'm not convinced you really need this when you can look it up again by ID.

I suspect your reliance on this is part of why you are only implementing this for one window. Really this per-window code should be in a browser-captivePortal.js file which runs in each window and listens for messages to open/close the notification bar. I'm okay with doing only the selected window if it's going to be easy to "fix" later though.

@@ +225,5 @@
> +      return;
> +    }
> +
> +    let doc = tab.ownerDocument;
> +    let button = doc.querySelector("notification[value=captive-portal-detected] button.notification-button");

Does it work to do n.querySelector("button.notification-button") instead? No point in using a descendant selector on the whole document when we already have the root of the bar.

@@ +227,5 @@
> +
> +    let doc = tab.ownerDocument;
> +    let button = doc.querySelector("notification[value=captive-portal-detected] button.notification-button");
> +    if (doc.defaultView.gBrowser.selectedTab == tab) {
> +      button.style.display = "none";

Use .hidden = true

@@ +229,5 @@
> +    let button = doc.querySelector("notification[value=captive-portal-detected] button.notification-button");
> +    if (doc.defaultView.gBrowser.selectedTab == tab) {
> +      button.style.display = "none";
> +    }
> +    else {

cuddle the else

@@ +230,5 @@
> +    if (doc.defaultView.gBrowser.selectedTab == tab) {
> +      button.style.display = "none";
> +    }
> +    else {
> +      button.style.display = "block";

.hidden = false

@@ +247,5 @@
> +          }
> +
> +          // If the tab is gone or going, we need to open a new one.
> +          if (!tab || tab.closing || !tab.parentNode) {
> +            tab = this._addCaptivePortalTab(win);

_addCaptivePortalTab only opens a tab if one isn't already open but what about the case where the login tab has navigated to an unrelated page (i.e. not the login page)? It may be safer to have it just open a new, possibly duplicate, tab?

Idea: perhaps after an origin change, the portal tab is no longer considered "the" portal tab.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

It seems this bug has stalled.
Nihanth, do you have time to finish up the patch, or should Dale try to get it landed?

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Hi Dale, Nihanth hasn't been very responsive lately.
Do you think you could push this bug across the finish line?

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

I'm back. I'll be working on this patch this week.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Matthew N. [:MattN] from comment #4)
> Comment on attachment 8766600 [details] [diff] [review]
> Patch

> ::: browser/modules/CaptivePortalWatcher.jsm
> @@ +31,5 @@
> >    // don't leak it if the user closes it.
> >    _captivePortalTab: null,
> >  
> > +  // This holds a weak reference to the captive portal notification.
> > +  _captivePortalNotification: null,
> 
> I'm not convinced you really need this when you can look it up again by ID.
> 
> I suspect your reliance on this is part of why you are only implementing
> this for one window. Really this per-window code should be in a
> browser-captivePortal.js file which runs in each window and listens for
> messages to open/close the notification bar. I'm okay with doing only the
> selected window if it's going to be easy to "fix" later though.

"Fixing" this will involve mainly just addition, so this should be fine for now. I'll file a follow-up.

> @@ +227,5 @@
> > +
> > +    let doc = tab.ownerDocument;
> > +    let button = doc.querySelector("notification[value=captive-portal-detected] button.notification-button");
> > +    if (doc.defaultView.gBrowser.selectedTab == tab) {
> > +      button.style.display = "none";
> 
> Use .hidden = true

The button increases the notification bar's height slightly which is very visible when switching back and forth between a non-portal tab and the portal tab. The patch now sets visibility to hidden/visible, which keeps the bar's height constant.

> @@ +247,5 @@
> > +          }
> > +
> > +          // If the tab is gone or going, we need to open a new one.
> > +          if (!tab || tab.closing || !tab.parentNode) {
> > +            tab = this._addCaptivePortalTab(win);
> 
> _addCaptivePortalTab only opens a tab if one isn't already open but what
> about the case where the login tab has navigated to an unrelated page (i.e.
> not the login page)? It may be safer to have it just open a new, possibly
> duplicate, tab?
> 
> Idea: perhaps after an origin change, the portal tab is no longer considered
> "the" portal tab.

Hmm, having thought about it, I think an origin change would work. We'll need to detect that the tab has fully loaded, and then monitor for an origin change and forget that it's a portal tab when we detect one.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

Addressed review comments. Meant to respond to your comments in the patch comment but ended up submitting that comment before this one by mistake. Oh well.

Comment 10

[Matthew N. [:MattN]]

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

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

This still needs test to land which I understand are coming in a new patch.

::: browser/modules/CaptivePortalWatcher.jsm
@@ +118,5 @@
> +    // If the tab is gone or going, we need to open a new one.
> +    if (!tab || tab.closing || !tab.parentNode) {
> +      tab = win.gBrowser.addTab(this.canonicalURL);
> +    }
> +    

Nit: whitespace

@@ +207,5 @@
> +
> +  get _productName() {
> +    delete this._productName;
> +    return (this._productName =
> +      Services.strings.createBundle("chrome://branding/locale/brand.properties")

Nit: remove the outer braces which makes it harder to read IMO

@@ +214,5 @@
> +
> +  get _browserBundle() {
> +    delete this._browserBundle;
> +    return (this._browserBundle =
> +      Services.strings.createBundle("chrome://browser/locale/browser.properties"));

Ditto

@@ +233,5 @@
> +    let button = n.querySelector("button.notification-button");
> +    if (doc.defaultView.gBrowser.selectedTab == tab) {
> +      button.style.visibility = "hidden";
> +    } else {
> +      button.style.visibility = "visible";

OK, please file the follow-up bug to get the default height of the bar or button changed such that adding a button doesn't increase the height. This should just be a temporary workaround as it means that an existing visible bar will still grow in height if a portal gets detected.

@@ +241,5 @@
> +  _showNotification(win) {
> +    let buttons = [
> +      {
> +        label: this._browserBundle.GetStringFromName(
> +          "captivePortal.showLoginPage"),

Nit: This wrapping isn't necessary

@@ +253,5 @@
> +      },
> +    ];
> +
> +    let message = this._browserBundle.formatStringFromName(
> +      "captivePortal.infoMessage", [this._productName], 1);

Nit: can the first argument fit on the previous line < 100 chars.? We normally align the args after `(`

@@ +260,5 @@
> +      if (aEventName != "removed") {
> +        return;
> +      }
> +      win.gBrowser.tabContainer.removeEventListener("TabSelect", this);
> +    }.bind(this);

Nit: You could just use an arrow function like you do in other places?
let closeHandler = (aEventName) => {…

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 989194 - Show captive portal notification bar when detected.

* * *
tests

Review commit: https://reviewboard.mozilla.org/r/87572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87572/

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8803257 [details]
Bug 989194 - Show captive portal notification bar when detected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87572/diff/1-2/

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v3

Uploading a patch file in addition to the MozReview request for the sake of having an interdiff.

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Okay, I rebased the patch and now interdiff'ing fails on the two patches. Even when I tried to interdiff locally. Sorry :(

FWIW, aside from the tests the changes are just addressing your comments.

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8803257 [details]
Bug 989194 - Show captive portal notification bar when detected.

https://reviewboard.mozilla.org/r/87572/#review86886

This code generally is crap at handling multiple windows, for example when a portal tab is moved between two windows. I've noted this in the etherpad (https://public.etherpad-mozilla.org/p/captive-portal-roadmap). This will be fixed in a future bug (to be filed) that uses a per-browser script as suggested in a previous review.

::: browser/modules/test/browser_CaptivePortalWatcher.js:37
(Diff revision 2)
> +    BrowserTestUtils.waitForGlobalNotificationBar(win, PORTAL_NOTIFICATION_VALUE),
> +    BrowserTestUtils.waitForNewTab(win.gBrowser, CANONICAL_URL)
> +  ]);
> -  is(win.gBrowser.selectedTab, tab,
> + is(win.gBrowser.selectedTab, tab,
> -    "The captive portal tab should be open and selected in the new window.");
> +   "The captive portal tab should be open and selected in the new window.");
> +  let showLoginPageButton = notification.querySelector("button.notification-button");

This should just use the helper function below, meh.

::: browser/modules/test/browser_CaptivePortalWatcher.js:241
(Diff revision 2)
> +   * Test the various expected behaviors of the "Show Login Page" button
> +   * in the captive portal notification. The button should be visible for
> +   * all tabs except the captive portal tab, and when clicked, should
> +   * ensure a captive portal tab is open and select it.
> +   */
> +  function* test_showLoginPageButton() {

I've tried to make this a very comprehensive test, let me know if you think it's overkill.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1220
(Diff revision 2)
>        });
>      });
>    },
>  
>    /**
> +   * Waits for a <xul:notification> with a particular value to appear

I wonder if we should just extend waitForNotificationBar above to accept a tabbrowser OR a window as the first argument, and make the second browser argument optional. The only thing different about this new function is the notificationbox it uses...

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8803257 [details]
Bug 989194 - Show captive portal notification bar when detected.

https://reviewboard.mozilla.org/r/87572/#review87114

Thanks

::: browser/modules/CaptivePortalWatcher.jsm:34
(Diff revision 2)
> +  // This holds a weak reference to the captive portal notification.
> +  _captivePortalNotification: null,

This is fine for now but as discussed before we need to have a follow-up which makes this properly handle multiple windows by moving the notification bar logic to a browser-….js script in each window.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1226
(Diff revision 2)
> +   * @param notificationValue (string)
> +   *        The "value" of the notification, which is often used as
> +   *        a unique identifier. Example: "plugin-crashed".

Change the example to not confused people as I suspect plugin-crashed doesn't use this notificationbox

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8803257 [details]
Bug 989194 - Show captive portal notification bar when detected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87572/diff/2-3/

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8803257 [details]
Bug 989194 - Show captive portal notification bar when detected.

https://reviewboard.mozilla.org/r/87572/#review88258

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1204
(Diff revisions 2 - 3)
> -    let notificationBox = tabbrowser.getNotificationBox(browser);
> -    return new Promise((resolve) => {
> -      let check = (event) => {
> +    return this.waitForNotificationInNotificationBox(
> +      tabbrowser.getNotificationBox(browser),
> +      notificationValue);

Nit: Can you use a local variable for the notification box to avoid the unusual wrapping? i.e. put the first argument after `(`

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1224
(Diff revisions 2 - 3)
>     * @return Promise
>     *        Resolves to the <xul:notification> that is being shown.
>     */
>    waitForGlobalNotificationBar(win, notificationValue) {
> -    let notificationBox =
> -      win.document.getElementById("high-priority-global-notificationbox");
> +    return this.waitForNotificationInNotificationBox(
> +      win.document.getElementById("high-priority-global-notificationbox"),

Ditto

Comment 19

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8803257 [details]
Bug 989194 - Show captive portal notification bar when detected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87572/diff/3-4/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2190ea5fc2d0
Show captive portal notification bar when detected. r=MattN

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2190ea5fc2d0

Comment 22

[Paul Silaghi, QA [:pauly]]

(In reply to Jennifer Morrow [:Boriss] (UX) from comment #0)
> - Drop-down notification displays on all windows and tabs when captive
> portal is detected
If there are multiple windows opened, the notification is displayed on all tabs for one window only.

> - Drop-down notification contains link to captive portal
Moreover, the captive portal page is opened in a new tab in the background.

> - Drop-down notification, when dismissed, is dismissed globally for all
> windows and tabs and does not reappear unless another captive portal is
> detected
The notification is displayed again if the browser is restarted.

Are these expected?
Tested on 52.0a1 (2016-11-14), OS X 10.12, local WiFi network.

Comment 23

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Paul Silaghi, QA [:pauly] from comment #22)
> (In reply to Jennifer Morrow [:Boriss] (UX) from comment #0)
> > - Drop-down notification displays on all windows and tabs when captive
> > portal is detected
> If there are multiple windows opened, the notification is displayed on all
> tabs for one window only.

This is expected.

> > - Drop-down notification contains link to captive portal
> Moreover, the captive portal page is opened in a new tab in the background.

We shouldn't be opening background tabs anymore after bug 1315969 - could you check this again and confirm?

> > - Drop-down notification, when dismissed, is dismissed globally for all
> > windows and tabs and does not reappear unless another captive portal is
> > detected
> The notification is displayed again if the browser is restarted.

This is expected.

> Tested on 52.0a1 (2016-11-14), OS X 10.12, local WiFi network.

Thanks!

Comment 24

[Matthew N. [:MattN]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #23)
> (In reply to Paul Silaghi, QA [:pauly] from comment #22)
> > (In reply to Jennifer Morrow [:Boriss] (UX) from comment #0)
> > > - Drop-down notification displays on all windows and tabs when captive
> > > portal is detected
> > If there are multiple windows opened, the notification is displayed on all
> > tabs for one window only.
> 
> This is expected.

It's expected for now but will be fixed in bug 1313568.

Comment 25

[Paul Silaghi, QA [:pauly]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #23)
> We shouldn't be opening background tabs anymore after bug 1315969 - could
> you check this again and confirm?
Confirmed it's fixed in 53.0a1 (2016-11-20).

I've noticed that the captive portal notification bar is not displayed at all on Aurora 52.0a2 (2016-11-21), while it works fine on Nightly 53.0a1 (2016-11-20). Thoughts?

Comment 26

[Matthew N. [:MattN]]

(In reply to Paul Silaghi, QA [:pauly] from comment #25)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #23)
> > We shouldn't be opening background tabs anymore after bug 1315969 - could
> > you check this again and confirm?
> Confirmed it's fixed in 53.0a1 (2016-11-20).
> 
> I've noticed that the captive portal notification bar is not displayed at
> all on Aurora 52.0a2 (2016-11-21), while it works fine on Nightly 53.0a1
> (2016-11-20). Thoughts?

It's preffed off in non-Nightly builds:
https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#586-589

You need to flip that pref (and probably restart Fx).

Comment 27

[Paul Silaghi, QA [:pauly]]

Thank you!