Notable traits popup position issue

[GuillaumeGomez]

When it's present on the item of the page (and too far on the right) and the width of the screen is too small, the popup breaks the page flow:

And when you scroll to see the popup content:

The best way to handle this would be to position it to the left instead of the right of the i depending of its width and i position, but I don't see a way to do it just using CSS. We also can't simply put it on the left by default because we might have the same issue but on the other side.

cc @Manishearth

[Manishearth]

My understanding is that there is a way to do this in CSS positioning but I don't really know it

[GuillaumeGomez]

I think there is a way in CSS as well, but I don't know it either.

[jsha]

I would like to propose we treat this as a reason to render this box using JS. Already this box causes us some HTML non-conformance since we have a div inside a <summary>, which can only contain "Phrasing content or one element of heading content". Also, it's duplicated a lot on some pages. I know we like to make things work without JS as often as possible but this seems like a case where requiring JS for the feature makes things easier and better. And it's still quite possible for the user to click on the type name to see what types it implements.

[Manishearth]

I'm fine with that. Perhaps we can try to make this work via HTML-attribute-set events instead of doing a for loop over all the elements and attaching listeners (which we used to do, wasn't great)

[GuillaumeGomez]

I had in mind to add a onclick callback (can be onhover too, we can discuss it on the PR I guess). Having loops is useful to generate new content, but I don't think it's a good fit to create new events. Or we can simply have a global "onclick" handler and if the target is the i element, we show it. Might be even simpler and would require much less new adds.

[GuillaumeGomez]

Thinking about what you said @jsha about duplication: do you mean having like an array objects for notable traits on each HTML page and the i button refers to the index into that array? Then in rustdoc, we would have a BTreeMap (to keep the insert order and the index too) and we would add it into it and get its index. I think I just answered my question. 😆 For now I'll keep the HTML as is when generating the JS array and we'll see how to improve it later on.

[jsha]

Perhaps we can try to make this work via HTML-attribute-set events instead of doing a for loop over all the elements and attaching listeners (which we used to do, wasn't great)

What was the not-great part? Performance? We currently have a loop over all <a> to set a handler, and another loop for all the toggles. I haven't noticed a performance impact but I also haven't profiled it. I'd like to avoid setting handlers in HTML attributes if we can.

about duplication: do you mean having like an array objects for notable traits on each HTML page and the i button refers to the index into that array?

I meant that often a single type is returned from many different methods, but the contents of the "Notable Traits" box are the same each time. So for instance, Vec<u8> is Write, so every method on https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.new that returns a Vec has a Notable Traits popup with the same HTML contents. We could save some page size by writing those HTML contents just once. So the JS array would have a mapping from type name -> HTML rather than i -> HTML. That said, I'm fine with doing i -> HTML for now if it's simpler, and we can do the page size optimization later.

I had in mind to add a onclick callback (can be onhover too, we can discuss it on the PR I guess).

I discussed the desired hover/click behavior here: #91100. In general I'm a fan of refactoring one thing at a time, which would mean keeping the current behavior (hover only) while doing this refactoring, and then as a follow-on adding a click behavior so things work on touchscreen devices.

[GuillaumeGomez]

Ok! My current work is click-based but since it's JS, it's pretty easy to change.

[Manishearth]

I don't think we should need a global array, we should still have the element be nearby in a way that's easy to find via js

[GuillaumeGomez]

It has a few advantages: no more duplicates and one file for a crate. The only thing I store in the DOM is the position in the array.

[Manishearth]

So we should definitely not have a single global onclick handler that handles dispatch. We used to do this for collapsing and it was a mess . This kind of thing ends up reimplementing event dispatch badly and other events tend to interact poorly with it.

Here's what I think we should do:

• we continue generating the notable trait HTML as we do today
• the notable trait icon gets an onclick="toggleNotable()" attribute. No other changes to the HTML
• hover and positioning CSS goes away
• toggleNotable() is able to look for the relevant notable dialog box element (it's probably a sibling or a child) and handle displaying it correctly

[jsha]

we should definitely not have a single global onclick handler that handles dispatch. We used to do this for collapsing and it was a mess . This kind of thing ends up reimplementing event dispatch badly and other events tend to interact poorly with it.

Agree.

we continue generating the notable trait HTML as we do today

This doesn't address the issue I raised earlier:

Already this box causes us some HTML non-conformance since we have a div inside a

, which can only contain "Phrasing content or one element of heading content".

One of the impacts of that issue is that tidy rejects our output, which is a blocker for having programmatic validation of our HTML.

It also doesn't save us any page size.

That said, I realize I'm violating the same principle I raised earlier, of refactoring one thing at a time. :-) So if we want to move the HTML generation as a separate issue, I'm fine with that.

the notable trait icon gets an onclick="toggleNotable()" attribute. No other changes to the HTML

I prefer not to use the onclick HTML attribute whenever possible. Mainly because it interacts poorly with CSP.

hover and positioning CSS goes away

I'm okay with this.

toggleNotable() is able to look for the relevant notable dialog box element (it's probably a sibling or a child) and handle displaying it correctly

In my preferred plan, toggleNotable() adds the relevant notable dialog box element to the DOM, from an HTML string stored somewhere else (i.e. not as a child of the <summary> node, and ideally once per type).

[Manishearth]

Yeah we can tweak the generation slightly to make it a sibling instead of a child

Regarding onclick; I meant mouseover (and whatever needed to make taps work(. What's the CSP interaction? Does it prevent us from using any attributes for events?

[jsha]

What's the CSP interaction? Does it prevent us from using any attributes for events?

Pretty much, yeah. See https://csp.withgoogle.com/docs/adopting-csp.html. You can allow them with unsafe-inline but that eliminates the main point of CSP.

[Manishearth]

Hmmm. In that case I guess we should do the for loop thing, but I'd still prefer to not have any kind of separate map or anything; either we include the stuff in data-foo attributes or we pregenerate the elements and display:none them.

[notriddle]

Fixed by #104177