Simplify the way we open captive portal tabs

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 months ago
We really shouldn't bother opening a captive portal tab if we don't immediately focus it.
(Assignee)

Comment 1

6 months ago
To elaborate: it's bad UX for a seemingly random tab to be opened in the background - it's really not obvious to a user what's going on. The "Show Login Page" button in the notification bar opens it anyway if needed, so there's no need to open the tab if we don't immediately focus it.
Comment hidden (mozreview-request)
Comment on attachment 8808622 [details]
Bug 1315969 - Only open captive portal tab if we also immediately focus it.

https://reviewboard.mozilla.org/r/91424/#review91284

::: browser/locales/en-US/chrome/browser/browser.properties:738
(Diff revision 1)
>  decoder.unsupportedLibavcodec.message = libavcodec may be vulnerable or is not supported, and should be updated to play video.
>  
>  # LOCALIZATION NOTE (captivePortal.infoMessage):
>  # This string is shown in a notification bar when we detect a captive portal is blocking network access
> -# and requires the user to log in before browsing. %1$S is replaced with brandShortName.
> -captivePortal.infoMessage=This network may require you to login to use the internet. %1$S has opened the login page for you.
> +# and requires the user to log in before browsing.
> +captivePortal.infoMessage = This network may require you to login to use the internet.

You need to change the string ID if you change the contents and don't forget to update the comment to match.
Attachment #8808622 - Flags: review?(MattN+bmo) → review+
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8809543 [details]
Bug 1315969 - Update captive portal notification string to not indicate that we automatically opened the login page.

https://reviewboard.mozilla.org/r/92094/#review92232

::: browser/locales/en-US/chrome/browser/browser.properties:738
(Diff revision 1)
>  decoder.unsupportedLibavcodec.message = libavcodec may be vulnerable or is not supported, and should be updated to play video.
>  
> -# LOCALIZATION NOTE (captivePortal.infoMessage):
> +# LOCALIZATION NOTE (captivePortal.infoMessage2):
>  # This string is shown in a notification bar when we detect a captive portal is blocking network access
> -# and requires the user to log in before browsing. %1$S is replaced with brandShortName.
> -captivePortal.infoMessage=This network may require you to login to use the internet. %1$S has opened the login page for you.
> +# and requires the user to log in before browsing.
> +captivePortal.infoMessage2 = This network may require you to login to use the internet.

I'm confused about the two commits but anyways… I don't see the new ID being used in CaptivePortalWatcher.jsm
Attachment #8809543 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

6 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1fe2f1909d4
Update captive portal notification string to not indicate that we automatically opened the login page. rs=MattN
(Assignee)

Updated

6 months ago
Keywords: leave-open
Comment on attachment 8809543 [details]
Bug 1315969 - Update captive portal notification string to not indicate that we automatically opened the login page.

https://reviewboard.mozilla.org/r/92094/#review92324

This patch is now stale.
Attachment #8809543 - Flags: review?(MattN+bmo)
Comment on attachment 8809748 [details]
Bug 1315969 - Update captive portal tests to not expect background tabs.

https://reviewboard.mozilla.org/r/92272/#review92326
Attachment #8809748 - Flags: review?(MattN+bmo) → review+

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1fe2f1909d4
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8809543 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

6 months ago
Green try push! With plenty of retriggers, too.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d795d63a7aad5752e7975682e0545511c13a5ab3
Comment on attachment 8809748 [details]
Bug 1315969 - Update captive portal tests to not expect background tabs.

https://reviewboard.mozilla.org/r/92272/#review92816

::: browser/modules/CaptivePortalWatcher.jsm:26
(Diff revision 4)
> +  kPortalRecheckDelayMs: 150,
> +
> +  // This is the value used to identify the captive portal notification.
> +  kPortalNotificationValue: "captive-portal-detected",

We don't use the "k" prefix in new JS code. Use UPPERCASE_PROPERTY_NAME instead.

::: browser/modules/test/browser_CaptivePortalWatcher.js:5
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetter(this, 'CaptivePortalWatcher',
> +  'resource:///modules/CaptivePortalWatcher.jsm');

Nit: double quotes
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

5 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d6c648dd77c8
Only open captive portal tab if we also immediately focus it. r=MattN
https://hg.mozilla.org/integration/autoland/rev/246f2ae7dca7
Update captive portal tests to not expect background tabs. r=MattN

Comment 23

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6c648dd77c8
https://hg.mozilla.org/mozilla-central/rev/246f2ae7dca7
(Assignee)

Updated

5 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED

Comment 24

5 months ago
(In reply to Wes Kocher (:KWierso) from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/a1fe2f1909d4
> 
> +captivePortal.infoMessage = This network may require you to login to use the internet. %1$S has opened the login page for you.
> +captivePortal.infoMessage2 = This network may require you to login to use the internet.

Please note that "login" is a noun, "log in" is the verb. "Login Page" should be fine.
Can this be corrected?

(Lowercase "i" for "internet" seems to be allowed according to the style guide.)

Also see bug 989197.
(Assignee)

Comment 25

5 months ago
Comment on attachment 8808622 [details]
Bug 1315969 - Only open captive portal tab if we also immediately focus it.

Approval Request Comment
[Feature/Bug causing the regression]: With this patch, we don't open captive portal tabs in the background.
[User impact if declined]: Confusion about the tab automatically opened in the background. Even more confusion if the tab gets automatically closed when the captive portal is freed.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: Tested in Nightly as well as by automated tests.
[String changes made/needed]:
Attachment #8808622 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

5 months ago
Comment on attachment 8809748 [details]
Bug 1315969 - Update captive portal tests to not expect background tabs.

Approval Request Comment
See comment 25. This patch updates the tests.
Attachment #8809748 - Flags: approval-mozilla-aurora?
Comment on attachment 8808622 [details]
Bug 1315969 - Only open captive portal tab if we also immediately focus it.

captive portal update, for aurora52
Attachment #8808622 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8809748 [details]
Bug 1315969 - Update captive portal tests to not expect background tabs.

test update for captive portal, for aurora52
Attachment #8809748 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf7877a40fe6
https://hg.mozilla.org/releases/mozilla-aurora/rev/392d293b5169
status-firefox52: affected → fixed

Comment 30

5 months ago
(In reply to Ton from comment #24)
> 
> Please note that "login" is a noun, "log in" is the verb. "Login Page"
> should be fine.
> Can this be corrected?

Please do not ignore but fix this prior to release.
(Assignee)

Updated

5 months ago
Depends on: 1322201
You need to log in before you can comment on or make changes to this bug.