Bug 214023

Summary: <attachment> layout does not scale with Dynamic Type size changes
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, mmaxfield, pdr, sam, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tim Horton 2020-07-06 17:21:03 PDT
<attachment> layout needs to scale with Dynamic Type size changes
Comment 1 Tim Horton 2020-07-06 17:23:14 PDT
<rdar://problem/64914762>
Comment 2 Tim Horton 2020-07-06 17:23:30 PDT
Created attachment 403645 [details]
Patch
Comment 3 Tim Horton 2020-07-06 17:24:08 PDT
Created attachment 403646 [details]
Patch
Comment 4 Tim Horton 2020-07-06 17:24:54 PDT
Created attachment 403647 [details]
Patch
Comment 5 Sam Weinig 2020-07-06 17:57:40 PDT
Comment on attachment 403647 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests; we don't currently have a mechanism for varying Dynamic Type size in tests.

Seems like the right time to add one!
Comment 6 Tim Horton 2020-07-06 18:10:42 PDT
(In reply to Sam Weinig from comment #5)
> Comment on attachment 403647 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403647&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests; we don't currently have a mechanism for varying Dynamic Type size in tests.
> 
> Seems like the right time to add one!

Only in the "best time to plant a tree" sense. When someone was implementing DT-related font-y features would be the "right time".
Comment 7 Sam Weinig 2020-07-06 18:41:03 PDT
(In reply to Tim Horton from comment #6)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 403647 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403647&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        No new tests; we don't currently have a mechanism for varying Dynamic Type size in tests.
> > 
> > Seems like the right time to add one!
> 
> Only in the "best time to plant a tree" sense. When someone was implementing
> DT-related font-y features would be the "right time".

No time like the present!
Comment 8 Tim Horton 2020-07-06 20:07:38 PDT
(In reply to Sam Weinig from comment #7)
> (In reply to Tim Horton from comment #6)
> > (In reply to Sam Weinig from comment #5)
> > > Comment on attachment 403647 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=403647&action=review
> > > 
> > > > Source/WebCore/ChangeLog:9
> > > > +        No new tests; we don't currently have a mechanism for varying Dynamic Type size in tests.
> > > 
> > > Seems like the right time to add one!
> > 
> > Only in the "best time to plant a tree" sense. When someone was implementing
> > DT-related font-y features would be the "right time".
> 
> No time like the present!

I have no doubt I will come to regret it as much as I always do when I add one-off bits for things like this, but I will see what I can do.
Comment 9 Tim Horton 2020-07-06 22:05:28 PDT
Created attachment 403667 [details]
Patch
Comment 10 Tim Horton 2020-07-07 01:23:17 PDT
Created attachment 403675 [details]
Patch
Comment 11 Sam Weinig 2020-07-07 07:40:29 PDT
(In reply to Tim Horton from comment #8)
> (In reply to Sam Weinig from comment #7)
> > (In reply to Tim Horton from comment #6)
> > > (In reply to Sam Weinig from comment #5)
> > > > Comment on attachment 403647 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=403647&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:9
> > > > > +        No new tests; we don't currently have a mechanism for varying Dynamic Type size in tests.
> > > > 
> > > > Seems like the right time to add one!
> > > 
> > > Only in the "best time to plant a tree" sense. When someone was implementing
> > > DT-related font-y features would be the "right time".
> > 
> > No time like the present!
> 
> I have no doubt I will come to regret it as much as I always do when I add
> one-off bits for things like this, but I will see what I can do.

Awww. Another approach you could take, if you don't want to mess with WebCore::Internals is to add support in WebKitTestRunner (or just do it in TestWebKitAPI) that overrides [WKWebView _contentSizeCategory] and sends the UIContentSizeCategoryDidChangeNotification notification. It might make sense to override [WKWebView _contentSizeCategory] regardless so we have consistent testing on devices that might have had some changes (though I think your call in Internals::resetToConsistentState() should cover most cases).
Comment 12 Tim Horton 2020-07-07 10:25:55 PDT
That's what I assumed I was going to have to do when I was whining. The Internals thing is way less annoying (no bounce to the UI process, no asynchrony in the test, etc.).
Comment 13 EWS 2020-07-07 10:38:31 PDT
Committed r264027: <https://trac.webkit.org/changeset/264027>

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