-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add cssom-view tests for display contents #55253
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
base: master
Are you sure you want to change the base?
Conversation
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.
The display inline tests I'm not sure are correct at least. It might be worth aligning the text as well on the display: contents
ones, or click on 5, 5
or so rather than 0, 0
?.
These found some interop bugs in various browsers.
5be945f
to
82ee032
Compare
<body> | ||
<div id="targetDiv">Text Content</div> | ||
<script> | ||
var element = document.elementFromPoint(5, 5); |
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.
So... Thinking a bit more about this, I think both returning targetDiv
and body
might be reasonable? Specially once you consider shadow DOM behavior / slots. You might want to get the element that a click event would be dispatched to, if you hit a text node...
If the spec is unclear, I suggest landing these with .tentative.html
and pointing to a CSS spec issue?
I agree in any case that document.elementsFromPoint(x, y).includes(document.elementFromPoint(x, y))
should be true. Maybe add a non-tentative test checking that?
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.
To me the spec is actually quite clear, and to me it clearly says that display:contents elements shouldn't be included.
https://www.w3.org/TR/cssom-view-1/#dom-document-elementfrompoint
If there is a box in the viewport that would be a target for hit testing at coordinates x,y...
display:contents elements don't produce a box and so can't be included?
The same language is true for elementsFromPoint too. If the use case is retargeting like events do we should probably just expose that function separately?
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.
Well, the point is that the text node does have a box. So the question is whether you retarget the text node to the display: contents element or just skip it.
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.
Well isn't text in the text node actually a text sequence?
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.
Well, sure, but those should be included. Otherwise per spec if you hit a text sequence rather than a box you should return the root element from elementFromPoint
, which doesn't seem like the intention, right?
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.
Well would the nearest parent that has a box not match this? E.g. body? That would be a box that's targeted by that hit test right? I guess because hit testing is undefined it's hard to know.
I'll open a spec issue to discuss 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.
These found some interop bugs in various browsers.
elementFromPoint-display-contents.html - Fails in Chromium, WebKit, and Firefox, and Servo.
elementsFromPoint-display-contents.html - Fails in WebKit. Passes in Chromium, Firefox and Servo.