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

rustdoc: Show nag box on IE11 #84340

Merged
merged 2 commits into from
Apr 20, 2021
Merged

rustdoc: Show nag box on IE11 #84340

merged 2 commits into from
Apr 20, 2021

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Apr 19, 2021

Rustdoc doesn't work on IE11. It's been broken for months, it isn't supported by the tiered browser support list, it's even more severely broken on other Rust websites, and IE11 doesn't support the <details> tag that we want to use.

In the interest of honesty, let's give an actual error message for anyone on IE11.

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2021
@GuillaumeGomez
Copy link
Member

Wouldn't it be simpler to fix the forEach call instead?

@notriddle
Copy link
Contributor Author

I can fix this, but I don't think I can make it stay fixed. The only policy the Rust project has is the RFC that explicitly doesn't promise to support IE11. The current, stable release of the standard library docs are not working in IE11, and nobody opened an issue for it. I was checking another PR to see if it broken IE11, and then noticed that IE11 had been broken for months without anybody noticing.

Discourse, the forum software used for users.rust-lang.org, dropped support for IE11 last year. GitHub doesn't support IE11 either, dropped years ago. Microsoft themselves are dropping IE11 support in Office 365 in a few months, and already heavily push IE11 users towards Edge.

This is what the rust-lang.org front page looks like. I can't actually click the Documentation link: it's covered by the logotype.

image

And this is The Book. The sidebar is inaccessible, and the Previous Page link is covered by the Next Page link, so I cannot switch to the previous page at all.

image

I really don't think anybody is using rustdoc with IE11. I just want to give anybody who does stumble upon it actionable advice, instead of just a page that inexplicably doesn't load.

@GuillaumeGomez
Copy link
Member

I'd argue in the sense that documentation should be available to the most users possible. :)

Can you check what would need to be changed to be fixed? I thought checking ES5 compatibility was enough but I guess I was wrong... Maybe we should add another check. If you know of any, it'd be very useful.

@notriddle
Copy link
Contributor Author

I'd argue in the sense that documentation should be available to the most users possible. :)

I agree, but I would rather show an actual error message to people than docs that silently refuse to load.

I don't want to put a bunch of work into supporting IE11 when nobody actually uses it. Everybody who really is stuck on IE11 has already been filtered out because they can't read The Book, can't download the toolchain, and can't ask for help.

Can you check what would need to be changed to be fixed?

It's trying to call forEach on a DOM list. To fix this, use the onEachLazy function instead.

@GuillaumeGomez
Copy link
Member

Oh wow. No DOM list should ever be used in forEach. So it's definitely gonna be worth a fix in any case. I still have cold sweat thinking about fixing that chrome bug a few years ago in rustdoc...

@notriddle
Copy link
Contributor Author

I don't disagree with that, but I still think we should encourage IE11 users to switch to Edge. It is obvious that IE11 in rustdoc is not being tested. We should be upfront about this.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 19, 2021

It's not our place to do so. If at some point it becomes too annoying to support it, then we'll simply do so. Until then, we should be fine. :)

EDIT: I mean, having it as a "best-effort" thing.

@notriddle
Copy link
Contributor Author

If the mention of MSEdge was removed, then would this PR be suitable to merge?

@GuillaumeGomez
Copy link
Member

Still no (unless the bug cannot be fixed of course, but then, please remove the edge mention).

@workingjubilee
Copy link
Member

workingjubilee commented Apr 19, 2021

While Microsoft is not removing Microsoft Internet Explorer 11 from their support per se as it is governed by their lifecycle policy and thus Microsoft's own rules forbid them from dropping it until a Windows 11 is released, Microsoft is terminating all extension support from MS365 applications and has even been sunsetting EdgeHTML support.

I do not believe accessibility is merely a monotonic feature of software that is simply "supporting more clients is more better", even though I do prefer supporting more. Notably, IE11 has the weakest ARIA support of all browsers and even has incomplete HTML5 support. This means that gating improvements to rustdoc's display on IE11 support means disallowing improvements that could notably break compatibility with IE11, and thus means disallowing improvements that actually make it more accessible for users who cannot e.g. see well, and use TTS to read the page.

It is my belief that if the desire is to improve accessibility, then focusing on improving rustdoc views on mobile devices (which are, right now, overwhelmed by a huge DOM size and use of JS where slightly more modern HTML and CSS would do), and its usability for text-to-speech (as there are many programmers who use such) seems more pertinent.

Anyways, MDN recommends for the case of forEach on a NodeList, for IE support, a pattern like the following snippet, which actually allows forEach because it explicitly uses the Array prototype.

const list = document.querySelectorAll('input[type=checkbox]');
Array.prototype.forEach.call(list, function (checkbox) {
  checkbox.checked = true;
});

@notriddle
Copy link
Contributor Author

Also, IE11 doesn't support <details>. I was all ready to start implementing a JavaScript polyfill, but then I noticed it wasn't working anyway.

@GuillaumeGomez
Copy link
Member

For the JS side, it's actually not linked to IE11 support but mostly around rustdoc search and toggle handling (and that part is getting removed currently, full HTML!). A lot of work has been done there to make it much faster and I expect to continue on that way. The next rustdoc releases will make a huge difference in that regard (on page load time more precisely).

As for the forEach, we actually have a forEachLazy function in rustdoc for DOM iterators that I added because it was taking more than 10 secs (!!!) on chrome to load some pages a few years ago. The reason was simply that every time we accessed an index of the iterator, chrome was recomputing the whole iterator. To prevent this, we clone the iterator (which is pretty light considering it's an array of IDs so to speak) and iterate over that new array.

@GuillaumeGomez
Copy link
Member

Also, IE11 doesn't support <details>. I was all ready to start implementing a JavaScript polyfill, but then I noticed it wasn't working anyway.

Oh, that is a good point. Then please do two things:

  1. Update the message from IE8 to IE11 (and nothing else, so please remove the rest)
  2. Replace the forEach that shouldn't be there in the first place.

@notriddle notriddle force-pushed the patch-4 branch 2 times, most recently from 345a0e4 to 72d5817 Compare April 19, 2021 20:39
Rustdoc doesn't work on IE11. It's been broken for months, it isn't supported
by the [tiered browser support list], it's even more severely broken on other
Rust websites, and IE11 doesn't support the `<details>` tag that we want
to use.

In the interest of honesty, let's give an actual error message for anyone
on IE11.

[tiered browser support list]: https://github.com/rust-lang/rfcs/blob/master/text/1985-tiered-browser-support.md
@notriddle
Copy link
Contributor Author

@GuillaumeGomez

Okay, both of these are now done.

@GuillaumeGomez
Copy link
Member

@notriddle Just one little update and we're good to go, thanks!

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2021
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Apr 20, 2021

📌 Commit 20c3627 has been approved by GuillaumeGomez

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 20, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 20, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2021
Rollup of 4 pull requests

Successful merges:

 - rust-lang#84337 (Clarify the difference between insert and get_or_insert)
 - rust-lang#84340 (rustdoc: Show nag box on IE11)
 - rust-lang#84345 (Remove comment about doc hack.)
 - rust-lang#84347 (rustdoc: Simplify some document functions)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5cc3994 into rust-lang:master Apr 20, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 20, 2021
@notriddle notriddle deleted the patch-4 branch April 20, 2021 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants