Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #391 - trim wysiwyg:// from URL field if it's there. #392

Merged
merged 1 commit into from
Nov 18, 2014

Conversation

miketaylr
Copy link
Member

Simple if matches regex, remove what was matched fix.

r? @magsout

@magsout
Copy link
Member

magsout commented Nov 14, 2014

To optimize this test, it might be interesting to add type=url? https://github.com/webcompat/webcompat.com/blob/master/webcompat/templates/form.html#L12 Instead of text ?

@miketaylr
Copy link
Member Author

That's a good question. Originally I think the decision was to just stick with text because writing valid URLs by hand--especially on mobile--is a PITA. Maybe it's better to just be strict?

Let's get the feedback of @karlcow or @calexity.

@karlcow
Copy link
Member

karlcow commented Nov 15, 2014

When do we receive ``wysiwyg://? in which circumstances? Is it the user typing it or is it created by the browser itself? With type=url` what are the things the user will not be able to do anymore?
What are the things this will allow the user to do?

Then after having answers to these questions we can decide. But that seems to be the call for an issue by itself ;)

@miketaylr
Copy link
Member Author

When do we receive `wysiwyg://? in which circumstances?

See https://bugzilla.mozilla.org/show_bug.cgi?id=1098037 and a bunch of other bugs.

Is it the user typing it or is it created by the browser itself?

The browser, if a user is on a site like wired.com.

With type=url what are the things the user will not be able to do anymore?

Type "google.com". They'll be forced to type "http://google.com". More keystrokes with special characters on mobile keyboards is not fun.

What are the things this will allow the user to do?

No idea. :)

@miketaylr
Copy link
Member Author

Type "google.com". They'll be forced to type "http://google.com"

Possibly fixed with @pattern attribute. But there's more problems: http://justmarkup.com/log/2012/12/28/input-url/

I think the simplest thing to do is not do strict url validation. Either way we decide (probably in another issue, maybe #306), removing wysiwyg://0/ as a workaround of a Gecko bug will need to happen.

@calexity
Copy link
Contributor

I would prefer to not force anyone to put in a "valid" address given that you can put Google.com into your address bar and it works.

If you can strip out anything before the http, I think that's a better way to handle it. Is the issue that it's messy or hard to match multiple issues from the same domain?

@karlcow
Copy link
Member

karlcow commented Nov 17, 2014

type='url'

  • on iOS: triggers the character '/' and the choices of '.com' and so on.
  • on Firefox OS: triggers the character '/'

On the other hand it indeed seems to require http:// Here I wonder if JS can specifically enhance the user experience by filling the http:// string so the user has only the rest to type. That's a good use of JS on the client side.

  • The browser provides the user with customized keyboard
  • The JS provides the fix for http://

@karlcow
Copy link
Member

karlcow commented Nov 17, 2014

Also:

See the idea of Tiffany.

I put a test

@miketaylr
Copy link
Member Author

I think these are all good thoughts, but orthogonal to the issue this PR addresses--which is to work around a Gecko bug. Let's continue the discussion in #404.

@karlcow
Copy link
Member

karlcow commented Nov 17, 2014

Yes. Just need to see if @magsout is r+ or not. Thanks for opening the issue.

@magsout
Copy link
Member

magsout commented Nov 18, 2014

r+

magsout added a commit that referenced this pull request Nov 18, 2014
Fixes #391 - trim wysiwyg:// from URL field if it's there.
@magsout magsout merged commit a1e7079 into master Nov 18, 2014
@magsout magsout deleted the wysiwyg-hack branch November 18, 2014 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants