Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Editorial review: Document popover="hint" #37990
base: main
Are you sure you want to change the base?
Editorial review: Document popover="hint" #37990
Changes from 2 commits
7363db0
c7749da
7668413
ca545ad
fb17a7c
7564a0b
49d5cd0
231d6e9
a9e5903
e4e4204
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possibly too restrictive language, the source can be any HTMLElement it doesn't have to be just a link or a button. Though those will be the common cases.
It doesn't even have to be the element that was clicked/hovered/etc to trigger showPopover() to be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. In my next commit, I've updated it to just say "...that is, its control element"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also enables the nested popover behaviour that popovertarget/commandfor create which is worth a mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably link to the nested popover section easy enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to leave the commandfor stuff to another PR, if that's OK. The scope creep on this one is big enough already ;-)
Can you file an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also should do a focus order fixup to put the popover next in the tab order from the source element. This is currently not implemented in Chromium. As such the Browser Compat Data should maybe be marked as a partial implementation? cc @mfreed7 to see what he thinks regarding this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mdn/browser-compat-data#25964 - I made a BCD PR to add Safari Tech Preview as partial (due to same bug) and also marked chrome as partial. Happy to change if people think it shouldn't be partial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/mdn/content/pull/37990/files#r1955372659
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisdavidmills I'm closing all these, but once you've looked at https://github.com/mdn/content/pull/37990/files#r1958808280 you might want to link these to a fixed up section in the Using doc as an intermediate step to getting to the CSS anchor positioning doc.
I.e. "Associating anchor and positioned elements for more details on anchor references." is very general, whereas a specific example for popover hints would be very specific and make things a lot easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, in my next commit, I have updated all of these bits to briefly explain all of the useful additional effects, and link to the relevant sections in the "Using..." guides. I don't want to repeat the entire explanation on each page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the policy is on documenting stuff that only applies in the middle stage where some browsers support hint but others don't. But it might be worth pointing out that popover="hint" will fallback to a manual popover in unsupporting browsers rather than auto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a quick note in my next commit. I think it's worth including, and we can always remove it when all modern browsers catch up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, you're saying that the implicit positioning is already in place, so you don't need to worry about it - a hint will appear in the right place above its associated element?
The first sentence doesn't quite say that "This makes it very convenient" - i.e. it sounds like something you have to do, and that you ware about to describe.
The second sentence says "for an example that uses this implicit association see ... you might say "You can see this implicit associate at work in the exmaple XXX: no special CSS had to be created to position the hints near their associated elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the association is implicit, not the positioning. The implicit association means you don't need to create an explicit anchor reference, for example using the
anchor-name
andposition-anchor
properties.You still need to do the anchor positioning explicitly, for example, using the
anchor()
function as values of inset properties, or theposition-area
property.I've not made much of a change here, except that I've added a link to the relevant section of the main anchor positioning guide that explains this association, and how the actual positioning is a separate step.
I've also added this link to the other pages that mention the implicit association, as I thought it would be helpful to clear up confusion.
Let me know if you think this needs anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisdavidmills Thanks for explaining - I get it now, but I can completely see why I made this assumption - you show no CSS.
The linked section in "For an example that uses this implicit association, see the Using "hint" popover state section above." does not provide any example. Nor does the rest of the code. Everything says there are implicit links and nothing shows how you style them.
I don't think it is enough to link to the generic styling section. We need a real example. Something like this, copied from the example source code, with explanation:
I would also rename https://pr37990.content.dev.mdn.mozit.cloud/en-US/docs/Web/API/Popover_API/Using#implicit_popover_anchor_associations to "Styling hints" - but that's up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think styling hints makes sense as a section name fwiw, because you get implicit anchoring for auto and manual popovers too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I understand the problem now! ;-)
So, in my next commit, I have overhauled this section, explaining that you still need to add the positioning CSS code, and showing examples of what this looks like.
I have also removed the link to the earlier section about
popover="hint"
, and just linked straight to the code example on GitHub for an actual real example, so they can just jump into the code and check it out themselves.I think it is better to not link between these two sections, as it creates a weird circular dependency that is not really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also added a new section earlier on in the article called "Popover accessibility features", which explains how popover/invoker relationships result in useful ARIA relationships and focus order updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear you don't get the aria when the invokers are done via JS because it can be any element and we don't know that it makes sense for a given element. But you should get the focus fixup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, perfect, thanks @lukewarlow; I've adjusted my explanation to suit.