Closed Bug 1369482 Opened 7 years ago Closed 6 years ago

Hostname in permissions prompt should be much more prominent

Categories

(Firefox :: Site Permissions, defect)

Product:
Firefox
Component:
Site Permissions
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bzbarsky, Assigned: prathiksha)

References

Details

Attachments

(4 files)

Bug 1369482 - Make hostnames in permission prompts much more prominent.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Example patch that adds two more <description> elements
5.87 KB, patch
johannh
: feedback+
Details | Diff | Splinter Review
test_rtl.png
43.78 KB, image/png
Details
1369482-webext-regressions.patch
7.89 KB, patch
Details | Diff | Splinter Review 
STEPS TO REPRODUCE:

1)  Load http://jsfiddle.net/jib1/srn9db4h/
2)  Get a prompt for allowing the page to use camera and microphone
3)  Try to figure out what server is actually asking you for permission

ACTUAL RESULTS: The "fiddle.jsshell.net" in the prompt is lost in a wall of other text

EXPECTED RESULTS: That part should be much more prominent (bolder, larger font, possibly with extra highlighting on the eTLD+1 like our URL bar has, etc).
I agree and this applies to all permission prompts IMO, though I think making it bold should be enough.

Unfortunately PopupNotifications doesn't know about URLs since it's just a generic module for displaying prompts of any kind. This could be more effort than it would seem on the surface.
Summary: Hostname in getUserMedia permissions prompt should be much more prominent → Hostname in permissions prompt should be much more prominent
(In reply to Johann Hofmann [:johannh] from comment #1)

> Unfortunately PopupNotifications doesn't know about URLs since it's just a
> generic module for displaying prompts of any kind. This could be more effort
> than it would seem on the surface.

We had to figure out a way to show bold text for the web extension install prompts, so there's an example at http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/browser/modules/ExtensionsUI.jsm#451
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

Hey Florian. Could you please take a look at this small patch and give me some feedback? :) 

I have made some changes like it is shown here but bundle.formatStringFromName() isn't formatting the string at all in my patch and I don't understand why.
Maybe we should split these strings(https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#619) like this (http://searchfox.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#50) ? Is that a good idea ? Thanks.
Attachment #8899352 - Flags: feedback?(florian)
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

https://reviewboard.mozilla.org/r/170580/#review175826

See bug 1358431 for a working example of something similar.

::: browser/modules/webrtcUI.jsm:377
(Diff revision 1)
>  
>    let uri = Services.io.newURI(aRequest.documentURI);
>    let host = getHost(uri);
>    let chromeDoc = aBrowser.ownerDocument;
>    let stringBundle = chromeDoc.defaultView.gNavigatorBundle;
> +  let bundle = Services.strings.createBundle(BROWSER_PROPERTIES);

How do you expect stringBundle and bundle to be different here?

::: browser/modules/webrtcUI.jsm:396
(Diff revision 1)
>      "getUserMedia.shareCameraAndAudioCapture2.message",
>      "getUserMedia.shareScreenAndMicrophone3.message",
>      "getUserMedia.shareScreenAndAudioCapture3.message",
>    ].find(id => id.includes(joinedRequestTypes));
>  
> -  let message = stringBundle.getFormattedString(stringId, [host]);
> +  let message = bundle.formatStringFromName(stringId, [host], 1);

The message given to PopupNotifications.show can't contain HTML, this is the reason why your patch doesn't work.
Attachment #8899352 - Flags: feedback?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Comment on attachment 8899352 [details]
> Bug 1369482 - Hostname in permissions prompt should be much more prominent

Thanks for the feedback. :)

> ::: browser/modules/webrtcUI.jsm:377
> (Diff revision 1)
> >  
> >    let uri = Services.io.newURI(aRequest.documentURI);
> >    let host = getHost(uri);
> >    let chromeDoc = aBrowser.ownerDocument;
> >    let stringBundle = chromeDoc.defaultView.gNavigatorBundle;
> > +  let bundle = Services.strings.createBundle(BROWSER_PROPERTIES);
> 
> How do you expect stringBundle and bundle to be different here?

'stringBundle' is an XUL:stringbundle object but 'bundle' here pertains to the nsIStringBundle service.
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> XUL:stringbundle is just a wrapper:
> http://searchfox.org/mozilla-central/source/toolkit/content/widgets/
> stringbundle.xml

oh, right. Thanks!
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

https://reviewboard.mozilla.org/r/170580/#review178302

So, seems like this patch specifically touches WebRTC prompts, which was the original target of this bug by the look of things. But Johann in comment 1 changed the description to indicate that we should do this for all permission prompts, which is an enhancement to the PopupNotifications module.

I reviewed the patch, but I wanted to point out that it'd be nice to confirm the scope of this bug. I'd want Johann to make the call on this, but since he's on PTO, maybe we can do a basic investigation on whether enhancing PopupNotifications.jsm will be difficult, and based on the findings, we can make a call?

::: browser/base/content/popup-notifications.inc:13
(Diff revision 5)
>             noautofocus="true"
>             role="alert"/>
>  
>      <popupnotification id="webRTC-shareDevices-notification" hidden="true">
> +      <popupnotificationcontent id="webRTC-share-devices-notification-header" orient="vertical">
> +        <description></description>

The example Florian linked in comment 2 sets the innerHTML of the <description>, which makes more sense to me. The markup relevant to Florian's example seems to be http://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/browser/base/content/popup-notifications.inc#85.

So I suspect the id of the <popupnotificationcontent> should be "webRTC-share-devices-notification-content" and the id of the description should be "webRTC-share-devices-notification-header".

::: browser/modules/webrtcUI.jsm:334
(Diff revision 5)
>        const kBundleURI = "chrome://browser/locale/browser.properties";
>        let bundle = Services.strings.createBundle(kBundleURI);
>        host = bundle.GetStringFromName("getUserMedia.sharingMenuUnknownHost");
>      }
>    }
> -  return host;
> +  return (`<span class="webRTC-share-devices-host-name">${host}</span>`);

Nit: are the parentheses necessary to return a template literal?

::: browser/modules/webrtcUI.jsm:683
(Diff revision 5)
>          menupopup.appendChild(menuitem);
>          return menuitem;
>        }
>  
> +      doc.getElementById("webRTC-share-devices-notification-header").innerHTML = message;
> +      doc.getElementById("webRTC-share-devices-notification-header").hidden =

Once .hidden is set here, if there's no chance the element will be unhidden later, setting .innerHTML above is wasteful. If we can, let's avoid that.
Attachment #8899352 - Flags: review?(nhnt11)
Florian and Johann, the broadest interpretation of the scope of this bug is to extend the popup notifications API to allow the hostname to be styled separately in the header <description> of popup notificatinos. There are a couple of possible approaches to this bug and I'm not sure which one might be best. Can you comment?

1. Prathiksha's latest patch creates a header <description> dynamically and sets its innerHTML to the notification message passed by the consumer. Trying to set the innerHTML of the existing <description>[1] was not successful: the innerHTML was successfully set but would get wiped when the panel was eventually shown. I suspect this is because of CSS rules being applied afresh when the panel is appended/displayed, causing the binding to be re-applied and wiping our changes to the anonymous inner element.

2. Another possible approach is to add two more <description> elements to the popup-notification binding markup, to allow for the middle one to be styled separately. This is similar to what we do with the "Search for <text> with:" string just above the one-off buttons in the search dropdown. It would be possible to implement this without breaking existing notifications, and modifying the existing notifications to use this in future patches. I've attached a simple patch to clarify what I'm thinking of. RTL makes it a little more complex but I think I prefer this to dynamically creating an element and inserting it into the binding parent, because it makes things more obvious to future readers of the code.

What do you think? Do you prefer either of these approaches? Is there a much better way to do this that I'm not seeing?

Thanks!

[1] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/toolkit/content/widgets/notification.xml#515
Flags: needinfo?(jhofmann)
Flags: needinfo?(florian)
(In reply to Nihanth Subramanya [:nhnt11] from comment #15)
> Created attachment 8904312 [details] [diff] [review]
> Example patch that adds two more <description> elements
> 
> Florian and Johann, the broadest interpretation of the scope of this bug is
> to extend the popup notifications API to allow the hostname to be styled
> separately in the header <description> of popup notificatinos. There are a
> couple of possible approaches to this bug and I'm not sure which one might
> be best. Can you comment?
> 
> 1. Prathiksha's latest patch creates a header <description> dynamically and
> sets its innerHTML to the notification message passed by the consumer.
> Trying to set the innerHTML of the existing <description>[1] was not
> successful: the innerHTML was successfully set but would get wiped when the
> panel was eventually shown. I suspect this is because of CSS rules being
> applied afresh when the panel is appended/displayed, causing the binding to
> be re-applied and wiping our changes to the anonymous inner element.
> 
> 2. Another possible approach is to add two more <description> elements to
> the popup-notification binding markup, to allow for the middle one to be
> styled separately. This is similar to what we do with the "Search for <text>
> with:" string just above the one-off buttons in the search dropdown. It
> would be possible to implement this without breaking existing notifications,
> and modifying the existing notifications to use this in future patches. I've
> attached a simple patch to clarify what I'm thinking of. RTL makes it a
> little more complex but I think I prefer this to dynamically creating an
> element and inserting it into the binding parent, because it makes things
> more obvious to future readers of the code.
> 
> What do you think? Do you prefer either of these approaches? Is there a much
> better way to do this that I'm not seeing?
> 
> Thanks!
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 999385a5e8c2d360cc37286882508075fc2078bd/toolkit/content/widgets/
> notification.xml#515

BTW, I have so far not considered approaches that would require a lot of string changes.
Comment on attachment 8904312 [details] [diff] [review]
Example patch that adds two more <description> elements

Sorry for the long wait here, I'm strongly in favor of anything that avoids innerHTML (in privileged content with web-provided input!!).

I think option 2/attachment 8904312 [details] [diff] [review] sounds very promising.

Prathiksha, would you like to take up Nihanths approach and push it over the finish line? :)

We definitely need tests and documentation for this.

Please flag flod for review/feedback to make sure we don't do anything unwise with localized strings here.
Flags: needinfo?(jhofmann)
Flags: needinfo?(florian)
Attachment #8904312 - Flags: feedback+
Prathiksha, Johann seems to like my approach, so do you want to take over the patch (attachment 8904312 [details] [diff] [review])? :)
Flags: needinfo?(prathikshaprasadsuman)
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

https://reviewboard.mozilla.org/r/170580/#review192864
Attachment #8899352 - Flags: review?(nhnt11)
(In reply to Nihanth Subramanya [:nhnt11] from comment #18)
> Prathiksha, Johann seems to like my approach, so do you want to take over
> the patch (attachment 8904312 [details] [diff] [review])? :)

yes
Flags: needinfo?(prathikshaprasadsuman)
Attachment #8899352 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

Adding :pike for a second pair of eyes, if he has time to take a look.

The approach is not working as expected in RTL. Could you verify that? 

Ideally, it should be tested with a full build, e.g. in Hebrew (artifact builds don't work)
https://firefox-source-docs.mozilla.org/build/buildsystem/locales.html

I also tried with hacking RTL on top of an en-US artifact build, getting the same result:
* I changed the code for web notification permissions to always assume is RTL.
* Changed intl.uidirection in about:config to 1.
* Replaced the English string with the Hebrew string: 

webNotifications.receiveFromSite2 = האם לאפשר ל־%S לשלוח התרעות?

See screenshot: 
* Top: Firefox 57 in Hebrew
* Middle: full Hebrew build with your patch
* Bottom: hacked en-US build with your patch

Additional minor concern: localized string dropping the placeholder on purpose. Given the strings involved in this specific context, it shouldn't be a concern, but keep it in mind for any reuse of this approach.

Without the placeholder, you would still end up with the variable appended to the string, which is really confusing and will lead to poor UI.n
Attachment #8899352 - Flags: feedback?(francesco.lodolo) → feedback-
Attached image test_rtl.png — 

    
    

    
      
  

    
    

    
  
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

:flod, the old patch did not handle RTL strings very well. This new patch should do it. Can you take another look, please? :)

        Attachment #8899352 -
        Flags: feedback?(francesco.lodolo)

    
    

    
  
I wonder if this bug has another cycle or so. Then we could fix this as part of a migration to fluent, which would be a ton easier in practice.

    
    

    
  
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

This seems to work correctly also in RTL builds, can't spot any other issues.

It would be good to have an answer about comment 26 though.

        Attachment #8899352 -
        Flags: feedback?(francesco.lodolo) → feedback+

    
    

    
  
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

https://reviewboard.mozilla.org/r/170580/#review215292

This looks good, I think we're getting closer! Just a few edge cases to cover.

Can you please change the documentation here: https://searchfox.org/mozilla-central/rev/0bc3300a3abc4b12f14bd0ddf77ce8aa8125043c/toolkit/modules/PopupNotifications.jsm#333

And please also amend the canvas fingerprinting permission prompt here: https://searchfox.org/mozilla-central/rev/0bc3300a3abc4b12f14bd0ddf77ce8aa8125043c/browser/base/content/browser.js#6609 (We could also make a follow up bug for that).

::: browser/themes/shared/browser.inc.css:150
(Diff revision 8)
> +  margin-inline-end: 0;
> +}
> +
> +.popup-notification-description.endlabel {
> +  margin-inline-start: 0;
> +}

I think all additions to this file should be in toolkit/themes/shared/popupnotification.inc.css instead.

::: toolkit/modules/PopupNotifications.jsm:771
(Diff revision 8)
>          gNotificationParents.set(popupnotification, popupnotification.parentNode);
>        else
>          popupnotification = doc.createElementNS(XUL_NS, "popupnotification");
>  
> -      popupnotification.setAttribute("label", n.message);
> +      // Create the notification header element.
> +      if (n.message.start) {

There are some prompts that look broken now because they didn't change their message structure (addon installation prompts for example). I think we should still support single string messages, so can you please add an if-condition that checks whether n.message is a string?

        Attachment #8899352 -
        Flags: review?(jhofmann) → review-

    
    

    
  
Thanks for the l10n review, flod!

(In reply to Axel Hecht [:Pike] from comment #26)
> I wonder if this bug has another cycle or so. Then we could fix this as part
> of a migration to fluent, which would be a ton easier in practice.

While I'm looking forward to Fluent arriving soon, I'd like to avoid making this bug depend on it. I'm curious to learn how Fluent would help with this, though. Maybe we can make a follow up bug to clean up the hacks we introduced here?

Thanks!

    
  
Blocks: 1427952

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8899352 [details]
Bug 1369482 - Make hostnames in permission prompts much more prominent.

https://reviewboard.mozilla.org/r/170580/#review215762

This is great, thank you! I know it took a lot of effort to figure out a good way to do this the right way. Great work!

::: browser/modules/PermissionUI.jsm:646
(Diff revision 10)
>      let learnMoreURL =
>        Services.urlFormatter.formatURLPref("app.support.baseURL") + "storage-permissions";
>      return {
>        checkbox,
> -      learnMoreURL
> +      learnMoreURL,
> +      displayURI: false

We should probably consider using false as the default for displayURI, but that doesn't have to be in this bug.

::: toolkit/modules/PopupNotifications.jsm:334
(Diff revision 10)
>     *        A unique ID that identifies the type of notification (e.g.
>     *        "geolocation"). Only one notification with a given ID can be visible
>     *        at a time. If a notification already exists with the given ID, it
>     *        will be replaced.
>     * @param message
> -   *        The text to be displayed in the notification.
> +   *        A JavaScript object containing the text to be displayed in the

You could mention that it can be either a string or an object.

::: toolkit/modules/PopupNotifications.jsm:779
(Diff revision 10)
>          popupnotification = doc.createElementNS(XUL_NS, "popupnotification");
>  
> -      popupnotification.setAttribute("label", n.message);
> +      // Create the notification header element.
> +
> +      // Adding an if condition to check if n.message is still a string in order to support some
> +      // prompts(like addon installation prompts) which are yet to adopt the new message structure(See Bug 1369482).

I think we should always continue to allow passing a single string as label instead of an object (some prompts may not show a host at all), so maybe we should remove the "which are yet to adopt" part? :)

        Attachment #8899352 -
        Flags: review?(jhofmann) → review+

    
    

    
      
  

    
    

    
      
  

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2c2c5e26ddc
Make hostnames in permission prompts much more prominent. r=johannh
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b2c2c5e26ddc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox59:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

    
    

    
  


  
This regressed the top padding on some add-ons notifications since they were hiding the header based on the class and popupid. In the future they should use this update to avoid having to do that, I'll file to bring it up to date.

This also regressed the New Tab doorhanger you get when you install an extension that sets the New Tab page. I'm also adding a similar doorhanger for the home page which is what made me notice this. That code is setting the popupnotification[label] which is now replaced with startlabel,host,endlabel.

Here's a patch that fixes the add-ons notifications and works with the updates here. It instead replaces label with label,host,endlabel. This should keep backwards compatibility with notifications that just want a label.

Johann, does this changeset look good to you?
Flags: needinfo?(jhofmann)

        Attachment #8943399 -
        Flags: review?(jhofmann)

    
    

    
  
I just checked the webextension install notification and it's phrased as a question "Add <bolded extension name>?" but this adds spaces. I'm guessing the white space between these elements should be removed.

    
    

    
  
(In reply to Mark Striemer [:mstriemer] from comment #38)
> I just checked the webextension install notification and it's phrased as a
> question "Add <bolded extension name>?" but this adds spaces. I'm guessing
> the white space between these elements should be removed.

Interesting, :aryx pointed out that risk since there are new lines in the markup, but I didn't notice extra spaces added when I tested the patch. 

These are the changes after that test, unless I missed it completely the first time
https://reviewboard.mozilla.org/r/170580/diff/8-12/

I changed the notification string to
webNotifications.receiveFromSite2=Will you allow-%S-to send notifications?

And I see spaces indeed, which is going to be a problem for some locales, besides the case pointed out in comment 38 for en-US.

P.S. there's a typo in a comment, 'notifiation'

    
    

    
  
Comment on attachment 8943399 [details] [diff] [review]
1369482-webext-regressions.patch

Hey, thanks for noticing this! Let's fix these issues in new bugs, since this one is resolved. I'll file them now!
Flags: needinfo?(jhofmann)

        Attachment #8943399 -
        Flags: review?(jhofmann)
Depends on: 1431320
Depends on: 1431374

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: