Summary: | Inert subtrees fail to block activation from contained svg elements | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Hite <danhite> | ||||||||
Component: | Layout and Rendering | Assignee: | Tim Nguyen (:ntim) <ntim> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, changseok, clopez, dino, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, koivisto, kondapallykalyan, mmaxfield, ntim, pdr, sabouhallawa, schenney, sergio, simon.fraser, webkit-bug-importer, youennf, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
See Also: | https://github.com/web-platform-tests/wpt/pull/32940 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 84635, 165279 | ||||||||||
Attachments: |
|
Description
Dan Hite
2022-01-28 14:18:49 PST
(In reply to Dan Hite from comment #0) > I just got ipadOS 15.4beta 1 and was excited to try the new "inert" > idl/attribute > which worked as expected wrt the spec (good work, very cool!!); > https://html.spec.whatwg.org/multipage/interaction.html#inert > > however the spec itself has a blindspot in that it refers to ~just html > elements, ignoring the > <svg> elements integration within html pretty sure from a spec POV it applies to the entire subtree, regardless of whether they're HTML or SVG or MathML or anything else > > that is, on very first page I tried testing inert, which describes the inert > attribute: > https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert > > ^ then setting document.body.inert = true > Immediately I found an oops-isn't-actually-inert misfeature when tapping > around the (inert) page suddenly nav'ed me > > you see, MDN does their upper left logo/link html <a> as having an <svg> > graphic within it > > if you replace the innerHTML of their <a> with text (ie html not svg) then > the page was satisfactorily inert > > but your hit-test-event-dispatch code for a tap within the svg apparently > doesn't follow your new html inert logic > since it dispatches and bubbles into the html <a> and thus navs > > > if the inert feature, in future, were in widespread use, then this spec foo > could be a minor security issue, as page authors might > assume they'd locked down ui on a piece sanitized html, but suddenly a > simplistic attack gets an activation ala > <svg><a ... That said, this does sound like a bug in WebKit… Seems like this affects inertness created by modal dialogs as well, not just the inert attribute. Created attachment 450893 [details]
Testcase using modal dialogs
Looks like SVG has its own pointerEvents checks: https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/WebKit/WebKit%24+pointerEvents%28%29&patternType=literal Created attachment 452854 [details]
Patch
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32940 This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Comment on attachment 452854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452854&action=review > Source/WebCore/ChangeLog:9 > + Re-using visibleToHitTesting in SVG code isn't possible, because visibleToHitTesting removes visibility: hidden; > + content from hit-testing, which we do not want to here, since pointer-events has values for SVG which still That's pretty silly. SVG spec really makes invisible things hittable? > Source/WebCore/rendering/style/RenderStyle.h:716 > + PointerEvents pointerEventsIncludingInert() const { return effectiveInert() ? PointerEvents::None : pointerEvents(); } I think you proposed effectiveUserSelect() earlier but we then went with *IncludingInert instead. I think you were right and we should just go with effective* with all these (it reads better and is more expandable), wether backed by a fake property or not. Comment on attachment 452854 [details]
Patch
visibleToHitTesting() should likely also use the new effective getter. That would fix an apparent bug too where ignoreCSSPointerEventsProperty is not respected for inert. Looking at the call site, it is used for color sampling which should happen on inert elements too (it could use renaming).
(In reply to Antti Koivisto from comment #9) > Comment on attachment 452854 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452854&action=review > > > Source/WebCore/ChangeLog:9 > > + Re-using visibleToHitTesting in SVG code isn't possible, because visibleToHitTesting removes visibility: hidden; > > + content from hit-testing, which we do not want to here, since pointer-events has values for SVG which still > > That's pretty silly. SVG spec really makes invisible things hittable? As surprising as it is, yes: https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events#svg_only_experimental_for_html There are the visibilePainted/visibleFill/visibleStroke/visible values to omit visibility: hidden elements. (In reply to Antti Koivisto from comment #10) > Comment on attachment 452854 [details] > Patch > > visibleToHitTesting() should likely also use the new effective getter. That > would fix an apparent bug too where ignoreCSSPointerEventsProperty is not > respected for inert. Looking at the call site, it is used for color sampling > which should happen on inert elements too (it could use renaming). Sounds good, will change that then. Created attachment 452858 [details]
[fast-cq] Patch
Committed r290306 (247629@trunk): <https://commits.webkit.org/247629@trunk> This fix shipped with Safari 15.5 (all platforms). |