Bug 181640

Summary: Support dynamic pseudo-classes on elements with display: contents
Product: WebKit Reporter: Pavel Klimashkin <klimashkin>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: as.avramenko, cdumez, commit-queue, dino, ews-watchlist, koivisto, rego, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=181680
Attachments:
Description Flags
patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
patch
none
rebased patch none

Description Pavel Klimashkin 2018-01-14 15:56:25 PST
Pseudo-classes don't work on element with display: contents in TP 47, but they should.

You can check demo here:
https://codepen.io/klimashkin/pen/LepyLb?editors=1100#0
Cells in table should change style on row hover.

It works correctly in Firefox and Chrome 65+, but not in Safari TP 47.
Comment 1 Radar WebKit Bug Importer 2018-01-17 18:52:12 PST
<rdar://problem/36605415>
Comment 2 Antti Koivisto 2018-01-18 01:37:18 PST
:hover and :active
Comment 3 Antti Koivisto 2018-01-18 01:39:21 PST
The bug here is that hover/active style doesn't get invalidated on display:contents elements.
Comment 4 Antti Koivisto 2018-01-23 07:47:10 PST
Created attachment 332029 [details]
patch
Comment 5 EWS Watchlist 2018-01-23 09:21:04 PST
Comment on attachment 332029 [details]
patch

Attachment 332029 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6182997

New failing tests:
fast/css/display-contents-hover-active.html
Comment 6 EWS Watchlist 2018-01-23 09:21:05 PST
Created attachment 332031 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 7 Antti Koivisto 2018-01-23 12:27:54 PST
Created attachment 332062 [details]
patch
Comment 8 Alexey 2018-11-08 04:25:53 PST
Hi from 08.11.2018.
This bug is still not fixed.

Demo: http://jsfiddle.net/6208eox5/
Safari version: 11.1.2 build 13605.3.8
Comment 9 Ryosuke Niwa 2018-11-08 18:46:30 PST
Antti, is this patch still valid? Not sure why we ended up not landing this patch.
Comment 10 Antti Koivisto 2018-11-09 04:30:02 PST
(In reply to Ryosuke Niwa from comment #9)
> Antti, is this patch still valid? Not sure why we ended up not landing this
> patch.

The patch is valid, feel free to review. 

It was slightly risky to land at that point due to switch to DOM based traversal and then got forgotten. Also I got sidetracked to 181680, :active was broken in general.
Comment 11 Antti Koivisto 2018-11-09 05:09:19 PST
Created attachment 354330 [details]
rebased patch
Comment 12 Dean Jackson 2018-11-12 10:13:00 PST
Comment on attachment 354330 [details]
rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354330&action=review

> Source/WebCore/dom/Element.cpp:2920
> +    if (!hasRareData())
> +        return nullptr;
> +    auto* style = elementRareData()->computedStyle();
> +    if (style && style->display() == DisplayType::Contents)

Could you have

if (!hasRareData() || !hasDisplayContents())
  return nullptr;

return elementRareData()->computedStyle();

Either way is fine with me.
Comment 13 Antti Koivisto 2018-11-12 10:34:17 PST
> Could you have
> 
> if (!hasRareData() || !hasDisplayContents())
>   return nullptr;
> 
> return elementRareData()->computedStyle();
> 
> Either way is fine with me.

This is slightly more efficient (in principle, meaningless in practice) since hasDisplayContents() needs to fetch the computed style from rare data too.
Comment 14 WebKit Commit Bot 2018-11-12 10:55:26 PST
Comment on attachment 354330 [details]
rebased patch

Clearing flags on attachment: 354330

Committed r238097: <https://trac.webkit.org/changeset/238097>
Comment 15 WebKit Commit Bot 2018-11-12 10:55:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Pavel Klimashkin 2018-11-12 12:29:35 PST
Thanks! In what release that patch will probably land?
Comment 17 Ryosuke Niwa 2018-11-12 15:43:35 PST
(In reply to Pavel Klimashkin from comment #16)
> Thanks! In what release that patch will probably land?

Unfortunately, Apple does not comment on future produce release plans so we can't say. But you can usually see up-coming fixes & features in Safari Technology Preview way before it gets released to public: https://developer.apple.com/safari/technology-preview/