Closed Bug 911169 Opened 7 years ago Closed 7 years ago
Thimble needs to warn users when there are http links in https document
62 bytes, text/plain
I've experienced this myself and in the workshop I hosted last night. If users want to link to an http document (like google fonts - the embed instructions use an http link), it doesn't work when you are working on an Https page (until you change the link url also to https). Since it is such a simple thing to fix, but is a problem that would really stump beginners, is there a way to set this as a warning, or somehow let them know why it isn't working (even though they are following the google font instructions perfectly)?
:Pomax, this looks like it might make for a good student bug, maybe have these errors pop up as slowparse warnings or errors? Thoughts?
jbuck: that sounds like a great idea. Let's add it as a place for students to jump in.
I would like to make a try, can somebody give me a starting point?
There's a few ways to do this, the most interesting is probably to add a new error case to the slowparse library and add a special handler for the "href" attribute (which will implicitly catch <a> and <link> elements), https://github.com/mozilla/slowparse/blob/gh-pages/slowparse.js#L1228
note: parse errors are define on https://github.com/mozilla/slowparse/blob/gh-pages/slowparse.js#L98 and on, and you would probably add your own code after https://github.com/mozilla/slowparse/blob/gh-pages/slowparse.js#L1253, which is where the code is technically valid HTML and so might be a legal (but mixed-content) href="..." pair.
code is ready, will setup local thimble to test it
After bug fixing, shall I build the package locally to make it work? how to build it?
Now start to test the basic function (locate href="http://..." and trigger error), but still one question, how to run thimble in a local https environment.
We brought this bug up in a team meeting today and would love to ship for our mozfest milestone. Adding Pomax to help with local install questions
Assignee: nobody → pomax
Whiteboard: uxdesign, → uxdesign, mozfest
Note that dchen has been working on this, so maybe Pomax wants to co-ordinate vs. take.
sounds good, dchen, how far did you get?
Hi, Mike I'm still working on it. The http links can be located, but trigger ParseError failed. If you have time, please have a look at my repo: https://github.com/xdchen/slowparse/commit/3b3146bc8188d907a4c534bee28e64feb393675d
update the warning trigger with tag-attributes pairs (like link-href, a-href, iframe-src etc). https://github.com/xdchen/slowparse/commit/15ca7d8281765c1fc10af3f21f4cbd48db6e0cc7
if you make this a pull request, and set me to r?, I can leave code comments.
Bug fixed, please review the pull request. https://github.com/mozilla/slowparse/pull/56
xdchen, you need to flag someone for review. I've done it for you, but you can do it yourself next time. Great work on this patch!
Attachment #818179 - Flags: review?(pomax)
Thank you for the help, David.
Comment on attachment 818179 [details] [review] https://github.com/mozilla/slowparse/pull/56 pretty nice solution, although we're missing a global check to see if slowparse itself is being run on http or https (if it's on http, we don't need to do mixed content checking). I left some comments in the pull request, set me back to R? when you've addressed then and I'll rereview =)
Attachment #818179 - Flags: review?(pomax) → review-
Hi, Mike For the https status, I just added the checkMixedContent check follow your suggestion, but I can not test it locally on http://localhost:3500/. Please have a review.
Attachment #818647 - Flags: review?(pomax)
I'll have a look, but you don't have to make a new attachment when you want a rereview, just go into the "details" link for the original attachment, and change the review flag from "-" to "?" and add the reviewer again. This also lets you add a comment for the details you changed.
Attachment #818647 - Flags: review- → review?(pomax)
Attachment #818647 - Flags: review?(pomax) → review+
Another question, shall we add (img/src) into isActiveContent?
Please review the pull request https://github.com/mozilla/slowparse/pull/57, and remove the last one (56), thank you
Attachment #819241 - Flags: review?(pomax)
images are not active content.
ok, I will remove img-src
Comment on attachment 819241 [details] [review] Rework on a new clean branch test and works pretty nicely! I think we need to change the error message to be more informative on what's gone wrong and how users can fix it (suggested text is in the PR), but with that changed, this can land.
Attachment #819241 - Flags: review?(pomax) → review-
Comment on attachment 819241 [details] [review] Rework on a new clean branch error message updated.
Attachment #819241 - Flags: review- → review?(pomax)
Comment on attachment 819241 [details] [review] Rework on a new clean branch very nice work!
Attachment #819241 - Flags: review?(pomax) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
merged into slowparse, but thimble does not use slowparse as submodule anymore, so we'll need to update the slowparse.js and the errors.base.html files in the thimble.webmaker.org repository, too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If we can get this done in Thimble before Monday at 6pm, we can ship this for Mozfest. It would be great if that were possible. This is a great fix, thanks for working on it.
updated for slowparse in thimble
@humph: yeah this is going to pop us so much at MozFest, having it in will make everyone's experience a lot better!
Comment on attachment 819494 [details] https://github.com/mozilla/thimble.webmaker.org/pull/259/files What an amazing patch!! R+ - อาร์พลัส
Attachment #819494 - Flags: review?(ali) → review+
Commit pushed to master at https://github.com/mozilla/thimble.webmaker.org https://github.com/mozilla/thimble.webmaker.org/commit/d0d16e099c05bfeb165f111279a9887a60056cbd Merge pull request #259 from Pomax/bug911169 in the mixed content world, we are now all winners!
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Thank you Mike and David, I learned a lot from you during the process.
(In reply to Mike "Pomax" Kamermans [:pomax] from comment #31) > merged into slowparse, but thimble does not use slowparse as submodule > anymore, so we'll need to update the slowparse.js and the errors.base.html > files in the thimble.webmaker.org repository, too. shall I do it?
Yeah, if you could get that patch ready that would be awesome
I didn't notice that :pomax have done the merge in thimble.webmaker.org repository as pull request 259, and duplicated the works in pull request 261. so please cancel it: https://github.com/mozilla/thimble.webmaker.org/pull/261
You need to log in before you can comment on or make changes to this bug.