Closed Bug 1224137 Opened 9 years ago Closed 7 years ago

List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel

Categories

(Firefox :: Site Identity, defect, P5)

Product:
Firefox
Component:
Site Identity
Version:
45 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox55 --- fixed

People

(Reporter: designakt, Assigned: prathiksha, Mentored)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(4 files)

alert steal window uncheck.gif
202.16 KB, image/gif
Details
ControlCenter-alerts.png
25.84 KB, image/png
Details
warning-triangle.svg
470 bytes, image/svg+xml
Details
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
Bug 332195 introduced the option to decide if a page can steal focus with window.alert(). However, if once allowed for a page, I could not find a way to undo this allowance. I would have expected the check-box to remain, and to only uncheck it if I don't what this page to take focus any-longer.

Easily tested in the console by first setting a function for an alert
> myAlert = function(){window.alert("Test-Message")}
and then firing this alert with a delay of a few seconds
> window.setTimeout(myAlert, 4000)
and quickly changing to another tab, waiting for the alarm to fire.
I was trying to balance always cluttering alert dialogs with this checkbox, and not giving people a choice.

I think if we always showed this when we did switch tabs, it'd then be weird because we still wouldn't show it for alerts that appear while the tab was current, and it would still be problematic because people couldn't configure this until/unless there was an alert that switched tabs, with the alerts being controlled by the website. That is, I'm not convinced just showing it in the case where we did switch tabs is going to be particularly helpful in giving people more control.

I also don't think this is super important. Are we afraid people accidentally opt in? Change their mind later? Considering the opt-in behaviour is one we've lived with for years, I'm not sure how highly this should be prioritized.

Of course, the flip side is that right now there is literally no way beyond firefox reset that will remove this permission (besides custom JS in the browser console).


Really, IMO the ideal solution would be having a settings menu of some description where users can:

- turn off alerts for this domain altogether
- control the focus behaviour

that's tucked away in the corner instead of as in-your-face as a dialog checkbox is. I think that could be part of the in-content dialog redesign. I don't think it needs to be prioritized right now.

Philipp, thoughts?
Blocks: 332195
Flags: needinfo?(philipp)
The use case I see is that someone checked the box because they thought it would be useful, but later find the site annoying. If you have ever revoked notification permissions for an app on your phone, you know the feeling.

I think it makes sense to keep control inside the context here.
The only time when the resetting the setting is relevant is when you're actually annoyed by the tab switching. In this case, it would be best if you could directly revoke it in that context.

So I think the behavior that makes most sense would be:
If the user has allowed a site to switch tab on alert, display the (checked) checkbox inside the alert whenever that site switches the tab through an alert.
Flags: needinfo?(philipp)
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #2)
> The use case I see is that someone checked the box because they thought it
> would be useful, but later find the site annoying. If you have ever revoked
> notification permissions for an app on your phone, you know the feeling.

I'm on an android phone; I have no possibility of doing that, so I have no idea. :-)

> So I think the behavior that makes most sense would be:
> If the user has allowed a site to switch tab on alert, display the (checked)
> checkbox inside the alert whenever that site switches the tab through an
> alert.

OK.
(In reply to :Gijs Kruitbosch from comment #1)

> I also don't think this is super important. Are we afraid people
> accidentally opt in? Change their mind later? Considering the opt-in
> behaviour is one we've lived with for years, I'm not sure how highly this
> should be prioritized.

Fair point -- the lack of an undo is no different/worse than the behavior we've had forever.

One odd approach -- we could ship as-is, but add telemetry to this for one or two releases (opt-out, for real Release data). If we don't see significant usage of it (ie, people actually enabling focus-stealing) we could just kill it entirely. Although philipp's comment 2 implies that at least some of the people enabling this might think they need to, and not understand what they're doing?

At least from my own usage, I found that the gcal-appointment-alert case that's been cited as a need for this case is actually just annoying, and I've not felt like I need to enable it. And, TBH, I'm also a little annoyed by checkbox and text for this cluttering up that alert all the time now. ;-)
Mentor: gijskruitbosch+bugs
I would, at all costs, avoid gumming up the UI with check boxes. Instead, simply make note of the permission in the new Control Center (COMING SOON), Which is specifically designed to uplifts and centralize all types of permissions granted to the website you're looking at.
(In reply to bbell from comment #5)
> Created attachment 8738674 [details]
> ControlCenter-alerts.png
> 
> I would, at all costs, avoid gumming up the UI with check boxes. Instead,
> simply make note of the permission in the new Control Center (COMING SOON),
> Which is specifically designed to uplifts and centralize all types of
> permissions granted to the website you're looking at.

That's nice. When is "SOON" ?
Flags: needinfo?(bbell)
> That's nice. When is "SOON" ?


49?
Flags: needinfo?(bbell)
Component: Tabbed Browser → Site Identity and Permission Panels
Summary: javascript alert() - no way to undo allowing to take focus → List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel
Hi Gijs, 
I am planning to work on this soon.
Would you briefly tell, what do I need to do, on getting started. 
Thanks :D
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → pavankarthikboddeda
Whiteboard: [fxprivacy] [triage]
(In reply to Pavan Karthik [:matrixisreal] from comment #8)
> Hi Gijs, 
> I am planning to work on this soon.
> Would you briefly tell, what do I need to do, on getting started. 
> Thanks :D

You will need to add code to SitePermissions.jsm (search for this file in https://dxr.mozilla.org/ ) and a string to label the permission (pending something better: "Alerts can switch tabs") in sitePermissions.properties . You will probably also need to add an icon - check how the code in SitePermissions work to figure out how to do that.

Bryan, does that string sound OK?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bbell)
I'd like to add that the permission id you want to add is "focus-tab-by-prompt". As Gijs mentioned it's best to look around SitePermissions.jsm and see how it's done for other permissions like "geolocation".

Also feel free to hop onto IRC and ask us things in the #fx-team channel.
Mentor: jhofmann
Markus, can you provide an SVG version of the alert icon please from your screenshots? Thanks!
Flags: needinfo?(mjaritz)
Attached image warning-triangle.svg — 
That was bbells mockup, but I think you can use the warning-triangle. But make sure to match the color to the other icons (e.g. Notifications).
Flags: needinfo?(mjaritz)
Priority: -- → P5
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Having just run into this issue, I was wondering if there's anything I can contribute to getting a fix applied - whether it be an attempt at writing a patch myself, or any assistance or support that would be helpful to those working on one.

In the meantime (in case anyone else is running into this) there's a workaround mentioned in bug 332195 comment 81:

> For a workaround, run:
> 
> Services.perms.removeFromPrincipal(gBrowser.selectedBrowser.contentPrincipal, "focus-tab-by-prompt");
> 
> in the (app-privileged) browser console ( https://developer.mozilla.org/en/docs/Tools/Browser_Console ).
Yeah, we should resolve this.

:matrixisreal, I'm unassigning you for now since there hasn't been any update in over two months. :)

So this is up for grabs again.

Stuart, you could work on this if you want. I might also know a couple of other people who could be interested in solving this, so let me know if you have time.
Assignee: pavankarthikboddeda → nobody
Johann, I might not have tie to work on this right now, Sorry :/
I'd like to be assigned to solve this bug. :D
Assigned!

See comment 9 and comment 10 on how to solve this.

Make sure to read the whole bug and get a good idea of what the desired result should be (mockup-ed in attachment 8738674 [details]) and ask any questions you need to clarify.

This probably requires a lot of small changes, so please contact me or Gijs on IRC or here if you have any question or if anything is unclear while working on it.

Have fun!
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Okay. Thanks, Johann. :)
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review125464

Great job! Just a couple of minor issues to fix.

And we need to test this. Since this test isn't adding a lot of logic by itself (more a bit of configuration for existing logic), we could consider just piggy-backing onto the existing test for this feature:

I was thinking we could simply add an assert that the identity icon has the little dot and that the control center shows the correct permission to this line:

http://searchfox.org/mozilla-central/source/browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js#51

You can find the kind of check we want to do in this file:

http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_permissions.js#53

There we check camera, geolocation, etc. but it shouldn't be very hard to adapt that to "focus-tab-by-prompt".

Let me know if anything is unclear and feel free to ask questions in IRC!

Huge thanks!

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:38
(Diff revision 1)
>  permission.screen.label = Share the Screen
>  permission.install.label = Install Add-ons
>  permission.popup.label = Open Pop-up Windows
>  permission.geo.label = Access Your Location
>  permission.indexedDB.label = Maintain Offline Storage
> +permission.focus-tab-by-prompt.label = Enable Focus Stealing

Gijs' suggestion was to call this "Alerts can switch tabs", I think that's a good start but we need to rephrase it so that it fits the style of the existing permission labels ([This website can] Receive Notifications, Access Your Location).

How about "[This website can] Focus this tab"

We should ask jsavory (since she's newly in charge of UX for this stuff) to be sure.

::: browser/modules/SitePermissions.jsm:608
(Diff revision 1)
>  
> -  "indexedDB": {}
> +  "indexedDB": {},
> +
> +  "focus-tab-by-prompt": {
> +    exactHostMatch: true,
> +    states: [ SitePermissions.UNKNOWN, SitePermissions.ALLOW, SitePermissions.BLOCK ],

You can remove SitePermissions.BLOCK from the states, since I think there's no difference between BLOCK and UNKNOWN for focus-tab

::: browser/modules/SitePermissions.jsm:609
(Diff revision 1)
> -  "indexedDB": {}
> +  "indexedDB": {},
> +
> +  "focus-tab-by-prompt": {
> +    exactHostMatch: true,
> +    states: [ SitePermissions.UNKNOWN, SitePermissions.ALLOW, SitePermissions.BLOCK ],
> +  }

Nit: can you please add a trailing comma here?

::: browser/themes/shared/notification-icons.inc.css:36
(Diff revision 1)
>    fill: currentColor;
>  }
>  
>  /* INDIVIDUAL NOTIFICATIONS */
>  
>  .popup-notification-icon[popupid="web-notifications"],

This line was moved to the wrong place. It belongs in line 45 above .desktop-notification-icon.

::: browser/themes/shared/notification-icons.inc.css:42
(Diff revision 1)
> +
> +.focus-tab-by-prompt-icon {
> +  list-style-image: url(chrome://browser/skin/notification-icons.svg#focus-tab-by-prompt);
> +}
> +
> +.focus-tab-by-prompt-icon.blocked-permission-icon{

Nit: add a space before the {

::: browser/themes/shared/notification-icons.svg:50
(Diff revision 1)
>        fill-opacity: 1;
>      }
>    </style>
>  
>    <defs>
> +    <path id="focus-tab-by-prompt-icon" d="M29.43,25,18.57,3.8A2.92,2.92,0,0,0,16,2a2.92,2.92,0,0,0-2.57,1.8L2.57,25a3.47,3.47,0,0,0,0,3.4A3.15,3.15,0,0,0,5.33,30H26.66a3.15,3.15,0,0,0,2.77-1.6A3.47,3.47,0,0,0,29.43,25ZM16,7.2a2.3,2.3,0,0,1,2.37,2.4L18,18a1.88,1.88,0,0,1-2,2,1.88,1.88,0,0,1-2-2l-.4-8.4A2.3,2.3,0,0,1,16,7.2ZM16,28a3,3,0,0,1,0-6,3,3,0,0,1,0,6Z"/>

These paths are ordered alphabetically, so please insert this line two lines below :)

::: browser/themes/shared/notification-icons.svg:86
(Diff revision 1)
>  
>    <use id="camera" xlink:href="#camera-icon" />
>    <use id="camera-sharing" xlink:href="#camera-icon"/>
>    <use id="camera-indicator" xlink:href="#camera-icon" />
>    <use id="camera-blocked" class="blocked" xlink:href="#camera-icon" />
> +  <use id="focus-tab-by-prompt" xlink:href="#focus-tab-by-prompt-icon" />

Please also get this one in the right alphabetic order.
Attachment #8850406 - Flags: review?(jhofmann) → review-
Oh, I forgot to actually flag jsavory for this:

Jacqueline, can you help us find a good label for this permission? See my note in comment 20:

> Gijs' suggestion was to call this "Alerts can switch tabs", I think that's a good start but we need to rephrase it so that it
> fits the style of the existing permission labels ([This website can] Receive Notifications, Access Your Location).
> 
> How about "[This website can] Focus this tab"

So my current suggestion would be "Focus this tab" as a permission label. Does that sound good to you?

Thanks!
Flags: needinfo?(bbell) → needinfo?(jsavory)
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review125464

Okay. I'll do it. Thanks. :)
(In reply to Johann Hofmann [:johannh] from comment #20)
> Gijs' suggestion was to call this "Alerts can switch tabs", I think that's a
> good start but we need to rephrase it so that it fits the style of the
> existing permission labels ([This website can] Receive Notifications, Access
> Your Location).
> 
> How about "[This website can] Focus this tab"

My two cents: "Focus" may not be a familiar concept to non-technical users.

How about something like:
"[This website can] Switch to its Tab"
"[This website can] Activate its Tab"
"[This website can] Bring its Tab to the Front"
"[This website can] Take You to its Tab"
...
(In reply to Stuart Ballard from comment #23)
> (In reply to Johann Hofmann [:johannh] from comment #20)
> > Gijs' suggestion was to call this "Alerts can switch tabs", I think that's a
> > good start but we need to rephrase it so that it fits the style of the
> > existing permission labels ([This website can] Receive Notifications, Access
> > Your Location).
> > 
> > How about "[This website can] Focus this tab"
> 
> My two cents: "Focus" may not be a familiar concept to non-technical users.
> 
> How about something like:
> "[This website can] Switch to its Tab"
> "[This website can] Activate its Tab"
> "[This website can] Bring its Tab to the Front"
> "[This website can] Take You to its Tab"
> ...

Talking about copy, let's ask Michelle if she has an opinion on that?
Flags: needinfo?(mheubusch)
Adding in my two cents; to me the first option "[This website can] Switch to its Tab" sounds like it would make the most sense to the user. The term "Switching tabs" is pretty common from what I can tell. However, I'd love to hear what Michelle recommends as well :)
Flags: needinfo?(jsavory)
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review126752

We're getting closer! Thanks for adding a test. I know that writing Mochitests is a bit tricky the first time around, so great job figuring that out!

As mentioned on IRC, you should get commit access level 1 and push this to try when you fixed the remarks below.

Thanks!!!

::: browser/base/content/test/general/browser_permissions.js:196
(Diff revision 2)
>  
>    SitePermissions.remove(gBrowser.currentURI, "geo");
>  
>    ok(!geoIcon.hasAttribute("showing"),
>      "blocked permission icon is not shown after reset");
>  });

Please get rid of the whitespace changes in this file.

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:52
(Diff revision 2)
>    });
>    // check permission is set
>    let ps = Services.perms;
>    is(ps.ALLOW_ACTION, ps.testPermission(makeURI(pageWithAlert), "focus-tab-by-prompt"),
>       "Tab switching should now be allowed");
> +  //Check if the control center shows the correct permission

Nit: Please add a space after the // slashes and end the comment with a dot. (Also below)

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:53
(Diff revision 2)
>    // check permission is set
>    let ps = Services.perms;
>    is(ps.ALLOW_ACTION, ps.testPermission(makeURI(pageWithAlert), "focus-tab-by-prompt"),
>       "Tab switching should now be allowed");
> +  //Check if the control center shows the correct permission
> +  gIdentityHandler._identityPopup.showPopup()

My advice on IRC wasn't really solid, sorry. It's probably better to show the popup like this:

let shown = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popupshown");
gIdentityHandler._identityBox.click();
yield shown;
  
This creates a Promise that waits for the popup to be shown and then clicks the (i) icon in the identity box. That way we know for sure that the popup is properly loaded and was loaded the way a user would do it.

This is also why your test is failing, I assume.

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:56
(Diff revision 2)
>       "Tab switching should now be allowed");
> +  //Check if the control center shows the correct permission
> +  gIdentityHandler._identityPopup.showPopup()
> +  let labelText = SitePermissions.getPermissionLabel("focus-tab-by-prompt");
> +  let permissionsList = document.getElementById("identity-popup-permission-list");
> +  let labels = permissionsList.querySelectorAll(".identity-popup-permission-label");

You can simply use querySelector instead of querySelectorAll, that will return only the first found element.

https://developer.mozilla.org/en-US/docs/Web/API/Element/querySelector

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:57
(Diff revision 2)
> +  //Check if the control center shows the correct permission
> +  gIdentityHandler._identityPopup.showPopup()
> +  let labelText = SitePermissions.getPermissionLabel("focus-tab-by-prompt");
> +  let permissionsList = document.getElementById("identity-popup-permission-list");
> +  let labels = permissionsList.querySelectorAll(".identity-popup-permission-label");
> +  is(labels[0], labelText, "Correct value");

Currently you're comparing with a DOM/XUL node, you need to compare with label.textContent

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:60
(Diff revision 2)
> +  let permissionsList = document.getElementById("identity-popup-permission-list");
> +  let labels = permissionsList.querySelectorAll(".identity-popup-permission-label");
> +  is(labels[0], labelText, "Correct value");
> +  gIdentityHandler._identityPopup.hidePopup()
> +
> +  //Check if the identity icon signals granted permission  

In addition to the comment above, there's trailing whitespace here.

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:62
(Diff revision 2)
> +  is(labels[0], labelText, "Correct value");
> +  gIdentityHandler._identityPopup.hidePopup()
> +
> +  //Check if the identity icon signals granted permission  
> +  ok(gIdentityHandler._identityBox.classList.contains("grantedPermissions"),
> +    "identity-box signals granted permissions");  

Nit: trailing whitespace.

::: browser/modules/SitePermissions.jsm:609
(Diff revision 2)
> -  "indexedDB": {}
> +  "indexedDB": {},
> +
> +  "focus-tab-by-prompt": {
> +    exactHostMatch: true,
> +    states: [ SitePermissions.UNKNOWN, SitePermissions.ALLOW ],
> +  }

Nit: Please add a comma here.

::: browser/themes/shared/notification-icons.inc.css:42
(Diff revision 2)
> +.focus-tab-by-prompt-icon {
> +  list-style-image: url(chrome://browser/skin/notification-icons.svg#focus-tab-by-prompt);
> +}
> +
>  .popup-notification-icon[popupid="web-notifications"],
> +

Please delete this empty line that was added.
Attachment #8850406 - Flags: review?(jhofmann) → review-
(In reply to Jacqueline Savory [:jsavory] UX from comment #25)
> Adding in my two cents; to me the first option "[This website can] Switch to
> its Tab" sounds like it would make the most sense to the user. The term
> "Switching tabs" is pretty common from what I can tell. However, I'd love to
> hear what Michelle recommends as well :)

I think "switching" is a good compromise (and I don't want to bikeshed too much around this). How about "Switch to this tab" instead of "its"? It makes more sense to me when used standalone (without "This website can") which is how it will eventually be. Without context you don't really know what "its tab" is (and that might even be worse for localized strings).

I'd also be interested what Michelle thinks (though I know she's busy with other stuff right now).
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review126752

Okay. I'll make the changes. Thanks :)
I agree with Johann - "Switch to this tab" is the best option.
Flags: needinfo?(mheubusch)
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review126752

> My advice on IRC wasn't really solid, sorry. It's probably better to show the popup like this:
> 
> let shown = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popupshown");
> gIdentityHandler._identityBox.click();
> yield shown;
>   
> This creates a Promise that waits for the popup to be shown and then clicks the (i) icon in the identity box. That way we know for sure that the popup is properly loaded and was loaded the way a user would do it.
> 
> This is also why your test is failing, I assume.

Thanks!
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review127604

Thanks for the update, this looks good but there are still two minor nits to solve.

::: browser/base/content/test/general/browser_permissions.js:196
(Diff revision 4)
>  
>    SitePermissions.remove(gBrowser.currentURI, "geo");
>  
>    ok(!geoIcon.hasAttribute("showing"),
>      "blocked permission icon is not shown after reset");
>  });

There are still whitespace changes here. You might want to do

hg revert -r central browser/base/content/test/general/browser_permissions.js

::: browser/themes/shared/notification-icons.inc.css:41
(Diff revision 4)
> +.focus-tab-by-prompt-icon {
> +  list-style-image: url(chrome://browser/skin/notification-icons.svg#focus-tab-by-prompt);
> +}
> +
>  .popup-notification-icon[popupid="web-notifications"],
> +

You still need to get rid of this blank line
Attachment #8850406 - Flags: review?(jhofmann) → review-
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review128082

Looks great, thank you!

::: browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js:60
(Diff revision 5)
> +  gIdentityHandler._identityBox.click();
> +  yield shown;
> +  let labelText = SitePermissions.getPermissionLabel("focus-tab-by-prompt");
> +  let permissionsList = document.getElementById("identity-popup-permission-list");
> +  let label = permissionsList.querySelector(".identity-popup-permission-label");
> +  is(label.textContent, labelText, "Correct value");

Nit: "Correct value" is not a very helpful assertion text, you can probably just remove it.
Attachment #8850406 - Flags: review?(jhofmann) → review+
Comment on attachment 8850406 [details]
Bug 1224137 - List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel.

https://reviewboard.mozilla.org/r/122996/#review128082

Okay. Thanks :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
ESLint is failing on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4e9ec27e89088f4057a81be6ef063b3139ad5be&selectedJob=88144072

because two files are missing newlines at the end (http://eslint.org/docs/rules/eol-last)

You can run ESLint locally using

./mach eslint browser/

and in this case I think the issue can be fixed with 

./mach eslint browser/ --fix

(This scans all of browser and might take a while, you can also run "./mach eslint path/to/the/file" for a quicker result)

Thanks!
Keywords: checkin-needed
Keywords: checkin-needed
Autoland can't merge this commit until all pending issues are marked as resolved in MozReview.
Keywords: checkin-needed
browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js is also still failing ESLint because of a missing newline, can you please patch that one up as well? :)

Thanks!
Flags: needinfo?(prathikshaprasadsuman)
So sorry about that. :( I'll do it.
Flags: needinfo?(prathikshaprasadsuman)
So sorry about that. :( I'll fix it.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce87577c5da8
List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel. r=johannh
Keywords: checkin-needed
Sorry, this had to be backed out for failing xpcshell test browser/modules/test/unit/test_SitePermissions.js:

https://hg.mozilla.org/integration/autoland/rev/c55f652b7ae25a922373a0119dffa650cab07aa6

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=88357083&repo=autoland

[task 2017-04-03T16:59:18.787510Z] 16:59:18  WARNING -  TEST-UNEXPECTED-FAIL | browser/modules/test/unit/test_SitePermissions.js | xpcshell return code: 0
[task 2017-04-03T16:59:18.808093Z] 16:59:18     INFO -  TEST-INFO took 781ms
[task 2017-04-03T16:59:18.808312Z] 16:59:18     INFO -  >>>>>>>
[task 2017-04-03T16:59:18.808609Z] 16:59:18     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-04-03T16:59:18.810029Z] 16:59:18     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-04-03T16:59:18.810289Z] 16:59:18     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-04-03T16:59:18.811617Z] 16:59:18     INFO -  running event loop
[task 2017-04-03T16:59:18.812781Z] 16:59:18     INFO -  browser/modules/test/unit/test_SitePermissions.js | Starting testPermissionsListing
[task 2017-04-03T16:59:18.813933Z] 16:59:18     INFO -  (xpcshell/head.js) | test testPermissionsListing pending (2)
[task 2017-04-03T16:59:18.815749Z] 16:59:18  WARNING -  TEST-UNEXPECTED-FAIL | browser/modules/test/unit/test_SitePermissions.js | testPermissionsListing - [testPermissionsListing : 10] Correct list of all permissions - ["camera","cookie","desktop-notification","focus-tab-by-prompt","geo","image","indexedDB","install","microphone","popup","screen deepEqual ["camera","cookie","desktop-notification","geo","image","indexedDB","install","microphone","popup","screen"]
[task 2017-04-03T16:59:18.817002Z] 16:59:18     INFO -      /home/worker/workspace/build/tests/xpcshell/tests/browser/modules/test/unit/test_SitePermissions.js:testPermissionsListing:10
[task 2017-04-03T16:59:18.817405Z] 16:59:18     INFO -      _run_next_test@/home/worker/workspace/build/tests/xpcshell/head.js:1554:9
[task 2017-04-03T16:59:18.818532Z] 16:59:18     INFO -      run@/home/worker/workspace/build/tests/xpcshell/head.js:703:9
[task 2017-04-03T16:59:18.828131Z] 16:59:18     INFO -      _do_main@/home/worker/workspace/build/tests/xpcshell/head.js:211:5
[task 2017-04-03T16:59:18.828492Z] 16:59:18     INFO -      _execute_test@/home/worker/workspace/build/tests/xpcshell/head.js:546:5
[task 2017-04-03T16:59:18.828691Z] 16:59:18     INFO -      @-e:1:1
[task 2017-04-03T16:59:18.828969Z] 16:59:18     INFO -  exiting test

Fix should be to just add that new permission in the test.
Flags: needinfo?(prathikshaprasadsuman)
Argh, I added this test while this bug was in development and forgot to mention it. Yes, you can simply add the permission id to this list

http://searchfox.org/mozilla-central/source/browser/modules/test/unit/test_SitePermissions.js#75

and you can locally run the test with

./mach test browser/modules/test/unit/test_SitePermissions.js

Thanks!
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8193c2ac137
List the 'allow pages from <example.com> to take you to their tab' permission (for alerts/prompts) in the site identity / permission panel. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c8193c2ac137
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: