Bug 181640 – Support dynamic pseudo-classes on elements with display: contents

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Pavel Klimashkin]

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.

[Radar WebKit Bug Importer]

<rdar://problem/36605415>

[Antti Koivisto]

:hover and :active

[Antti Koivisto]

The bug here is that hover/active style doesn't get invalidated on display:contents elements.

[Antti Koivisto]

Created attachment 332029 [details]
patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antti Koivisto]

Created attachment 332062 [details]
patch

[Alexey]

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

[Ryosuke Niwa]

Antti, is this patch still valid? Not sure why we ended up not landing this patch.

[Antti Koivisto]

(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.

[Antti Koivisto]

Created attachment 354330 [details]
rebased patch

[Dean Jackson]

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.

[Antti Koivisto]

> 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.

[WebKit Commit Bot]

Comment on attachment 354330 [details]
rebased patch

Clearing flags on attachment: 354330

Committed r238097: <https://trac.webkit.org/changeset/238097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Klimashkin]

Thanks! In what release that patch will probably land?

[Ryosuke Niwa]

(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/