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+

Rob Buis
Reported 2022-10-28 07:57:54 PDT
[content-visibility] Make innerText take css content-visibility: hidden into account.
Attachments
Patch (1.72 KB, patch)
2022-10-28 08:00 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (6.22 KB, patch)
2022-11-01 11:00 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (5.48 KB, patch)
2022-11-30 07:17 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (5.51 KB, patch)
2022-12-16 02:24 PST, Rob Buis 		rniwa: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Rob Buis
Comment 1 2022-10-28 08:00:00 PDT
Created attachment 463295 [details] Patch
Rob Buis
Comment 2 2022-11-01 11:00:03 PDT
Created attachment 463349 [details] Patch
Radar WebKit Bug Importer
Comment 3 2022-11-04 07:58:19 PDT
<rdar://problem/101959795>
Rob Buis
Comment 4 2022-11-30 07:17:01 PST
Created attachment 463808 [details] Patch
Rob Buis
Comment 5 2022-12-16 02:24:39 PST
Created attachment 464074 [details] Patch
Tim Nguyen (:ntim)
Comment 6 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?
Ryosuke Niwa
Comment 7 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?
Tim Nguyen (:ntim)
Comment 8 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.
Rob Buis
Comment 9 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).
Rob Buis
Comment 10 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.
Rob Buis
Comment 11 2022-12-18 08:33:47 PST
Pull request: https://github.com/WebKit/WebKit/pull/7828
EWS
Comment 12 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.
Note You need to log in before you can comment on or make changes to this bug.