ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
recreates a popup by a HEAD element change.

This CL adds a |reason| argument to PopupMenu::updateFromElement. It is one of
BySelectionChange, ByStyleChange, and ByDOMCHange. ExternalPopupMenu doesn't
recreates its popup if the |reason| is ByStyleChange because ExternalPopupMenu
doesn't support styling.

BUG=608140
Committed: https://crrev.com/1d5b6507ac2cbfdbb733cbefa1dbbce091b11a06
Cr-Commit-Position: refs/heads/master@{#392965}

[tkent, 2016-05-11 06:32:06]

Description was changed from

==========
Don't call PopupMenu::updateFromElement on HTMLSelectElement::didRecalcStyle.

BUG=608140
==========

to

==========
Don't call PopupMenu::updateFromElement in HTMLSelectElement::didRecalcStyle.

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
refreshes the popup.

We wonder if updating popup only for style change is practically necessary. So
this CL removes PopupMenu::updateFromElement in
HTMLSelectElement::didRecalcStyle.
However, we relied on this callsite to update popup on adding/removing OPTIONs
from the SELECT element.  So this CL adds an internal MutationObserver to detect
DOM tree changes.

BUG=608140
==========

[tkent, 2016-05-11 06:32:19]

tkent@chromium.org changed reviewers:
+ keishi@chromium.org

[tkent, 2016-05-11 06:32:19]

Keishi, would you review this please?

[keishi, 2016-05-11 07:17:32]

On 2016/05/11 06:32:19, tkent wrote:
> Keishi, would you review this please?

Do we have to give up "realtime styling of popup menus" completely? Isn't this a
problem just with ExternalPopupMenu? PopupMenuImpl can handle the test case in
the bug properly.

[tkent, 2016-05-11 07:23:28]

On 2016/05/11 at 07:17:32, keishi wrote:
> On 2016/05/11 06:32:19, tkent wrote:
> > Keishi, would you review this please?
> 
> Do we have to give up "realtime styling of popup menus" completely? Isn't this
a problem just with ExternalPopupMenu? PopupMenuImpl can handle the test case in
the bug properly.

Actually, realtime popup styling will work well in many cases even with this CL
because updating style= or class= of a SELECT/OPTION element triggers
MutationObserver.  This CL will drop only style change without DOM mutation such
as CSS animation and style changes in ancestors.

[keishi, 2016-05-11 07:48:00]

On 2016/05/11 07:23:28, tkent wrote:
> On 2016/05/11 at 07:17:32, keishi wrote:
> > On 2016/05/11 06:32:19, tkent wrote:
> > > Keishi, would you review this please?
> > 
> > Do we have to give up "realtime styling of popup menus" completely? Isn't
this
> a problem just with ExternalPopupMenu? PopupMenuImpl can handle the test case
in
> the bug properly.
> 
> Actually, realtime popup styling will work well in many cases even with this
CL
> because updating style= or class= of a SELECT/OPTION element triggers
> MutationObserver.  This CL will drop only style change without DOM mutation
such
> as CSS animation and style changes in ancestors.

Do we want to ease up on updating the popup menu for security reasons?

Otherwise I think updating the style even for ancestor changes is the proper
behavior and it is ExternalPopupMenu's fault for failing to handle style updates
properly.
So to fix ExternalPopupMenu in the short term I thought maybe we could replace
ExternalPopupMenu::m_shownDOMTreeVersion with a flag that is turned on by the
MutationObserver in this CL.

[tkent, 2016-05-11 07:59:02]

On 2016/05/11 at 07:48:00, keishi wrote:
> On 2016/05/11 07:23:28, tkent wrote:
> > On 2016/05/11 at 07:17:32, keishi wrote:
> > > On 2016/05/11 06:32:19, tkent wrote:
> > > > Keishi, would you review this please?
> > > 
> > > Do we have to give up "realtime styling of popup menus" completely? Isn't
this
> > a problem just with ExternalPopupMenu? PopupMenuImpl can handle the test
case in
> > the bug properly.
> > 
> > Actually, realtime popup styling will work well in many cases even with this
CL
> > because updating style= or class= of a SELECT/OPTION element triggers
> > MutationObserver.  This CL will drop only style change without DOM mutation
such
> > as CSS animation and style changes in ancestors.
> 
> Do we want to ease up on updating the popup menu for security reasons?
> 
> Otherwise I think updating the style even for ancestor changes is the proper
behavior and it is ExternalPopupMenu's fault for failing to handle style updates
properly.
> So to fix ExternalPopupMenu in the short term I thought maybe we could replace
ExternalPopupMenu::m_shownDOMTreeVersion with a flag that is turned on by the
MutationObserver in this CL.

No security reasons.

Yeah, PopupMenuImpl doesn't have the issue. The approach should be:
 - didRecalcStyle calls PopupMenu::updateFromElement as ever.
 - Introduce PopupMenu::updateFromElementWithMutation
 - ExternalPopupMenu::updateFromElement does nothing.
 - ExternalPopupMenu::updateFromElementWithMutation will recreate popup
 - PopupMenuImpl::updateFromElementWithMutation just calls
PopupMenuImpl::updateFromElement

WDYT?

[keishi, 2016-05-11 08:04:50]

On 2016/05/11 07:59:02, tkent wrote:
> On 2016/05/11 at 07:48:00, keishi wrote:
> > On 2016/05/11 07:23:28, tkent wrote:
> > > On 2016/05/11 at 07:17:32, keishi wrote:
> > > > On 2016/05/11 06:32:19, tkent wrote:
> > > > > Keishi, would you review this please?
> > > > 
> > > > Do we have to give up "realtime styling of popup menus" completely?
Isn't
> this
> > > a problem just with ExternalPopupMenu? PopupMenuImpl can handle the test
> case in
> > > the bug properly.
> > > 
> > > Actually, realtime popup styling will work well in many cases even with
this
> CL
> > > because updating style= or class= of a SELECT/OPTION element triggers
> > > MutationObserver.  This CL will drop only style change without DOM
mutation
> such
> > > as CSS animation and style changes in ancestors.
> > 
> > Do we want to ease up on updating the popup menu for security reasons?
> > 
> > Otherwise I think updating the style even for ancestor changes is the proper
> behavior and it is ExternalPopupMenu's fault for failing to handle style
updates
> properly.
> > So to fix ExternalPopupMenu in the short term I thought maybe we could
replace
> ExternalPopupMenu::m_shownDOMTreeVersion with a flag that is turned on by the
> MutationObserver in this CL.
> 
> No security reasons.
> 
> Yeah, PopupMenuImpl doesn't have the issue. The approach should be:
>  - didRecalcStyle calls PopupMenu::updateFromElement as ever.
>  - Introduce PopupMenu::updateFromElementWithMutation
>  - ExternalPopupMenu::updateFromElement does nothing.
>  - ExternalPopupMenu::updateFromElementWithMutation will recreate popup
>  - PopupMenuImpl::updateFromElementWithMutation just calls
> PopupMenuImpl::updateFromElement
> 
> WDYT?

Sounds great!

[tkent, 2016-05-11 08:19:36]

Description was changed from

==========
Don't call PopupMenu::updateFromElement in HTMLSelectElement::didRecalcStyle.

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
refreshes the popup.

We wonder if updating popup only for style change is practically necessary. So
this CL removes PopupMenu::updateFromElement in
HTMLSelectElement::didRecalcStyle.
However, we relied on this callsite to update popup on adding/removing OPTIONs
from the SELECT element.  So this CL adds an internal MutationObserver to detect
DOM tree changes.

BUG=608140
==========

to

==========
ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
recreates a popup by a HEAD element change.

This CL introduces PopupMenu::updateFromElementWithMutation. It is called when
a SELECT element subtree is modified. ExternalPopupMenu recreates its popup only
in updateFromElementWithMutation, and does nothing in updateFromElement because
ExternalPopupMenu doesn't support styling.

BUG=608140
==========

[tkent, 2016-05-11 08:35:17]

Uploaded a new patch.  Please take another look.

[keishi, 2016-05-11 09:14:41]

LGTM

https://codereview.chromium.org/1970653003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/1970653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSelectElement.cpp:1051:
m_popup->updateFromElement();
nit: Maybe selectedness is important enough that we should update the
ExternalPopupMenu?

[tkent, 2016-05-11 15:26:46]

Description was changed from

==========
ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
recreates a popup by a HEAD element change.

This CL introduces PopupMenu::updateFromElementWithMutation. It is called when
a SELECT element subtree is modified. ExternalPopupMenu recreates its popup only
in updateFromElementWithMutation, and does nothing in updateFromElement because
ExternalPopupMenu doesn't support styling.

BUG=608140
==========

to

==========
ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
recreates a popup by a HEAD element change.

This CL adds a |reason| argument to PopupMenu::updateFromElement. It is one of
BySelectionChange, ByStyleChange, and ByDOMCHange. ExternalPopupMenu doesn't
recreates its popup if the |reason| is ByStyleChange because ExternalPopupMenu
doesn't support styling.

BUG=608140
==========

[tkent, 2016-05-11 15:56:14]

https://codereview.chromium.org/1970653003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/1970653003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSelectElement.cpp:1051:
m_popup->updateFromElement();
On 2016/05/11 at 09:14:41, keishi wrote:
> nit: Maybe selectedness is important enough that we should update the
ExternalPopupMenu?

ok, I added an enum argument to updateFromElement instead of adding
updateFromElementWithMutation.

[tkent, 2016-05-11 15:58:39]

The CQ bit was checked by tkent@chromium.org

[tkent, 2016-05-11 15:58:40]

The patchset sent to the CQ was uploaded after l-g-t-m from keishi@chromium.org
Link to the patchset: https://codereview.chromium.org/1970653003/#ps60001
(title: "Add an enum argument to updateFromElement")

[commit-bot: I haz the power, 2016-05-11 15:58:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970653003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970653003/60001

[commit-bot: I haz the power, 2016-05-11 17:13:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-11 17:15:02]

Description was changed from

==========
ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
recreates a popup by a HEAD element change.

This CL adds a |reason| argument to PopupMenu::updateFromElement. It is one of
BySelectionChange, ByStyleChange, and ByDOMCHange. ExternalPopupMenu doesn't
recreates its popup if the |reason| is ByStyleChange because ExternalPopupMenu
doesn't support styling.

BUG=608140
==========

to

==========
ExternalPopupMenu should recreate its popup only if a SELECT subtree is updated

crbug.com/608140 happens because animation on a SELECT element causes
multiple didRecalcStyle calls, and ExternalPopupMenu::updateFromElement
recreates a popup by a HEAD element change.

This CL adds a |reason| argument to PopupMenu::updateFromElement. It is one of
BySelectionChange, ByStyleChange, and ByDOMCHange. ExternalPopupMenu doesn't
recreates its popup if the |reason| is ByStyleChange because ExternalPopupMenu
doesn't support styling.

BUG=608140

Committed: https://crrev.com/1d5b6507ac2cbfdbb733cbefa1dbbce091b11a06
Cr-Commit-Position: refs/heads/master@{#392965}
==========

[commit-bot: I haz the power, 2016-05-11 17:15:05]

Patchset 4 (id:??) landed as
https://crrev.com/1d5b6507ac2cbfdbb733cbefa1dbbce091b11a06
Cr-Commit-Position: refs/heads/master@{#392965}