Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

do not allow emojis for page title in titlemode #10051

Closed
cezaraugusto opened this issue Jul 19, 2017 · 12 comments
Closed

do not allow emojis for page title in titlemode #10051

cezaraugusto opened this issue Jul 19, 2017 · 12 comments

Comments

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Jul 19, 2017

Test plan

  1. have title mode enabled
  2. go to http://xn--https-5w14d.cf/paypal.com/
  3. titlebar should NOT have emoji in it (should NOT look like picture below)

Original issue description

  1. have title mode enabled
  2. go to http://xn--https-5w14d.cf/paypal.com/

result:

screen shot 2017-07-19 at 12 02 54 am

cc @diracdeltas

@bsclifton
Copy link
Member

bsclifton commented Jul 19, 2017

We are showing punycode (ex: the domain still has the xn-- in it)... however, the page title is showing emoji (which is tricky). We may consider filtering certain emoji?

@diracdeltas
Copy link
Member

i'm a fan of erring on the safe side and not allowing emoji to show up in titleMode. @bradleyrichter thoughts?

@bradleyrichter
Copy link
Contributor

I agree. Sounds messy and problematic.

@cezaraugusto cezaraugusto changed the title show punycode for title in titlemode do not allow emojis for page title in titlemode Jul 19, 2017
@diracdeltas diracdeltas added this to the 0.21.x milestone Jul 25, 2017
@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented Aug 1, 2017

how-to steps

for the person interested: we render the title for titleMode in urlBar component, most precisely in this line. A safe way to solve this is to install the emoji-regex module, which is a small package made just for emojis:

npm i emoji-regex --save

you can check our package.json to ensure we have it. Once confirmed, include it on top of urlBar.js as a dependency:

const emojiRegex = require('emoji-regex')

Then still in urlBar.js, search for the titleValue getter. That's where our hero is waiting for a fix. At the moment of this writing the method looks like this:

  get titleValue () {
    // For about:newtab we don't want the top of the browser saying New Tab
    // Instead just show "Brave"
    return ['about:blank', 'about:newtab'].includes(this.props.urlbarLocation)
      ? '' : this.props.title // HERE'S OUR HERO!!
  }

See that the else condition in the above ternary? this.props.title is the code for the title you want to strip out the emojis.

You can make use of the emoji-regex module we just installed, and just replace() all emoji occurrences with an empty string:

  get titleValue () {
    // For about:newtab we don't want the top of the browser saying New Tab
    // Instead just show "Brave"
    const title = 
    return ['about:blank', 'about:newtab'].includes(this.props.urlbarLocation)
-      ? '' : this.props.title
+      ? '' : this.props.title.replace(emojiRegex(), '')
  }

ensure to run our app again and test http://xn--https-5w14d.cf/paypal.com/ to see if you still see that golden evil lock emoji. Don't you? We're waiting for your PR!

@cezaraugusto cezaraugusto added the includes hints ╭(◔ ◡ ◔)/ A good first bugs w/ hints made by someone from the team. label Aug 1, 2017
@prasanthp96
Copy link
Contributor

I am interested to fix this bug.

@prasanthp96
Copy link
Contributor

hy @cezaraugusto how to enable the title mode in brave??

@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented Aug 3, 2017

if you're running Windows you'll need to disable "always show the URL bar" in preferences->advanced->URL Bar Options:

screen shot 2017-08-03 at 10 37 00 am

we call titleMode the additional layer which hides the urlbar form until you mouse over the chrome bar. It looks like the screenshot I added in the first comment.

@prasanthp96
Copy link
Contributor

hello @cezaraugusto , I have sent a PR . Kindly have a look at it.

@prasanthp96
Copy link
Contributor

hy @cezaraugusto , my pr does not pass the travis test cases.. can you help me to fix the issue ?

@luixxiul
Copy link
Contributor

luixxiul commented Aug 6, 2017

@prasanthp96 currently there are some travis tests which consistently fail (we are trying to fix them all). If you see that your change does not break another test, then please leave the change as it is. If you cannot tell, please ping @cezaraugusto for audit :-)

@srirambv
Copy link
Collaborator

Still happens on Windows.
10051

@srirambv
Copy link
Collaborator

Works as expected

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants