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: allow popover child links to work #106391

Merged
merged 3 commits into from
Jan 5, 2023
Merged

rustdoc: allow popover child links to work #106391

merged 3 commits into from
Jan 5, 2023

Conversation

ardislu
Copy link
Contributor

@ardislu ardislu commented Jan 3, 2023

No need to prevent default click behavior on a <div>, it will also disable all child click behavior.

Closes #106390

No need to prevent default click behavior on a <div>, it will also disable all child click behavior.
@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@GuillaumeGomez
Copy link
Member

Do you want to add the GUI test too (in src/test/rustdoc-gui, there is a README file)? If not I'll add it once this PR is merged.

@ardislu
Copy link
Contributor Author

ardislu commented Jan 4, 2023

Sure, I gave it a shot.

Some notes:

  • I'm not familiar with this testing framework, apologies if I put the test in the wrong place.
  • I'm hardcoding the URL https://doc.rust-lang.org/rustdoc/ into the test, if this URL changes or redirects to a different one in the future then the test will fail.

@ardislu
Copy link
Contributor Author

ardislu commented Jan 4, 2023

My initial test case was getting this error:

(line 71) Error: Execution context was destroyed, most likely because of a navigation.: for command `assert-document-property: {"URL": "https://doc.rust-lang.org/rustdoc/"}`

I checked some Puppeteer issues and it seems like the proper way to write this test is to use Page.waitForNavigation(), however I'm not sure how to translate this method into the .goml file/testing framework.

I tried to look for some other test cases that navigated to external websites, but I couldn't find any (all other test navigations I could find only tested navigation within the rustdoc website).

Inserting a simple wait-for-document-property didn't work (same error):

wait-for-document-property: {"URL": "https://doc.rust-lang.org/rustdoc/"}

Waiting for generic HTML also didn't work (same error):

wait-for: "html"
wait-for: "body"

As a last resort workaround, I hardcoded a wait time of 2000ms and now it works. However, this is pretty fragile and not the best way to do it. I think this might be a limitation of the testing framework (ability to call Page.waitForNavigation() needs to get added somewhere). Am I missing something? Or maybe there's a better way to test a successful link navigation to an external website?

Appreciate any suggestions. Thank you!

@GuillaumeGomez
Copy link
Member

I actually had in mind to use wait-for-document-property. I'm surprised that the context issue is a problem in this case as well... For now let's keep it as is and then come back to it once we have the needed pieces.

Thanks for adding the GUI test in any case!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 4, 2023

📌 Commit 9792c9a has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#106391 (rustdoc: allow popover child links to work)
 - rust-lang#106398 (Fix a few clippy lints in libtest)
 - rust-lang#106412 (Fix link generation for local primitive types in rustdoc JSON output)
 - rust-lang#106437 (rustdoc: fix buggy JS check for absolute URL)
 - rust-lang#106451 (Merge borrowck permission checks)
 - rust-lang#106460 (Move tests)
 - rust-lang#106461 (docs: fix broken link "search bar")

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b41d81c into rust-lang:master Jan 5, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 5, 2023
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.

rustdoc: clicking a link inside the help popover does not work
4 participants