Bug 247196

Summary: [content-visibility] Make innerText take content-visibility: hidden into account
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, esprehn+autocc, ews-watchlist, kangil.han, mifenton, ntim, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 15   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 236238    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch rniwa: review+

Description Rob Buis 2022-10-28 07:57:54 PDT
[content-visibility] Make innerText take css content-visibility: hidden into account.
Comment 1 Rob Buis 2022-10-28 08:00:00 PDT
Created attachment 463295 [details]
Patch
Comment 2 Rob Buis 2022-11-01 11:00:03 PDT
Created attachment 463349 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2022-11-04 07:58:19 PDT
<rdar://problem/101959795>
Comment 4 Rob Buis 2022-11-30 07:17:01 PST
Created attachment 463808 [details]
Patch
Comment 5 Rob Buis 2022-12-16 02:24:39 PST
Created attachment 464074 [details]
Patch
Comment 6 Tim Nguyen (:ntim) 2022-12-16 09:50:47 PST
Comment on attachment 464074 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:487
> +            } else if (!isSkippedText(*renderer)) {

Isn't this just the same as `else {`? Can you even set `content-visibility: hidden` on a text node?
Comment 7 Ryosuke Niwa 2022-12-16 16:09:16 PST
Comment on attachment 464074 [details]
Patch

What happens if content-visibility: visible content appears within content-visibility: hidden. Would it also be hidden?
Comment 8 Tim Nguyen (:ntim) 2022-12-16 17:43:53 PST
(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 464074 [details]
> Patch
> 
> What happens if content-visibility: visible content appears within
> content-visibility: hidden. Would it also be hidden?

The spec says `content-visibility: hidden` is similar to `display: none` so I would assume it would be skipped.

https://w3c.github.io/csswg-drafts/css-contain/#content-visibility

Rob would know better than I would though.
Comment 9 Rob Buis 2022-12-17 13:22:51 PST
(In reply to Tim Nguyen (:ntim) from comment #8)
> (In reply to Ryosuke Niwa from comment #7)
> > Comment on attachment 464074 [details]
> > Patch
> > 
> > What happens if content-visibility: visible content appears within
> > content-visibility: hidden. Would it also be hidden?
> 
> The spec says `content-visibility: hidden` is similar to `display: none` so
> I would assume it would be skipped.
> 
> https://w3c.github.io/csswg-drafts/css-contain/#content-visibility
> 
> Rob would know better than I would though.

Ryosuke is correct, content-visibility: hidden overrides any value of content-visibility in its subtree (also note that 'visible' is the default for this property).
Comment 10 Rob Buis 2022-12-17 13:31:33 PST
Comment on attachment 464074 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:487
>> +            } else if (!isSkippedText(*renderer)) {
> 
> Isn't this just the same as `else {`? Can you even set `content-visibility: hidden` on a text node?

Not directly, but RenderText style goes through the parent style, so it is valid to test for effectiveSkipsContent. I need to do more testing with this patch though to see how much of this patch is actually needed. I am also wary of the effect this has on content-visibility WPT tests and it rendering some text as blank and giving output that gives no clue to the issuer. To that end I am fixing those tests (like https://github.com/web-platform-tests/wpt/pull/37552) but I have to see how many need to be adjusted.
Comment 11 Rob Buis 2022-12-18 08:33:47 PST
Pull request: https://github.com/WebKit/WebKit/pull/7828
Comment 12 EWS 2022-12-21 06:41:28 PST
Committed 258187@main (63fdf6b4ce81): <https://commits.webkit.org/258187@main>

Reviewed commits have been landed. Closing PR #7828 and removing active labels.