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

Restructure URL rendering section and add additional guidance #434

Merged
merged 11 commits into from
Apr 4, 2019

Conversation

estark37
Copy link
Contributor

@estark37 estark37 commented Mar 15, 2019

This commit expands the URL rendering section to contain some of the
non-Chrome-specific guidelines from Chromium's URL display guidelines
(https://chromium.googlesource.com/chromium/src/+/master/docs/security/url_display_guidelines/url_display_guidelines.md).
The guidance is geared towards URLs that are rendered primarily in a
context in which a user is making a security decision.

The advice is separated into 3 sections: simplifying URLs to prevent
spoofing and confusion, eliding in space-constrained displays, and
i18n/bidi/special characters.


Preview | Diff

This commit expands the URL rendering section to contain some of the
non-Chrome-specific guidelines from Chromium's URL display guidelines
(https://chromium.googlesource.com/chromium/src/+/master/docs/security/url_display_guidelines/url_display_guidelines.md).
The guidance is geared towards URLs that are rendered primarily in a
context in which a user is making a security decision.

The advice is separated into 3 sections: simplifying URLs to prevent
spoofing and confusion, eliding in space-constrained displays, and
i18n/bidi/special characters.
@estark37
Copy link
Contributor Author

@annevk As we briefly discussed over email, this is an attempt to incorporate some of Chromium's general URL display guidelines into the URL rendering section. I'd be happy to open an issue to discuss first if that's preferred -- just thought it might be easier to have a specific proposal to work from.

cc @mikewest @emilyschechter

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for following up on this, this looks really good. I added some inline comments that I'd love your feedback on.

url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
@estark37
Copy link
Contributor Author

Thanks for the review @annevk and @domenic! Pushed a new commit to address your comments.

url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
@estark37
Copy link
Contributor Author

Thanks @jyasskin! Comments addressed.

Copy link
Contributor

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy edits

url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
Collapse www removal into the bullet about displaying only the host,
and list various simplifications as examples of what browsers may
choose to do to simplify the host (omit `www` or `m`, or show
registrable domain only).
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really happy with this. Big improvement over the status quo.

I left a lot of nits I'm happy to push as a fixup commit, but I left them as comments for now. And a couple final questions.

url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
url.bs Outdated Show resolved Hide resolved
@annevk annevk mentioned this pull request Mar 27, 2019
@annevk
Copy link
Member

annevk commented Mar 27, 2019

I pushed a commit with my final nits and filed a follow-up issue on domain labels. From my perspective I'm happy to merge this, but since lots of people have helped shaped this (thanks!) I'll give it a bit more time before doing so.

@estark37
Copy link
Contributor Author

estark37 commented Apr 3, 2019

Friendly ping @annevk

@estark37
Copy link
Contributor Author

estark37 commented Apr 3, 2019

(To be clear, that ping was about whether that can be merged now that there's been a chance for people to look it over again)

@annevk annevk merged commit 8809598 into whatwg:master Apr 4, 2019
@annevk
Copy link
Member

annevk commented Apr 4, 2019

Ah, thanks! I had both forgotten a bit due to travel, but I was also hoping you'd mention you're okay with my edits, which I'll assume you are now. 😊

Really appreciate the work you put into this and everyone that helped refine it here!

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

Successfully merging this pull request may close these issues.

8 participants