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

Section 23 does not expand when clicked. #1789

Closed
ursi opened this issue Nov 20, 2019 · 10 comments · Fixed by tc39/ecmarkup#194
Closed

Section 23 does not expand when clicked. #1789

ursi opened this issue Nov 20, 2019 · 10 comments · Fixed by tc39/ecmarkup#194
Assignees
Labels
rendering bug A bug in the rendering of the spec on the web.

Comments

@ursi
Copy link

ursi commented Nov 20, 2019

this is with the version hosted at https://tc39.es/ecma262
Doesn't Work: Brave 1.1.1
Doesn't Work: Chrome 78.0.3904
Works: Firefox 70.0.1

@ljharb
Copy link
Member

ljharb commented Nov 20, 2019

Works in Safari as well, and only that section seems to not expand in Chrome.

@ljharb ljharb added the rendering bug A bug in the rendering of the spec on the web. label Nov 20, 2019
@jmdyck
Copy link
Collaborator

jmdyck commented Nov 22, 2019

I tried it in Chrome 75.0.3770, worked for me.

@bakkot
Copy link
Contributor

bakkot commented Nov 22, 2019

The issue I observe is that scrolling to section 23 (either manually, by clicking the link in the sidebar, or by clicking a link such as this one) does not cause that section to expand in the sidebar.

I expect this is because that section has oldids="sec-keyed-collection", which causes a <span id="sec-keyed-collection"></span> tag to be inserted at the beginning of the section. Ecmarkup relies on the margin-top of the first child of a clause as a part of figuring out which clause you are within. Normally that's the <h1> containing the section name, which has a fairly large margin, but here it's the <span>, which has no margin at all. Since clicking on the anchor puts you very slightly above the clause element (a fraction of a pixel, in my browsers), ecmarkup does not recognize you as being within the clause.

That said, I observe this in Firefox and Safari as well as Chrome, so maybe that's not the issue described here. @ljharb or @ursi, can you describe the issue you see more precisely?

@ursi
Copy link
Author

ursi commented Nov 22, 2019

@bakkot I'm not at all familiar with the code that makes up the spec. Is there anything in particular you want?

@bakkot
Copy link
Contributor

bakkot commented Nov 22, 2019

@ursi I am interested in what bugged behavior you observe. Is it that clicking on "23 Keyed Collections" in the sidebar scrolls you to that section but does not expand it in the sidebar? If so, do you observe that scrolling the main page down slightly does cause it to expand in the sidebar?

@ursi
Copy link
Author

ursi commented Nov 22, 2019

@bakkot when click I am scrolled to the section, however the section is not expanded on the sidebar. If I scroll down by even a pixel (document.scrollingElement.scrollTop++) it will expand.

@rkirsling
Copy link
Member

I can confirm that deleting the <span> or removing oldids and rebuilding fixes the issue.
(Explicit height: 0; or visibility: hidden; or even display: none; doesn't help.)

FWIW, in addition to the originally-suggested results, I also repro this in STP 96.

@ljharb
Copy link
Member

ljharb commented Nov 22, 2019

Seems like "relies on the margin-top of the first child of a clause" in ecmarkup is the brittle piece; can that be made more robust?

@bakkot
Copy link
Contributor

bakkot commented May 8, 2020

Reopening this until ecmarkup is upstreamed.

@bakkot bakkot reopened this May 8, 2020
@bakkot
Copy link
Contributor

bakkot commented May 20, 2020

This is fixed now that ecmarkup has been upstreamed in #1995.

@bakkot bakkot closed this as completed May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rendering bug A bug in the rendering of the spec on the web.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants