Stop matching scrollbar pseudo element without a scrollbar.

Stop matching scrollbar pseudo element without a scrollbar.

While matching rules for elements, we mark elements as affected-by-* for
user action pseudo classes like hover. It means that when the element is
later hovered, we need to recalculate style to apply hover styles to
that element.

In general, we currently don't support pseudo classes after pseudo
elements, but for scrollbar pseudo elements we do:

  ::-webkit-scrollbar:hover {}

However, we do not want such rules to mark the element as affected-by-
hover. The hover style on scrollbar parts get their hover style updated
when hovered regardless of any flags, and making scrollbar pseudo
element rules affect hover updates on the actual elements causes
unnecessary style recalcs.

BUG=716006
Review-Url: https://codereview.chromium.org/2850743003
Cr-Commit-Position: refs/heads/master@{#469309}
Committed: https://chromium.googlesource.com/chromium/src/+/40bdd179053a9b7c6c7eb3a82462a860f981e5d8

[rune, 2017-04-28 13:31:49]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 13:32:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 17:29:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 17:29:29]

Dry run: This issue passed the CQ dry run.

[rune, 2017-04-28 21:37:35]

rune@opera.com changed reviewers:
+ esprehn@chromium.org

[rune, 2017-04-28 21:37:35]

ptal

Re-introducing parts of: https://codereview.chromium.org/1605523002

[esprehn, 2017-04-28 22:07:35]

How do affected by bits impact table layout? This makes me nervous there's a
hidden style sharing but we're covering up.

[esprehn, 2017-04-28 22:07:52]

On 2017/04/28 at 22:07:35, esprehn wrote:
> How do affected by bits impact table layout? This makes me nervous there's a
hidden style sharing *bug* we're covering up.

^ bug.

[rune, 2017-04-28 22:36:32]

On 2017/04/28 22:07:52, esprehn wrote:
> On 2017/04/28 at 22:07:35, esprehn wrote:
> > How do affected by bits impact table layout? This makes me nervous there's a
> hidden style sharing *bug* we're covering up.
> 
> ^ bug.

I doubt it's style sharing. I've debugged down to finding that we are reducing
the 50% table cell, which contains the radio button we are clicking, by 2px.
That is the same as the HBorderSpacing() in [1], so it could be that subtracted
twice re-layouting parts of the table, but I'm not familar with the code yet.

You're right that this change will cover up the real issue here, so we could
hold off this CL while searching for the real issue.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

[esprehn, 2017-04-28 22:43:09]

On 2017/04/28 at 22:36:32, rune wrote:
> On 2017/04/28 22:07:52, esprehn wrote:
> > On 2017/04/28 at 22:07:35, esprehn wrote:
> > > How do affected by bits impact table layout? This makes me nervous there's
a
> > hidden style sharing *bug* we're covering up.
> > 
> > ^ bug.
> 
> I doubt it's style sharing. I've debugged down to finding that we are reducing
the 50% table cell, which contains the radio button we are clicking, by 2px.
That is the same as the HBorderSpacing() in [1], so it could be that subtracted
twice re-layouting parts of the table, but I'm not familar with the code yet.
> 
> You're right that this change will cover up the real issue here, so we could
hold off this CL while searching for the real issue.
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

But why would the RestyleFlags impact something in the layout code? The only
place we really use them is inside style sharing and scheduling invalidations...
is there a different number of elements going through recalcStyle with/without
the patch?

[rune, 2017-05-02 13:58:06]

On 2017/04/28 22:43:09, esprehn wrote:
> On 2017/04/28 at 22:36:32, rune wrote:
> > On 2017/04/28 22:07:52, esprehn wrote:
> > > On 2017/04/28 at 22:07:35, esprehn wrote:
> > > > How do affected by bits impact table layout? This makes me nervous
there's
> a
> > > hidden style sharing *bug* we're covering up.
> > > 
> > > ^ bug.
> > 
> > I doubt it's style sharing. I've debugged down to finding that we are
reducing
> the 50% table cell, which contains the radio button we are clicking, by 2px.
> That is the same as the HBorderSpacing() in [1], so it could be that
subtracted
> twice re-layouting parts of the table, but I'm not familar with the code yet.
> > 
> > You're right that this change will cover up the real issue here, so we could
> hold off this CL while searching for the real issue.
> > 
> > [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> 
> But why would the RestyleFlags impact something in the layout code? The only
> place we really use them is inside style sharing and scheduling
invalidations...
> is there a different number of elements going through recalcStyle with/without
> the patch?

LayoutTable::StyleDidChange() seems to have a bug. I've been able to trigger the
issue by causing SetStyle() on the table prior to a re-layout of out-of-flow
contents of a table cell. Got rid of scrollbar pseudos and affected-by bits in
the repro. Perhaps a change in affected-by bits may caused StyleDidChange()
where the style otherwise did not change?

Attempt at fixing the layout code:

https://codereview.chromium.org/2855853002/

[rune, 2017-05-03 09:12:00]

On 2017/05/02 13:58:06, rune wrote:
> On 2017/04/28 22:43:09, esprehn wrote:
> > On 2017/04/28 at 22:36:32, rune wrote:
> > > On 2017/04/28 22:07:52, esprehn wrote:
> > > > On 2017/04/28 at 22:07:35, esprehn wrote:
> > > > > How do affected by bits impact table layout? This makes me nervous
> there's
> > a
> > > > hidden style sharing *bug* we're covering up.
> > > > 
> > > > ^ bug.
> > > 
> > > I doubt it's style sharing. I've debugged down to finding that we are
> reducing
> > the 50% table cell, which contains the radio button we are clicking, by 2px.
> > That is the same as the HBorderSpacing() in [1], so it could be that
> subtracted
> > twice re-layouting parts of the table, but I'm not familar with the code
yet.
> > > 
> > > You're right that this change will cover up the real issue here, so we
could
> > hold off this CL while searching for the real issue.
> > > 
> > > [1]
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> > 
> > But why would the RestyleFlags impact something in the layout code? The only
> > place we really use them is inside style sharing and scheduling
> invalidations...
> > is there a different number of elements going through recalcStyle
with/without
> > the patch?
> 
> LayoutTable::StyleDidChange() seems to have a bug. I've been able to trigger
the
> issue by causing SetStyle() on the table prior to a re-layout of out-of-flow
> contents of a table cell. Got rid of scrollbar pseudos and affected-by bits in
> the repro. Perhaps a change in affected-by bits may caused StyleDidChange()
> where the style otherwise did not change?
> 
> Attempt at fixing the layout code:
> 
> https://codereview.chromium.org/2855853002/

So, I just landed a fix for the LayoutTable bug.

What I think happens with the affected-by bits, is that you have a rule:

  ::-webkit-scrollbar:hover {}

which sets the affected-by-hover flag on all elements without the change in this
CL. When hovering the table to click the radio button in the Google Docs
spreadsheet, we would recalculate the style for the table element and nudge the
first column position in LayoutTable::StyleDidChange() even if the style didn't
actually change. Then, clicking the radio button would trigger a re-layout of
the out-of-flow content inside the table cell, but since the in-flow content of
the table cell didn't change, the column positions would not be re-calculated,
but the incorrect column position set in LayoutTable::StyleDidChange() would be
used to set the table cell to a new position. With the change in this CL, we
would skip the style recalc on hovering the table as the affected-by-hover flag
would not be set, hence the StyleDidChange() on the table would not happen.

I still think it makes sense to land this CL since scrollbar rules like the one
above causes every element in a document to have the affected-by-hover flag set
without this fix.

[esprehn, 2017-05-03 18:09:44]

lgtm, I think you want to update your change description to talk about avoiding
setting the bit for everything in the situation you described above and remove
the section about the table layout bug since that's unrelated.

[rune, 2017-05-04 08:42:04]

Description was changed from

==========
Stop matching scrollbar pseudo element without a scrollbar.

We may see user action pseudo classes after scrollbar pseudo elements.
We should avoid marking elements with affected-by flags on the actual
elements when they appear after the pseudo element. For instance, while
matching the div below, we would mark the div affected-by-hover without
this change. While actually matching the scrollbar pseudo element,
there is a separate code path (CheckScrollbarPseudoClass()) which will
not mark the actual element:

  div::-webkit-scrollbar-thumb:hover {}

This fixes issue 716006 for the print dialog for google docs
spreadsheets, but the underlying problem is more likely a table layout
issue.

BUG=716006
==========

to

==========
Stop matching scrollbar pseudo element without a scrollbar.

While matching rules for elements, we mark elements as affected-by-* for
user action pseudo classes like hover. It means that when the element is
later hovered, we need to recalculate style to apply hover styles to
that element.

In general, we currently don't support pseudo classes after pseudo
elements, but for scrollbar pseudo elements we do:

  ::-webkit-scrollbar:hover {}

However, we do not want such rules to mark the element as affected-by-
hover. The hover style on scrollbar parts get their hover style updated
when hovered regardless of any flags, and making scrollbar pseudo
element rules affect hover updates on the actual elements causes
unnecessary style recalcs.

BUG=716006
==========

[rune, 2017-05-04 08:42:19]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2017-05-04 08:42:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 10:40:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 10:40:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rune, 2017-05-04 10:52:01]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2017-05-04 10:52:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 11:34:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493895121165200,
"parent_rev": "7d472f296e715c26fa1dbe8295b607421319c53f", "commit_rev":
"40bdd179053a9b7c6c7eb3a82462a860f981e5d8"}

[commit-bot: I haz the power, 2017-05-04 11:34:42]

Description was changed from

==========
Stop matching scrollbar pseudo element without a scrollbar.

While matching rules for elements, we mark elements as affected-by-* for
user action pseudo classes like hover. It means that when the element is
later hovered, we need to recalculate style to apply hover styles to
that element.

In general, we currently don't support pseudo classes after pseudo
elements, but for scrollbar pseudo elements we do:

  ::-webkit-scrollbar:hover {}

However, we do not want such rules to mark the element as affected-by-
hover. The hover style on scrollbar parts get their hover style updated
when hovered regardless of any flags, and making scrollbar pseudo
element rules affect hover updates on the actual elements causes
unnecessary style recalcs.

BUG=716006
==========

to

==========
Stop matching scrollbar pseudo element without a scrollbar.

While matching rules for elements, we mark elements as affected-by-* for
user action pseudo classes like hover. It means that when the element is
later hovered, we need to recalculate style to apply hover styles to
that element.

In general, we currently don't support pseudo classes after pseudo
elements, but for scrollbar pseudo elements we do:

  ::-webkit-scrollbar:hover {}

However, we do not want such rules to mark the element as affected-by-
hover. The hover style on scrollbar parts get their hover style updated
when hovered regardless of any flags, and making scrollbar pseudo
element rules affect hover updates on the actual elements causes
unnecessary style recalcs.

BUG=716006

Review-Url: https://codereview.chromium.org/2850743003
Cr-Commit-Position: refs/heads/master@{#469309}
Committed:
https://chromium.googlesource.com/chromium/src/+/40bdd179053a9b7c6c7eb3a82462...
==========

[commit-bot: I haz the power, 2017-05-04 11:34:44]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/40bdd179053a9b7c6c7eb3a82462...