Closed Bug 1206229 Opened 9 years ago Closed 8 years ago

Inform the user that changes may require a page reload

Categories

(Firefox :: General, defect, P2)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: MarcoM, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

Bug 1206229 - Inform the user that changes may require a page reload
58 bytes, text/x-review-board-request
Paolo
: review+
Details 
      No description provided.
Flags: qe-verify?
Priority: P3 → P2
Priority: P2 → P3
There are ~10 permissions defined in SitePermissions 
https://dxr.mozilla.org/mozilla-central/source/browser/modules/SitePermissions.jsm#126

Do we just show the notification every time when permission is changed? Or could you help list the items that require a page reload when permissions changes?
Flags: needinfo?(mmucci)
Aislinn I've checked the interaction UI https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/96832047
but have not found the related UI for this bug. 

Could you help confirm if this is required and describe/provide the proper UI for this?
Flags: needinfo?(agrigas)
(In reply to Fred Lin [:gasolin] from comment #1)
> There are ~10 permissions defined in SitePermissions 
> https://dxr.mozilla.org/mozilla-central/source/browser/modules/
> SitePermissions.jsm#126
> 
> Do we just show the notification every time when permission is changed? Or
> could you help list the items that require a page reload when permissions
> changes?

Forwarding to Javaun.
Flags: needinfo?(mmucci) → needinfo?(jmoradi)
(In reply to Fred Lin [:gasolin] from comment #2)
> Aislinn I've checked the interaction UI
> https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/96832047
> but have not found the related UI for this bug. 
> 
> Could you help confirm if this is required and describe/provide the proper
> UI for this?

Do you mean this UI? https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100296204
Where you clear out a permission and the user has to reload the page?
Flags: needinfo?(agrigas)
Thanks!
I see the `Two permissions will reset next time you reload this page` message is shown at the bottom of the control center. 

It would be good to work with bug 1203292 or after it is landed
Depends on: 1203292
Flags: needinfo?(jmoradi)
Priority: P3 → P2
Hi Fred, just wondering, now that bug 1203292 is landed, would you plan to work on this one ?
Flags: needinfo?(gasolin)
Since Bug 1203292 is mostly resolved by Ricky, he is more familiar with this part of code.

Ricky, does this issue still necessary in revised UI spec?
Flags: needinfo?(gasolin) → needinfo?(rchien)
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
Aisling,

According to the spec at https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100296204, it would be difficult if we decide to display the "literal" number on panel (in above prototype: "Two" permissions).

How about we just display this description with an Arabic numbers or change it to another one?
Flags: needinfo?(agrigas)
(In reply to Ricky Chien [:rickychien] from comment #8)
> Aisling,
> 
> According to the spec at
> https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100296204, it would
> be difficult if we decide to display the "literal" number on panel (in above
> prototype: "Two" permissions).
> 
> How about we just display this description with an Arabic numbers or change
> it to another one?

Yes it is required. Arabic numbers are fine.
Flags: needinfo?(agrigas)
Hi Paolo,

According to the spec at https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100296204, we have to display amount of permissions which user removes in permission list.

I'd like to set up a counter to accumulate the amount of permissions until user reload its webpage (it will reset the counter after page reload). Just figured out that we could do that in browser/base/content/browser.js by having a counter in gIdentityHandler and starting to accumulate the counter when a permission item is removed, and so far, so good.

Then I was trying to detect content page's "reload" event in browser/base/content/browser.js so that I'd be able to reset a counter after a page reload. 

Do you have any good idea how to do this? thanks a lot!
Flags: needinfo?(paolo.mozmail)
BTW, where is the best place to add this "reload" event listener in browser/base/content/browser.js?
(In reply to Ricky Chien [:rickychien] from comment #10)
> According to the spec at
> https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100296204, we have
> to display amount of permissions which user removes in permission list.

This state is actually closely related to the "allow temporarily" case that Florian and Johann are investigating these days, I'll redirect the request to them, as they may have discussed it already.

> I'd like to set up a counter to accumulate the amount of permissions until
> user reload its webpage (it will reset the counter after page reload). Just
> figured out that we could do that in browser/base/content/browser.js by
> having a counter in gIdentityHandler and starting to accumulate the counter
> when a permission item is removed, and so far, so good.

One feature of gIdentityHandler is that it is global to the browser window, in other words the same object operates for all the tabs. When there is a tab switch event, all the state is replaced with the one for the other tab. This makes it difficult to use this object to keep a counter that is tab-specific, or to track reload events for a specific tab.

We probably need something different, Florian or Johann can help.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jhofmann)
Flags: needinfo?(florian)
I think there are opportunities for simplification in the design here.

In lots of cases, the permission change will apply immediately (eg. stop sharing the camera). I don't have any case in mind where the change is guaranteed to apply after a page reload, as it's really up to the website author to decide if they re-prompt on page load. So I find the "will reset next time you reload this page" wording misleading.

I would suggest a wording along the lines of "Some changes may not apply until you reload the page." or "You may need to reload the page for changes to apply." because we don't really know when the changes will actually apply. It's possible that as soon as you stop blocking something the page will request it again. The opposite is also possible: the page may not prompt again at the next page load.

I think the main use of the message is to confirm to the user that something happened, as having lines just disappear when clicking a [x] is slightly awkward. I don't think it matters much for this message to still be there if you close or reopen the panel, or if you switch tab, so that can probably simplify the code. I also don't think the counter provides much user value, so we could probably simplify the code there too.

Aislinn, how do you feel about this alternative simplified proposal?
Flags: needinfo?(florian) → needinfo?(agrigas)
I'm fine using ""You may need to reload the page for changes to apply."

Thanks for the suggestion.
Flags: needinfo?(agrigas)
Cool, it is helpful to simplify the code!

So do we still adopt original design for showing up the message when clicking a [x] and remove the message after reloading?

If so, we still have to figure out how to detect a reload event for each tab.
Flags: needinfo?(agrigas)
I agree that the original behavior was unnecessarily complex and that a simple message does the job.

With that said, I think the current approach of this patch is a bit too simple, we should at least show the message only when the user actually cleared a permission by clicking on the [x] (maybe even only a permission that we know requires a page reload to clear). It's just distracting if it's always shown.

> If so, we still have to figure out how to detect a reload event for each tab.

As Florian said, this doesn't have to be persisted anywhere. We could simply mutate the DOM on clicking the [x] button and reset it on refreshIdentityPopup. I think that provides enough value for the user. It's more about giving an immediate reaction to the [x] click event, as far as I see it. 

You might have to listen to a load event for the current tab and make sure the identity popup is either closed or refreshed in that case.
Flags: needinfo?(jhofmann)
Comment on attachment 8779962 [details]
Bug 1206229 - Inform the user that changes may require a page reload

What Johann said.

Also, please add a separate <description> element and control its visibility from the code, rather than changing the label of the existing description. Among other benefits, this doesn't require to move the existing label to a different file, which reduces localization effort.
Attachment #8779962 - Flags: review?(paolo.mozmail)
Got it!

So let's go back to previous question:

Where is the best place to put the "load" listener? 
What's APIs we can use to access current tab and listen to its "load" event?

IIRC, every website will maintain their own permission state and so forth they should keep a reload state by their own for clearing the page reload message.
Flags: needinfo?(jhofmann)
You can find these methods implemented in browser.js: https://developer.mozilla.org/en-US/docs/Listening_to_events_on_all_tabs

This is what we mostly use to listen to all kinds of events right now. Note that most of them currently return early if the URL remains unchanged, because these events could also be triggered by e.g. subframes loading or permissions changing in the background.

http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#4425
http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#4595

I'm not certain how to get the state that we're looking for in these handlers, but Paolo and Florian might know.
Flags: needinfo?(jhofmann)
Mmh looking at it some more you might try to reload the identity popup in this block http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#4466.
(In reply to Johann Hofmann [:johannh] from comment #24)
> Mmh looking at it some more you might try to reload the identity popup in
> this block
> http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#4466.

This could work. We could add a gIdentityHandler.onLocationChange() call, similarly to how we call BookmarkingUI.onLocationChange().

If this gIdentityHandler.onLocationChange method only hides the <description> element and does nothing else, it's not going to create issues with other reload behavior.
(In reply to Ricky Chien [:rickychien] from comment #15)
> Cool, it is helpful to simplify the code!
> 
> So do we still adopt original design for showing up the message when
> clicking a [x] and remove the message after reloading?
> 
> If so, we still have to figure out how to detect a reload event for each tab.

yes
Flags: needinfo?(agrigas)
(In reply to :Paolo Amadini from comment #25)
> (In reply to Johann Hofmann [:johannh] from comment #24)
> > Mmh looking at it some more you might try to reload the identity popup in
> > this block
> > http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> > js#4466.
> 
> This could work. We could add a gIdentityHandler.onLocationChange() call,
> similarly to how we call BookmarkingUI.onLocationChange().

Thanks!

However, I noticed that tab change will invoke a gIdentityHandler.onLocationChange() call so that reload message of website A would be removed after switching tab to website B.

Have any idea?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jhofmann)
(In reply to Ricky Chien [:rickychien] from comment #27)
> However, I noticed that tab change will invoke a
> gIdentityHandler.onLocationChange() call so that reload message of website A
> would be removed after switching tab to website B.

This is fine, we don't need to persist the information after switching tabs. As comment 20 noted, the purpose here is more about giving an immediate feedback after the user action.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jhofmann)
OK, can we just remove the message after permission panel dismiss?

Switching a tab will dismiss permission panel automatically. We can achieve the requirement by removing the message at updateSitePermission()

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7246
Flags: needinfo?(paolo.mozmail)
Ah, we still have to remove message after a page reload since permission panel will not dismiss automatically when reloading.

However, I noticed that there is a panel shrinking bug if description element is set to "display:none".
Flags: needinfo?(paolo.mozmail)
Paolo, here is the current implementation. Just want to hear your feedback first to make sure patch is on the right track.
Comment on attachment 8779962 [details]
Bug 1206229 - Inform the user that changes may require a page reload

https://reviewboard.mozilla.org/r/70834/#review68712

Thanks for the update! So, the new logic is:
 - If a permission was just removed from the UI, show only the reload hint. Otherwise:
 - If the list is empty, show the empty hint.
 - If the list is not empty, show nothing.

The "just removed" state is reset by both onLocationChange and updatePermissions, and the elements are refreshed.

If you keep the "just removed" state as a boolean on gIdentityHandler, and only set the attributes on the elements using the logic above in one function, the code will be easier to read and update in the future.

::: browser/base/content/test/general/browser_permissions.js
(Diff revision 3)
>    ok(img.classList.contains("camera-icon"), "proper class is in image class");
>  
>    SitePermissions.remove(gBrowser.currentURI, "camera");
>  
>    gIdentityHandler._identityBox.click();
> -  ok(!is_hidden(emptyLabel), "List of permissions is empty");

This check should stay, but we need new test cases to check the new variants introduced by the patch.

::: browser/themes/shared/controlcenter/panel.inc.css:373
(Diff revision 3)
> -#identity-popup-permission-list:not(:empty) + description {
> +#identity-popup-permission-empty-hint,
> +#identity-popup-permission-reload-hint {
>    display: none;
>  }

You can use setAttribute("hidden", "true") and removeAttribute("hidden") in the code so you don't need to have this rule and set the style directly.
Attachment #8779962 - Flags: review?(paolo.mozmail)
Forgot to update test case in previous patch.
New test case has been added, please see latest patch on comment 35!

Push to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64879d0bf5eb
Oops there is an intermittent failure and error messages are different on local and try...
Paolo, Do you have any idea how this intermittent failure happened? (see log on comment 37)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8779962 [details]
Bug 1206229 - Inform the user that changes may require a page reload

(In reply to Ricky Chien [:rickychien] from comment #39)
> Paolo, Do you have any idea how this intermittent failure happened? (see log
> on comment 37)

I suspect that the methods that open and close the panel may not take effect immediately, or the notification of reloads might be out of sync with what we are waiting for.

Unfortunately debugging this kind of issues takes time. One thing you might try is to wait for the "popupshown" and "popuphidden" events using "BrowserTestUtils.waitForEvent". You may need to add a helper function like Sean did for the Downloads Panel:

https://reviewboard.mozilla.org/r/62304/diff/17#5

For the rest, the production code looks quite good, so this patch should be able to land once the test is fixed.
Attachment #8779962 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #44)
> Comment on attachment 8779962 [details]
> Bug 1206229 - Inform the user that changes may require a page reload
> 
> (In reply to Ricky Chien [:rickychien] from comment #39)
> > Paolo, Do you have any idea how this intermittent failure happened? (see log
> > on comment 37)
> 
> I suspect that the methods that open and close the panel may not take effect
> immediately, or the notification of reloads might be out of sync with what
> we are waiting for.

Thank you for pointing out the clue even though test results of my latest patch seems good after many tries on local and Try. I've followed your suggestion and add a helper function to openIdentifyPopup. Please take a look again.

Attach a try result:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b529ec65b54a
Comment on attachment 8779962 [details]
Bug 1206229 - Inform the user that changes may require a page reload

https://reviewboard.mozilla.org/r/70834/#review71374

Looks good in general, but there is still an issue with the test.

::: browser/base/content/test/general/browser_permissions.js:20
(Diff revision 8)
> +function* openIdentityPopup() {
> +  let {gIdentityHandler} = gBrowser.ownerGlobal;
> +  gIdentityHandler._identityPopup.hidden = true;
> +  gIdentityHandler._identityBox.click();
> +  return BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popupshown");
> +}

You need two functions, openIdentityPopup that waits for "popupshown" and closeIdentityPopup that waits for "popuphidden".

To avoid race conditions, you need to start waiting for the event \_before\_ the event is triggered, and check the Promise later:

let promise = BrowserTestUtils.waitForEvent(...);
gIdentityHandler.\_identityBox.click();
yield promise;

Note that the "return" here does nothing, it's a common error with generators that happened to me as well.

Since the test is not currently waiting for the event, after fixing it you need to test again that it does not block. Note that this requires that the open and close functions are called at the right time and that they match. For example, the test would block if you call the open function two times in a row. Fortunately, a local test run would catch these mismatches easily.
Attachment #8779962 - Flags: review?(paolo.mozmail)
gIdentityHandler._identityPopup.hidden = true; doesn't trigger a "popuphidden" event so that test case will get stuck to wait "popuphidden".

I come up with this solution for closing popup:

function* closeIdentityPopup() {
  let {gIdentityHandler} = gBrowser.ownerGlobal;
  let promise = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popuphidden");
  EventUtils.sendKey("esc");
  return promise;
}

But it threw exception as follow:

50 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_permissions.js | Uncaught exception - Unknown key: VK_ESC
51 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_permissions.js | Uncaught exception - Unknown key: VK_ESC
52 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_permissions.js | Uncaught exception - Unknown key: VK_ESC

Is there better way to close identityPopup?
Flags: needinfo?(paolo.mozmail)
Have you tried the hidePopup method?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel#Methods
Flags: needinfo?(paolo.mozmail)
Good catch! It works! 

Test passed locally and push to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=623c42ad8382
Comment on attachment 8779962 [details]
Bug 1206229 - Inform the user that changes may require a page reload

https://reviewboard.mozilla.org/r/70834/#review71396

Thanks! This can land if the tryserver build succeeds.
Attachment #8779962 - Flags: review?(paolo.mozmail) → review+
Thanks a lots! try server build succeeds!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d1380b51b174
Inform the user that changes may require a page reload r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d1380b51b174
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.2 - Aug 29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: