Closed
Bug 1315969
Opened 8 years ago
Closed 8 years ago
Simplify the way we open captive portal tabs
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: nhnt11, Assigned: nhnt11)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxprivacy])
Attachments
(2 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
MattN
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details |
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.
Assignee | ||
Comment 1•8 years 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 3•8 years ago
|
||
mozreview-review |
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+
Updated•8 years ago
|
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
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•8 years 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•8 years ago
|
Keywords: leave-open
Comment 11•8 years ago
|
||
mozreview-review |
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 12•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
bugherder |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8809543 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•8 years ago
|
||
Green try push! With plenty of retriggers, too.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d795d63a7aad5752e7975682e0545511c13a5ab3
Comment 19•8 years ago
|
||
mozreview-review |
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•8 years 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•8 years ago
|
||
bugherder |
Assignee | ||
Updated•8 years ago
|
Comment 24•8 years 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•8 years 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•8 years 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 27•8 years ago
|
||
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 28•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
Comment 30•8 years 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•