Bug 199506 - [ContentChangeObserver] didFinishTransition triggers a nested style recalc via isConsideredClickable
Summary: [ContentChangeObserver] didFinishTransition triggers a nested style recalc vi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-04 16:19 PDT by zalan
Modified: 2019-07-09 05:09 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.71 KB, patch)
2019-07-04 16:57 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (6.65 KB, patch)
2019-07-04 20:54 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (6.61 KB, patch)
2019-07-04 21:06 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2019-07-04 16:19:44 PDT
<rdar://problem/52656221>
Comment 1 zalan 2019-07-04 16:57:01 PDT
Created attachment 373475 [details]
Patch
Comment 2 zalan 2019-07-04 20:54:12 PDT
Created attachment 373476 [details]
Patch
Comment 3 zalan 2019-07-04 21:06:29 PDT
Created attachment 373477 [details]
Patch
Comment 4 zalan 2019-07-04 21:06:51 PDT
^^more reliable test case
Comment 5 Antti Koivisto 2019-07-04 21:42:04 PDT
Another option would be to add a parameter to willRespondToMouseClickEvents that prevents style update. computeEditability() where the nested recalc is entered already takes such parameter.
Comment 6 zalan 2019-07-04 21:50:31 PDT
(In reply to Antti Koivisto from comment #5)
> Another option would be to add a parameter to willRespondToMouseClickEvents
> that prevents style update. computeEditability() where the nested recalc is
> entered already takes such parameter.
There's another isConsideredClickable() callsite (which I can't make async) and I was planning to do exactly that in there.
Comment 7 WebKit Commit Bot 2019-07-04 22:44:23 PDT
Comment on attachment 373477 [details]
Patch

Clearing flags on attachment: 373477

Committed r247147: <https://trac.webkit.org/changeset/247147>
Comment 8 WebKit Commit Bot 2019-07-04 22:44:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-07-04 22:45:18 PDT
<rdar://problem/52669516>
Comment 10 Ryosuke Niwa 2019-07-05 01:10:27 PDT
(In reply to zalan from comment #6)
> (In reply to Antti Koivisto from comment #5)
> > Another option would be to add a parameter to willRespondToMouseClickEvents
> > that prevents style update. computeEditability() where the nested recalc is
> > entered already takes such parameter.
> There's another isConsideredClickable() callsite (which I can't make async)
> and I was planning to do exactly that in there.

Is that guaranteed to be correct? computeEditability needs the up-to-date style. In cases where computeEditability is called and we don't end up updating the style due to this flag, is the style computeEditability uses guaranteed to be up-to-date?
Comment 11 Antti Koivisto 2019-07-05 04:42:43 PDT
Presumably the heuristics would trigger when the style actually changes, if needed.
Comment 12 zalan 2019-07-06 06:14:29 PDT
Actually the other callsite is at RenderTreeUpdater::updateElementRenderer() (ContentChangeObserver::StyleChangeScope -> ~StyleChangeScope). I assume by the time we get here all the styles are resolved and it's safe to call computeEditability. Antti?
Comment 13 Ryosuke Niwa 2019-07-08 19:36:41 PDT
(In reply to zalan from comment #12)
> Actually the other callsite is at RenderTreeUpdater::updateElementRenderer()
> (ContentChangeObserver::StyleChangeScope -> ~StyleChangeScope). I assume by
> the time we get here all the styles are resolved and it's safe to call
> computeEditability. Antti?

Why can't the code in StyleChangeScope::~StyleChangeScope() be pushed as a post style update task??
Comment 14 Antti Koivisto 2019-07-09 05:09:22 PDT
(In reply to zalan from comment #12)
> Actually the other callsite is at RenderTreeUpdater::updateElementRenderer()
> (ContentChangeObserver::StyleChangeScope -> ~StyleChangeScope). I assume by
> the time we get here all the styles are resolved and it's safe to call
> computeEditability. Antti?

Yes, it should be safe. Style has just been updated by updateElementRenderer when  ~StyleChangeScope runs. Cutting close though.