838402 - Change site identity / Larry messages for mixed active content and mixed display content

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Larissa has worked on creating new Site Identity Messages while designing the Mixed Content Blocker.

She has worked with the security team, the UX team, Jonathan, Matej, and Dolske.  She needs to do one last pass with Matej for some tense changes.

This bug is to put these new site identity messages into the code base.



https://firefox-ux.etherpad.mozilla.org/MixedContent-messages

Case 1: Mixed Script - blocked and No Mixed Display
Icon: lock + shield
The connection to this page is secure. Only encrypted content is displayed.

Case 2: Mixed Script Blocked + Mixed Display
Icon: globe + shield
Interactive content (such as scripts) that isn't encrypted have been blocked for your protection.

Case 3: Mixed Script - enabled and No Mixed Display
Icon: warning
This page contains interactive content (such as scripts) that isn't encrypted. 
Others can view or modify the page's behavior.

Case 4: Mixed Script Enabled + Mixed Display
Icon: Warning
This page contains interactive content (such as scripts) that isn't encrypted. 
Others can view or modify the page's behavior.


Case 5: Mixed Display only
Icon: globe
The connection to this page is not fully secure because it contains unencrypted elements (such as images).

Comment 1

[Larissa Co [:lco]]

Updated Messages (Matej, I moved the phrase "such as scripts" after "isn't encrypted" because of tense confusion. I know it's still not ideal because it modifies "interactive content" and not "isn't encrypted", but I thought it would be more clear. 

Case 1: Mixed Script - blocked and No Mixed Display
Icon: lock + shield
The connection to this page is secure. Only encrypted content is displayed.

Case 2: Mixed Script Blocked + Mixed Display
Icon: globe + shield
Interactive content that isn't encrypted (such as scripts) have been blocked for your protection.

Case 3: Mixed Script - enabled and No Mixed Display
Icon: warning
This page contains interactive content that isn't encrypted (such as scripts). 
Others can view or modify the page's behavior.

Case 4: Mixed Script Enabled + Mixed Display
Icon: Warning
This page contains interactive content that isn't encrypted (such as scripts). 
Others can view or modify the page's behavior.

Case 5: Mixed Display only
Icon: globe
The connection to this page is not fully secure because it contains unencrypted elements (such as images).

Comment 2

[u428464]

Shouldn't there be a new Larry icon for cases 3 and 4 (the yellow one is already used for uncertified error page) ?

Comment 3

[Tanvi Vyas[:tanvi]]

(In reply to Larissa Co [:lco] from comment #1)
> Updated Messages (Matej, I moved the phrase "such as scripts" after "isn't
> encrypted" because of tense confusion. I know it's still not ideal because
> it modifies "interactive content" and not "isn't encrypted", but I thought
> it would be more clear. 
> 
> Case 1: Mixed Script - blocked and No Mixed Display
> Icon: lock + shield
> The connection to this page is secure. Only encrypted content is displayed.
> 
> Case 2: Mixed Script Blocked + Mixed Display
> Icon: globe + shield
> Interactive content that isn't encrypted (such as scripts) have been blocked
> for your protection.
> 
> Case 3: Mixed Script - enabled and No Mixed Display
> Icon: warning
> This page contains interactive content that isn't encrypted (such as
> scripts). 
> Others can view or modify the page's behavior.
> 
> Case 4: Mixed Script Enabled + Mixed Display
> Icon: Warning
> This page contains interactive content that isn't encrypted (such as
> scripts). 
> Others can view or modify the page's behavior.
> 
> Case 5: Mixed Display only
> Icon: globe
> The connection to this page is not fully secure because it contains
> unencrypted elements (such as images).

Are these final?  Should I create a patch for these?  If anyone has issues, please speak up now.

Comment 4

[Tanvi Vyas[:tanvi]]

Bug 810820 addded some test cases to check the site identity message for mixed content pages.  This bug will have to update them when we change the identity messages to differentiate between mixed active and mixed display content.

tests/functional/testSecurity/manifest.ini
tests/functional/testSecurity/testEncryptedPageWarning.js
tests/functional/testSecurity/testMixedContentPage.js

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Updating site identity messages v1

Here is a patch that makes the changes suggested by Larissa.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 741572 [details] [diff] [review]
Updating site identity messages v1

Review of attachment 741572 [details] [diff] [review]:
-----------------------------------------------------------------

r- mainly for the property name change.


Which string gets displayed when an SSL page contains only mixed display content, and the user has set |security.mixed_content.block_display_content| to true?

::: browser/base/content/browser.js
@@ +6247,2 @@
>    IDENTITY_MODE_MIXED_ACTIVE_CONTENT    : "unknownIdentity mixedContent mixedActiveContent",  // SSL with unauthenticated content
> +  IDENTITY_MODE_MIXED_DISPLAY_LOADED_ACTIVE_BLOCKED    : "unknownIdentity mixedContent",  // SSL with unauthenticated content

That name is... Kind of long and unwieldy. But I don't have a better suggestion offhand. :)

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +221,5 @@
>  identity.identified.verifier=Verified by: %S
>  identity.identified.verified_by_you=You have added a security exception for this site.
>  identity.identified.state_and_country=%S, %S
>  
> +identity.encrypted=The connection to this page is secure. Only encrypted content is displayed.

Due to how our fantastic L10N tools work, you'll need to change the name of "identity.encrypted" so they trigger re-localization.

Are you sure you want the "Only encrypted content is displayed" bit? While it's symmetric with the other messages, if the user doesn't actually know anything about mixed content it kind of sticks out in a weird way. "Your car is running fine. Also the engine is not on fire."

These messages all talk about a "connection to a page" now, instead of a "website". But other (unchanged) messages still talk about a "website". They should probably be consistent.

@@ +224,5 @@
>  
> +identity.encrypted=The connection to this page is secure. Only encrypted content is displayed.
> +identity.mixed_content=The connection to this page is not fully secure because it contains unencrypted elements (such as images).
> +identity.mixed_display_loaded_active_blocked=Interactive content that isn't encrypted (such as scripts) have been blocked for your protection.
> +identity.mixed_active_content=This page contains interactive content that isn't encrypted (such as scripts). Others can view or modify the page's behavior.

Replace "Others" with "Eavesdroppers" or "Other people", maybe?

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Updating site identity messages v2

Updated patch.  See comments inline.

(In reply to Justin Dolske [:Dolske] from comment #6) 
> Which string gets displayed when an SSL page contains only mixed display
> content, and the user has set |security.mixed_content.block_display_content|
> to true?
If a page has only mixed display content that has been blocked, then the user should get the string for identity.encrypted:
identity.encrypted=The connection to this page is secure. Only encrypted content is displayed.

We did not special case this situation.  Also, if a user visits such a page and has set security.mixed_content.block_display_content, then there will be no shield icon.  The user will see a lock and potentially missing images/audio.  Here is an example page: https://people.mozilla.com/~tvyas/mixeddisplay.html

> ::: browser/base/content/browser.js
> @@ +6247,2 @@
> >    IDENTITY_MODE_MIXED_ACTIVE_CONTENT    : "unknownIdentity mixedContent mixedActiveContent",  // SSL with unauthenticated content
> > +  IDENTITY_MODE_MIXED_DISPLAY_LOADED_ACTIVE_BLOCKED    : "unknownIdentity mixedContent",  // SSL with unauthenticated content
> 
> That name is... Kind of long and unwieldy. But I don't have a better
> suggestion offhand. :)
> 
I know :(  But I don't have any better ideas either.  I've tried to change the other identity modes to match this closer, so we have:

IDENTITY_MODE_MIXED_DISPLAY_LOADED    : "unknownIdentity mixedContent mixedDisplayContent",  // SSL with unauthenticated display content
IDENTITY_MODE_MIXED_ACTIVE_LOADED    : "unknownIdentity mixedContent mixedActiveContent",  // SSL with unauthenticated active (and perhaps also display) content
IDENTITY_MODE_MIXED_DISPLAY_LOADED_ACTIVE_BLOCKED    : "unknownIdentity mixedContent",  // SSL with unauthenticated display content; unauthenticated active content is blocked.

Please also advise on the spacing/formatting for this part of the code.


> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +221,5 @@
> >  identity.identified.verifier=Verified by: %S
> >  identity.identified.verified_by_you=You have added a security exception for this site.
> >  identity.identified.state_and_country=%S, %S
> >  
> > +identity.encrypted=The connection to this page is secure. Only encrypted content is displayed.
> 
> Due to how our fantastic L10N tools work, you'll need to change the name of
> "identity.encrypted" so they trigger re-localization.

Any suggestions on what I should change this to?  identity.encrypted_ssl?  Is there anyway to get around this L10N issue and alert L10N without changing the identifier?


For the text changes, I will add that in a separate comment and flag Larissa with needinfo.

Comment 8

[Tanvi Vyas[:tanvi]]

(In reply to Justin Dolske [:Dolske] from comment #6) 
> Are you sure you want the "Only encrypted content is displayed" bit? While
> it's symmetric with the other messages, if the user doesn't actually know
> anything about mixed content it kind of sticks out in a weird way. "Your car
> is running fine. Also the engine is not on fire."
> 
Larissa, Justin is referring to this message; what do you think?
identity.encrypted=The connection to this page is secure. Only encrypted content is displayed.


> These messages all talk about a "connection to a page" now, instead of a
> "website". But other (unchanged) messages still talk about a "website". They
> should probably be consistent.
>

Larrisa, Justin is referring to:
identity.unencrypted=Your connection to this website is not encrypted.
identity.unknown.tooltip=This website does not supply identity information.

For consistency, do you want to change these from "website" to "page"?  Or should we change the proposal (comment 1) to "connection to this website"?


> > +identity.mixed_active_content=This page contains interactive content that isn't encrypted (such as scripts). Others can view or modify the page's behavior.
> 
> Replace "Others" with "Eavesdroppers" or "Other people", maybe?

In the latest patch, I have replaced "Others" with "Other people".  Is that okay?

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Bug 810820 addded some test cases to check the site identity message for
> mixed content pages.  This bug will have to update them when we change the
> identity messages to differentiate between mixed active and mixed display
> content.
> 
> tests/functional/testSecurity/manifest.ini
> tests/functional/testSecurity/testEncryptedPageWarning.js
> tests/functional/testSecurity/testMixedContentPage.js

I also need to update http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug435035.js#8

I will add another patch that updates these tests.  A push to try will reveal if there are other tests that also need updating.

Comment 10

[Larissa Co [:lco]]

(In reply to Tanvi Vyas [:tanvi] from comment #8)
> (In reply to Justin Dolske [:Dolske] from comment #6) 
> > Are you sure you want the "Only encrypted content is displayed" bit? While
> > it's symmetric with the other messages, if the user doesn't actually know
> > anything about mixed content it kind of sticks out in a weird way. "Your car
> > is running fine. Also the engine is not on fire."
> > 
> Larissa, Justin is referring to this message; what do you think?
> identity.encrypted=The connection to this page is secure. Only encrypted
> content is displayed.
> 

It's fine with me if we drop the "Only encrypted content is displayed." part.

> 
> > These messages all talk about a "connection to a page" now, instead of a
> > "website". But other (unchanged) messages still talk about a "website". They
> > should probably be consistent.
> >
> 
> Larrisa, Justin is referring to:
> identity.unencrypted=Your connection to this website is not encrypted.
> identity.unknown.tooltip=This website does not supply identity information.
> 
> For consistency, do you want to change these from "website" to "page"?  Or
> should we change the proposal (comment 1) to "connection to this website"?
> 
> 

Agree that it should be consistent if possible. But which is more accurate? For the Mixed content messages, it seems to me that it's "page", because not every page on a website will have mixed content errors? 

Then again, for the regular cases, "website" seems more accurate since it generally refers to the security of the entire website.

If the distinction between "pages" and "websites" is not a big deal in this case, then sure, let's change whichever's easiest (I assume it's this patch, since we're already tweaking it).

> > > +identity.mixed_active_content=This page contains interactive content that isn't encrypted (such as scripts). Others can view or modify the page's behavior.
> > 
> > Replace "Others" with "Eavesdroppers" or "Other people", maybe?
> 
> In the latest patch, I have replaced "Others" with "Other people".  Is that
> okay?

Ok.

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Larissa Co [:lco] from comment #10)
> (In reply to Tanvi Vyas [:tanvi] from comment #8)
> > Larrisa, Justin is referring to:
> > identity.unencrypted=Your connection to this website is not encrypted.
> > identity.unknown.tooltip=This website does not supply identity information.
> > 
> > For consistency, do you want to change these from "website" to "page"?  Or
> > should we change the proposal (comment 1) to "connection to this website"?
> > 
> > 
> 
> Agree that it should be consistent if possible. But which is more accurate?
> For the Mixed content messages, it seems to me that it's "page", because not
> every page on a website will have mixed content errors? 
> 
> Then again, for the regular cases, "website" seems more accurate since it
> generally refers to the security of the entire website.
> 
> If the distinction between "pages" and "websites" is not a big deal in this
> case, then sure, let's change whichever's easiest (I assume it's this patch,
> since we're already tweaking it).
> 

I think for identity.unknown.tooltip and identity.unecrypted, website is more accurate.

That means, I'd change the others to:


identity.encrypted=The connection to this website is secure.
identity.mixed_display_loaded=The connection to this website is not fully secure because it contains unencrypted elements (such as images).
identity.mixed_display_loaded_active_blocked=Interactive content that isn't encrypted (such as scripts) have been blocked for your protection.
identity.mixed_active_loaded=This page contains interactive content that isn't encrypted (such as scripts). Other people can view or modify the website's behavior.

Justin, Larissa - sound okay?

Comment 12

[Tanvi Vyas[:tanvi]]

> identity.mixed_active_loaded=This page contains interactive content that
> isn't encrypted (such as scripts). Other people can view or modify the
> website's behavior.
> 
Quick correction:
identity.mixed_active_loaded=This website contains interactive content that isn't encrypted (such as scripts). Other people can view or modify the website's behavior.

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: Updating site identity messages v3

Updated patch with text changes.  Justin, please also look at comment 7 while reviewing.  Thanks!

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 758195 [details] [diff] [review]
Updating site identity messages v3

Review of attachment 758195 [details] [diff] [review]:
-----------------------------------------------------------------

r- since you still need to fix the property name. The rest is ok, but do look into the nit I mention.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +229,5 @@
>  identity.identified.verifier=Verified by: %S
>  identity.identified.verified_by_you=You have added a security exception for this site.
>  identity.identified.state_and_country=%S, %S
>  
> +identity.encrypted=The connection to this website is secure.

You still need to change the property name when changing a string, that's just how L10N works. I know, it sucks. Pick something vaguely suitable or just call it identity.encrypted2.

A brief aside on this specific string change that's been gnawing at me:

I keep asking myself if we should be removing the mention of "encryption / eavesdropping" here, since while it might be technobabble to some it's not totally exotic. And quite topical since this patch was posted, given the NSA/PRISM stuff in the press! :)

But I think it's OK. If you click the "More Info" button you can get to the technical details which also explains the state of things (aka Page Info -> Security -> Technical Details. Very much tertiary UI!). It's ok for the Larry UI to explain the common case with just "Am I ok? Yes you're secure".

@@ +231,5 @@
>  identity.identified.state_and_country=%S, %S
>  
> +identity.encrypted=The connection to this website is secure.
> +identity.mixed_display_loaded=The connection to this website is not fully secure because it contains unencrypted elements (such as images).
> +identity.mixed_display_loaded_active_blocked=Interactive content that isn't encrypted (such as scripts) have been blocked for your protection.

Nit: isn't s/have/has/ the correct grammar? But it would not shock me if the plural "scripts" throws a technical oddball. Would making that singular ("code"? "JavaScript"?) help, or maybe just remove the parenthetical entirely?

Comment 15

[Larissa Co [:lco]]

(In reply to Justin Dolske [:Dolske] from comment #14)
> 
> I keep asking myself if we should be removing the mention of "encryption /
> eavesdropping" here, since while it might be technobabble to some it's not
> totally exotic. And quite topical since this patch was posted, given the
> NSA/PRISM stuff in the press! :)
> 
> But I think it's OK. If you click the "More Info" button you can get to the
> technical details which also explains the state of things (aka Page Info ->
> Security -> Technical Details. Very much tertiary UI!). It's ok for the
> Larry UI to explain the common case with just "Am I ok? Yes you're secure".

I think "eavesdropping" can be a confusing metaphor because it's associated with listening in on phone calls, rather than intercepting the network connection. So for the non-savvy user, using this term might cause further confusion about what's going on, whereas for the savvy user, it's an oversimplification of what's happening. I'd rather place the technical details in the "More Info" button, since the main message is that the connection is secure.


> 
> @@ +231,5 @@
> >  identity.identified.state_and_country=%S, %S
> >  
> > +identity.encrypted=The connection to this website is secure.
> > +identity.mixed_display_loaded=The connection to this website is not fully secure because it contains unencrypted elements (such as images).
> > +identity.mixed_display_loaded_active_blocked=Interactive content that isn't encrypted (such as scripts) have been blocked for your protection.
> 
> Nit: isn't s/have/has/ the correct grammar? But it would not shock me if the
> plural "scripts" throws a technical oddball. Would making that singular
> ("code"? "JavaScript"?) help, or maybe just remove the parenthetical
> entirely?

Oh man, grammar. This is a tricky sentence. I believe you are correct, when we simplify the sentence down to the essentials:

"Content that ISN"T encrypted HAS been blocked"

Unfortunately, "Interactive content" is pretty vague (links are interactive too!), so we have to clarify by giving an example. We can't say "Scripts that aren't encrypted have been blocked..." either, since active mixed content encompasses more than just scripts. 

I guess we could say something like:
"Scripts and other interactive content that aren't encrypted have been blocked for your protection." or 
"Unencrypted scripts and similar interactive content have been blocked for your protection."

Either sentence removes the confusing tense. But I think they're not any more clarifying. 

I'm happy to just fix the tense for accuracy's sake: "Interactive content that isn't encrypted (such as scripts) has been blocked for your protection", and live with the somewhat awkward grammar.

Comment 16

[Tanvi Vyas[:tanvi]]

I think these two are more confusing:
"Scripts and other interactive content that aren't encrypted have been blocked for your protection." or 
"Unencrypted scripts and similar interactive content have been blocked for your protection."

However, using "has" instead of "have" here is equally ugly because it's right after the plural scripts:
"Interactive content that isn't encrypted (such as scripts) has been blocked for your protection"

So I propose one of the following
1) Interactive content (such as scripts) that isn't encrypted has been blocked for your protection.
2) Interactive content (such as script) that isn't encrypted has been blocked for your protection.
3) Interactive content that isn't encrypted (such as script) has been blocked for your protection.

Yes, I know; now I'm the guilty party that is bikeshedding.

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Updating site identity messages v4

In this patch, I went with the following for identity.mixed_display_loaded_active_blocked

Interactive content (such as script) that isn't encrypted has been blocked for your protection.

Can change it based on what Larissa says.

Comment 18

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Bug 810820 addded some test cases to check the site identity message for
> mixed content pages.  This bug will have to update them when we change the
> identity messages to differentiate between mixed active and mixed display
> content.
> 
> tests/functional/testSecurity/manifest.ini
> tests/functional/testSecurity/testEncryptedPageWarning.js
> tests/functional/testSecurity/testMixedContentPage.js

Looks like these are mozmill tests and not mochitests.

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Update test case with new identity mode v1

Updated test case with new identity mode for mixed display content.

Comment 20

[Tanvi Vyas[:tanvi]]

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=e55601ca0740

Comment 21

[Larissa Co [:lco]]

(In reply to Tanvi Vyas [:tanvi] from comment #17)
> Created attachment 768693 [details] [diff] [review]
> Updating site identity messages v4
> 
> In this patch, I went with the following for
> identity.mixed_display_loaded_active_blocked
> 
> Interactive content (such as script) that isn't encrypted has been blocked
> for your protection.
> 
> Can change it based on what Larissa says.

I don't think "script" works as an actual term but according to the tech dictionary, it's valid. So let's just call it a day. The .0001% who read the Larry Menu and are grammar nazis can just hate us forever :P

Comment 22

[Tanvi Vyas[:tanvi]]

Attachment: Update site identity messages v5

Had to rebase.  Carrying over r+

Comment 23

[Tanvi Vyas[:tanvi]]

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b2cb47bddd
https://hg.mozilla.org/integration/mozilla-inbound/rev/213b01805710

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e6b2cb47bddd
https://hg.mozilla.org/mozilla-central/rev/213b01805710

Comment 25

[Francesco Lodolo [:flod]]

Hi, I'm a grammar nazi (and it's not even my grammar) :-)

"Other people can view or modify the website's behavior." 

Jokes apart, isn't this phrase confusing? At least it is for me ("view a website's behavior"), not being English my first language.

Comment 26

[Matej Novak [:matej]]

That's a great catch, flod. You're correct that it's a bit awkward. Let's change it to this:

"Other people can view the website or modify its behavior."

Comment 27

[Larissa Co [:lco]]

 (In reply to Matej Novak [:matej] from comment #26)
> "Other people can view the website or modify its behavior."

Spoke with Tanvi, and it turns out neither phrase is quite accurate. The message we're trying to convey is that other people can:

1. see that you're visiting the website and see any information that you're entering on it
2. modify the website's behavior 

So how about this:
"This page contains interactive content that isn't encrypted (such as scripts). Other people can view the information you enter or modify the website's behavior"

(I tried to focus on the important implications to the user, but let me know if the specificity is misleading)

Comment 28

[Tanvi Vyas[:tanvi]]

(In reply to Larissa Co [:lco] from comment #27)
>  (In reply to Matej Novak [:matej] from comment #26)
> > "Other people can view the website or modify its behavior."
> 
> Spoke with Tanvi, and it turns out neither phrase is quite accurate. The
> message we're trying to convey is that other people can:
> 
> 1. see that you're visiting the website and see any information that you're
> entering on it

They can see information that you are entering on the page.  For sites that you are logged into, they can also read information on the page that may be personal to you and sensitive (ex: if it is your email, they could read your email because of the mixed content).

Comment 29

[Matej Novak [:matej]]

(In reply to Larissa Co [:lco] from comment #27)
> So how about this:
> "This page contains interactive content that isn't encrypted (such as
> scripts). Other people can view the information you enter or modify the
> website's behavior"

That looks good to me. Are there any revisions required based on comment 28?

Comment 30

[Larissa Co [:lco]]

Well, it's going to be a balance between brevity and completeness.

If my previous suggestion is not broad enough, what about:

"This page contains interactive content that isn't encrypted (such as scripts). Other people can view your information or modify the website's behavior"

Comment 31

[Tanvi Vyas[:tanvi]]

(In reply to Larissa Co [:lco] from comment #30)
> Well, it's going to be a balance between brevity and completeness.
> 
> If my previous suggestion is not broad enough, what about:
> 
> "This page contains interactive content that isn't encrypted (such as
> scripts). Other people can view your information or modify the website's
> behavior"

Yes!  This is better :)  Matej, are you fine with this?

Comment 32

[Matej Novak [:matej]]

(In reply to Tanvi Vyas [:tanvi] from comment #31)
> (In reply to Larissa Co [:lco] from comment #30)
> > Well, it's going to be a balance between brevity and completeness.
> > 
> > If my previous suggestion is not broad enough, what about:
> > 
> > "This page contains interactive content that isn't encrypted (such as
> > scripts). Other people can view your information or modify the website's
> > behavior"
> 
> Yes!  This is better :)  Matej, are you fine with this?

Yup, looks great. Thanks!