Closed Bug 1315969 Opened 8 years ago Closed 8 years ago

Simplify the way we open captive portal tabs

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files, 1 obsolete file)

Bug 1315969 - Only open captive portal tab if we also immediately focus it.
58 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-aurora+
Details
Bug 1315969 - Update captive portal tests to not expect background tabs.
58 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-aurora+
Details 
We really shouldn't bother opening a captive portal tab if we don't immediately focus it.
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 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 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)
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
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+
Attachment #8809543 - Attachment is obsolete: true
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
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
(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.
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?
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+
(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.
Depends on: 1322201
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: