Bug 233343

Summary: Add a basic heuristic for sizing text in image overlay blocks
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, cdumez, esprehn+autocc, ews-watchlist, hi, kangil.han, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Fix unused param
thorton: review+
Rebase on trunk
ews-feeder: commit-queue-
Fix unused param none

Description Wenson Hsieh 2021-11-18 16:24:07 PST
.
Comment 1 Wenson Hsieh 2021-11-18 18:38:04 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-11-18 19:39:07 PST
Created attachment 444773 [details]
Fix unused param
Comment 3 Radar WebKit Bug Importer 2021-11-25 16:25:23 PST
<rdar://problem/85755755>
Comment 4 Wenson Hsieh 2021-11-28 23:04:02 PST
*** Bug 233111 has been marked as a duplicate of this bug. ***
Comment 5 Tim Horton 2021-11-29 11:17:42 PST
Comment on attachment 444773 [details]
Fix unused param

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

> Source/WebCore/dom/ImageOverlay.cpp:306
> +        rootContainer->setTranslate(false);

Seems like a nontrivial behavior change that should be in its own patch?

> Source/WebCore/dom/ImageOverlay.cpp:536
> +        document->updateLayoutIgnorePendingStylesheets();

Does the layout of the rest of the page affect these things at all? If they're independently positioned and sized, isn't this a bit overkill? (but also maybe necessary? I don't know)
Comment 6 Wenson Hsieh 2021-11-29 12:18:34 PST
Comment on attachment 444773 [details]
Fix unused param

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

Thanks for the review!

>> Source/WebCore/dom/ImageOverlay.cpp:306
>> +        rootContainer->setTranslate(false);
> 
> Seems like a nontrivial behavior change that should be in its own patch?

True! I will revert this change before landing, and file a separate bug for it.

>> Source/WebCore/dom/ImageOverlay.cpp:536
>> +        document->updateLayoutIgnorePendingStylesheets();
> 
> Does the layout of the rest of the page affect these things at all? If they're independently positioned and sized, isn't this a bit overkill? (but also maybe necessary? I don't know)

That's a good point — after chatting with Alan, it looks like each layout update here should be relatively cheap (assuming the rest of the document is laid out), since each of the boxes where we're changing font sizes are absolutely positioned.

I *think* we could use CSS containment to avoid part of the tree walk, but I think that cost is likely much less than that of running line breaking.
Comment 7 Wenson Hsieh 2021-11-29 12:28:33 PST
Created attachment 445325 [details]
Rebase on trunk
Comment 8 Wenson Hsieh 2021-11-29 12:59:42 PST
Created attachment 445328 [details]
Fix unused param
Comment 9 EWS 2021-11-29 14:47:43 PST
Committed r286265 (244628@main): <https://commits.webkit.org/244628@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445328 [details].