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

Help feature for first-time visitors who land on a Github issue #741

Closed
hallvors opened this issue Oct 1, 2015 · 22 comments
Closed

Help feature for first-time visitors who land on a Github issue #741

hallvors opened this issue Oct 1, 2015 · 22 comments

Comments

@hallvors
Copy link
Contributor

hallvors commented Oct 1, 2015

If we @mention GitHub-users, they are going to get a notification sending them to a GitHub issue URL. This is extremely useful for us and one of the main reasons we are sticking to GitHub as a backend.

However, it is not necessarily easy for an unsuspecting first-time visitor to some issue to understand what this project is about when arriving on a random bug report.

I suggest we add some standard text at the top of each bug report (we can hide this when we are on webcompat.com) - perhaps a "What's this about?" link?

@hallvors
Copy link
Contributor Author

hallvors commented Oct 1, 2015

@karlcow you like thinking about end users with different perspectives - thoughts here?

@miketaylr
Copy link
Member

You make a good point @hallvors. This would be essentially the other half to #267, and suddenly it seems more than just "nice to have". 👍

@karlcow
Copy link
Member

karlcow commented Oct 11, 2015

We discussed this a bit during Paris meeting.
I think it's true for all notifications. The "View on Github" thing.

And indeed adjusting the first comment on Github once it has been POSTed would help anyone not only beginners to go back to webcompat.com.

see this comment #267 (comment)
which explains how to do it.

@hallvors
Copy link
Contributor Author

Yes, this is like half of what we're talking about in #267 but I think that one is getting a bit scope creepy / messy. I would consider closing #267 and say the [G] shortcut handles the use case, then continue the work here.

@miketaylr
Copy link
Member

Yep, agreed. #267 is closed. :)~

@karlcow
Copy link
Member

karlcow commented Mar 10, 2016

To continue the discussion about this and because @deckycoss is interested on actually working on it.

When a user posts to webcompat.com a new issue…

  • webcompat.com is adding a boilerplate text in the JSON body (so it's visible on github)
  • webcompat.com is hiding the text on webcompat.com
  • to check if it is text only or html is authorized
  • Bonus: check if, by any chance, we can trigger a POST then PATCH to adjust a link with the issue number URI to webcompat.com

The boilerplate text will be to determined, but should be a variable. Putting a no meaning text for the purpose of testing. We can bikeshed later.

GITHUB_MSG = "I WILL arise and go now, and go to Innisfree,"

@daliacoss
Copy link
Contributor

assuming that github does not allow special metadata or html (i.e., a span tag) in its issue descriptions, my approach for the help feature would be something like this:

  1. from either views.create_issue, issues.report_issue, or form.build_formdata, insert the help message into the form's "description" field
  2. use js to hide or strip the first line of the description on the webcompat.com issue page

does this sound like the right track?

@karlcow
Copy link
Member

karlcow commented Mar 15, 2016

@deckycoss I think there is an easier way.

So let's try the "to check if it is text only or html is authorized" above.

I tested a local instance of webcompat on localhost and created a new bug report containing the following markup by hand:

<p class="boilerplate">testing a boilerplate from the form.</p>

The result is visible at webcompat/webcompat-tests#338
The markup saved on github side is:

<!-- @browser: Firefox 47.0  -->
<!-- @ua_header: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0 -->

**URL**: http://foo.example.com/
**Browser / Version**: Firefox 47.0 
**Operating System**: Mac OS X 10.11
**Problem type**: Text is not visible

**Steps to Reproduce**
<p class="boilerplate">testing a boilerplate from the form.</p>

1) Navigate to: http://foo.example.com/
2) …

Expected Behavior:

Actual Behavior:

Here for sure the markup is not at the right place, because I used the form. That's normal. But it also means that the markup was kept so if we insert it in the start of the body. It means we can just use css to hide it on webcompat.com without relying on JS.

@daliacoss
Copy link
Contributor

@karlcow i've been experimenting with this and i'm curious about something. i've found that inserting the following help message immediately before or after the browser information results in the message becoming hidden:

<p something="something">This issue was filed via webcompat.com</p>

however, removing the something="something" results in the message displaying normally. in other words, as long as the p tag has any attribute defined at all, it is hidden.

could you explain how this works? i can see that the message and browser information are enclosed in a paragraph with the "is-hidden" class, but i haven't figured out what makes that happen.

@karlcow
Copy link
Member

karlcow commented Mar 16, 2016

could you try with
<p class="something">This issue was filed via webcompat.com</p>

instead of

<p something="something">This issue was filed via webcompat.com</p>

@karlcow
Copy link
Member

karlcow commented Mar 16, 2016

For the feature which is adding the is-hidden. This is done in issues.js
https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/issues.js#L140
But that doesn't matter that much given that these are already <!-- HTML comments -->
which are inserted by https://github.com/webcompat/webcompat.com/blob/master/webcompat/form.py#L97-L104

but here we do not want a comment we really want something which is visible on Github and can be hidden on webcompat side.

something is not a valid attribute and should be clearly axed in any HTML sanitizer. On the other hand class which is a valid attribute might be accepted.

@daliacoss
Copy link
Contributor

@karlcow i actually tried <p class="something"> before trying the example i showed; the result appears to be the same.

so the js filter, if i'm reading the code right, looks for text matching html comment tags, and wraps everything in them in the hidden paragraph? i wonder then why it is able to hide the help message when it is inserted before the comment...

@karlcow
Copy link
Member

karlcow commented Mar 16, 2016

@deckycoss

so the js filter, if i'm reading the code right, looks for text matching html comment tags, and wraps everything in them in the hidden paragraph?

Yes but that doesn't matter.

i wonder then why it is able to hide the help message when it is inserted before the comment...

What do you mean?

@daliacoss
Copy link
Contributor

@karlcow i guess what i'm trying to say is, i'm still confused as to why a p without a class is not hidden by the filter, but a p with any class is hidden.

@karlcow
Copy link
Member

karlcow commented Mar 16, 2016

ok let me see.
I just tested locally.

→ git checkout -b deleteme
Switched to a new branch 'deleteme'

Then dumb editing the form.py

diff --git a/webcompat/form.py b/webcompat/form.py
index e5617c5..fcbd4cd 100644
--- a/webcompat/form.py
+++ b/webcompat/form.py
@@ -201,6 +201,8 @@ def build_formdata(form_object):

     # Preparing the body
     body = u'''{browser_label}{ua_label}
+<p class="boilerplate">This is the boilerplate text with a class.</p>
+<p>This is the boilerplate text without class.</p>
 **URL**: {url}
 **Browser / Version**: {browser}
 **Operating System**: {os}

Then

→ python run.py
secrets /Users/karl/code/webcompat.com
 * Running on http://localhost:5000/ (Press CTRL+C to quit)
 * Restarting with stat
secrets /Users/karl/code/webcompat.com
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET / HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /css/webcompat.dev.css HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /img/step1icon.png HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /img/step2icon.png HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /img/step3icon.png HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/jquery-1.11.2.min.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/lodash.custom.min.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/backbone-min.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/moment-min.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/prism.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/mousetrap-min.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/backbone.mousetrap.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/markdown-it.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/markdown-it-emoji-1.0.0.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/vendor/markdown-it-sanitizer-0.4.1.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/flash-message.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/homepage.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/bugform.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/models/label-list.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/labels.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/models/issue.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /js/lib/diagnose.js HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:24] "GET /font/webcompat/webcompat.woff?ccpilb HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:26] "GET /api/issues/labels?per_page=100 HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:26] "GET /api/issues/category/new HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:32] "GET /img/valid.svg HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:51] "GET /img/loader.gif HTTP/1.1" 200 -
127.0.0.1 - - [16/Mar/2016 15:31:52] "POST /issues/new HTTP/1.1" 302 -
127.0.0.1 - - [16/Mar/2016 15:31:52] "GET /thanks/339 HTTP/1.1" 200 -

Checking the results at:

webcompat/webcompat-tests#339

Bingo! Both worked.

So still not sure what you are trying to do.

@karlcow
Copy link
Member

karlcow commented Mar 16, 2016

btw the markup of that test issue is

<!-- @browser: Firefox 47.0  -->
<!-- @ua_header: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0 -->

<p class="boilerplate">This is the boilerplate text with a class.</p>
<p>This is the boilerplate text without class.</p>
**URL**: http://foo.example.com/
**Browser / Version**: Firefox 47.0 
**Operating System**: Mac OS X 10.11
**Problem type**: Something else - I'll add details below

**Steps to Reproduce**
Testing boilerplate text. There should be something above this.

@daliacoss
Copy link
Contributor

@karlcow thanks for testing this.

i decided to test this again using a message similar to yours:

<!-- @browser: Firefox 45.0 -->
<!-- @ua_header: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 -->

<p class="something">This issue was filed via webcompat.com</p>
<p>This issue was filed via webcompat.com</p>

...

i found, as you did, that both paragraphs are hidden: http://i.imgur.com/p3t8rSg.png

however, i then removed the first paragraph so the message looked like this:

<!-- @browser: Firefox 45.0 -->
<!-- @ua_header: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 -->

<p>This issue was filed via webcompat.com</p>

...

this time, it was not hidden: http://i.imgur.com/1lrSTkn.png

i don't know if this is important or not, but i at least find it curious. if it isn't a big deal then i will submit my pull request.

@karlcow
Copy link
Member

karlcow commented Mar 17, 2016

i found, as you did, that both paragraphs are hidden: http://i.imgur.com/p3t8rSg.png

Note that my paragraphs are not hidden. :) It worked.

I see you seem to have tested here. And I see both paragraph so it worked? or is it something else.
daliacoss/test-repo-wc#11

Push your commits to your repo with this issue number in the commits messages and we can start discuss about it. ^_^ Maybe the code will help us clarify what's happening on your side.

@daliacoss
Copy link
Contributor

sorry, i should have been clearer. by "hidden", i meant that it is hidden on the webcompat.com app, but visible on github.

i've made a pull request: #973

@karlcow
Copy link
Member

karlcow commented Mar 17, 2016

Ah understood I should have checked on my local instance. This is happening because the hidden is swallowing everything:

<div class="js-Issue-markdown wc-Markdown"><p class="is-hidden">
            &lt;!-- @browser: Firefox 47.0  --&gt;
&lt;!-- @ua_header: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0 --&gt;
&lt;p class="boilerplate"&gt;This is the boilerplate text with a class.&lt;/p&gt;
&lt;p&gt;This is the boilerplate text without class.&lt;/p&gt;
**URL**: http://foo.example.com/
**Browser / Version**: Firefox 47.0 
**Operating System**: Mac OS X 10.11
**Problem type**: Something else - I'll add details below
</p><p><strong>Steps to Reproduce</strong><br>
Testing boilerplate text. There should be something above this.</p>

          </div>

And it gives indeed:

capture d ecran 2016-03-18 a 06 48 03

We probably need to check how
https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/issues.js#L140

is swallowing the DOM.

@daliacoss
Copy link
Contributor

@karlcow do you think i should try to figure out why this is happening? or should that be saved for another issue?

@karlcow
Copy link
Member

karlcow commented Mar 21, 2016

Ok understood the issue. To have it work we really need to fix issues.js

issues.BodyView = Backbone.View.extend({
  el: $('.wc-Issue-report'),
  template: _.template($('#issue-info-tmpl').html()),
  initialize: function() {
    this.QrView = new issues.QrView({
      model: this.model
    });
  },
  render: function() {
    this.$el.html(this.template(this.model.toJSON()));
    // hide metadata
    $('.js-Issue-markdown')
      .contents()
      .filter(function() {
        //find the bare html comment-ish text nodes
        return this.nodeType === 3 && this.nodeValue.match(/<!--/);
        //and hide them
      }).wrap('<p class="is-hidden"></p>');
    this.QrView.setElement('.wc-QrCode').render();
    return this;
  }
});

I think we should solve this in this issue @deckycoss :)

What is happening is that the JavaScript is matching a textNode this.nodeType === 3
and which contains <!-- that will swallow everything which is part of this textNode

The bad thing is that it is pretty generic. Trying to break the thing I did
webcompat/webcompat-tests#342

Which in localhost is rendered as:

<div class="js-Issue-markdown wc-Markdown"><p class="is-hidden">
            &lt;!-- @browser: &lt;p&gt;Firefox 44.0&lt;/p&gt; --&gt;
&lt;!-- @ua_header: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:44.0) Gecko/20100101 Firefox/44.0 --&gt;
&lt;p class="boilerplate"&gt;foo&lt;/p&gt;
</p><p><strong>URL</strong>: <a href="http://foo.example.com/" rel="nofollow">foo.example.com/</a>&lt;iframe src="http://www.la-grange.net"/&gt;<br>
<strong>Browser / Version</strong>: &lt;p&gt;Firefox 44.0&lt;/p&gt;<br>
<strong>Operating System</strong>: Mac OS X 10.11<br>
<strong>Problem type</strong>: Something else - I'll add details below</p>
<p><strong>Steps to Reproduce</strong></p><p class="is-hidden">
&lt;!-- foo bar --&gt;
&lt;p&gt;
1) Navigate to: foo.example.com/&lt;iframe src="http://www.la-grange.net"/&gt;
2) …
&lt;/p&gt;
Expected Behavior:
</p><p>Actual Behavior:</p>

          </div>

We need to improve our sanitization process and have a better targetting at the comments.

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

No branches or pull requests

4 participants