Bug 199506

Summary:

[ContentChangeObserver] didFinishTransition triggers a nested style recalc via isConsideredClickable

Product:

WebKit

Reporter:

alan <zalan>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

alan

Reported 2019-07-04 16:19:44 PDT

<rdar://problem/52656221>

Attachments
Patch (6.71 KB, patch) 2019-07-04 16:57 PDT, alan	no flags	Details Formatted Diff Diff
Patch (6.65 KB, patch) 2019-07-04 20:54 PDT, alan	no flags	Details Formatted Diff Diff
Patch (6.61 KB, patch) 2019-07-04 21:06 PDT, alan	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2019-07-04 16:57:01 PDT

Created attachment 373475 [details] Patch

alan

Comment 2 2019-07-04 20:54:12 PDT

Created attachment 373476 [details] Patch

alan

Comment 3 2019-07-04 21:06:29 PDT

Created attachment 373477 [details] Patch

alan

Comment 4 2019-07-04 21:06:51 PDT

^^more reliable test case

Antti Koivisto

Comment 5 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.

alan

Comment 6 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.

WebKit Commit Bot

Comment 7 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>

WebKit Commit Bot

Comment 8 2019-07-04 22:44:25 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 9 2019-07-04 22:45:18 PDT

<rdar://problem/52669516>

Ryosuke Niwa

Comment 10 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?

Antti Koivisto

Comment 11 2019-07-05 04:42:43 PDT

Presumably the heuristics would trigger when the style actually changes, if needed.

alan

Comment 12 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?

Ryosuke Niwa

Comment 13 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??

Antti Koivisto

Comment 14 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.

Note You need to log in before you can comment on or make changes to this bug.