Make `getComputedStyle` return the correct style for collapsed <img> elements.

Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. This code path is also oblivious of style
customizations that would otherwise be applied during style recalculation by
Element::customStyleForLayoutObject for some element types.

Therefore this CL instead implements the `collapsed` state by explicitly
setting the `display` property from StyleAdjuster::adjustComputedStyle, which
is consulted directly by StyleResolver::styleForElement, and is therefore
applied in both Element::ensureComputedStyle and Element::styleForLayoutObject.

As a side effect, SharedStyleFinder now has to perform HTLMImageElement-specific
checks to prevent sharing styles between `collapsed` and `non-collapsed` images.
Marking the ComputedStyle for `collapsed` images as unique is not sufficient,
because it does not prevent reusing the ComputedStyle from a `non-collapsed`
image element on a `collapsed` one.

BUG=609747, 682243
Review-Url: https://codereview.chromium.org/2627953003
Cr-Commit-Position: refs/heads/master@{#450694}
Committed: https://chromium.googlesource.com/chromium/src/+/d77ba4c9ecb5315db4dc26ac3f42ff767e94ae63

[engedy, 2017-01-12 15:20:59]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 15:21:32]

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-01-12 16:38:19]

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

[commit-bot: I haz the power, 2017-01-12 16:38:20]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-01-12 18:05:41]

Description was changed from

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

BUG=609747
==========

to

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. To make things worse, this code path was also
oblivious of style customizations that would otherwise be applied during style
recalculation by Element::customStyleForLayoutObject for some element types.

This CL instead implements the `collapsed` state by means of explicitly setting
the `display` property in HTMLImageElement::customStyleForLayoutObject, and
making the style non-shareable.

Because ultimately we still do not create a LayoutObject that would otherwise
retain the effective ComputedStyle, the style is cached on the element and
returned through Node::nonLayoutObjectComputedStyle, which is consulted by
Node::computedStyle. So from now on, both internal callers, as well as DOM APIs
can be provided with correct style.

BUG=609747
==========

[engedy, 2017-01-12 18:05:41]

engedy@chromium.org changed reviewers:
+ dominicc@chromium.org, esprehn@chromium.org

[engedy, 2017-01-12 18:12:27]

@Elliott, @Dominic: This is a follow-up to [1] to address the discrepancy
between the DOM-visible ComputedStyle and the effective ComputedStyle for
collapsed image elements.

I encountered quite a few pitfalls on the way, so please take a close look.

[1] https://codereview.chromium.org/2535383003/

[esprehn, 2017-01-12 19:45:38]

Why do you need to cache the style like <option> ? I don't think you need that
complexity, just call setDisplay() inside customStyleForLayoutObject and
setUnique() ?

[engedy, 2017-01-12 20:21:19]

On 2017/01/12 19:45:38, esprehn wrote:
> Why do you need to cache the style like <option> ? I don't think you need that
> complexity, just call setDisplay() inside customStyleForLayoutObject and
> setUnique() ?

Then the customized ComputedStyle is destroyed as soon as
Element::attachLayoutTree() returns, as there is no layout object to retain it.

Later, when `window.getComputedStyle` is called, Element::ensureComputedStyle
will try to on-demand recalculate the style, but it is not prepared to deal with
elements that tweak their ComputedStyle in customStyleForLayoutObject. So we
either need to:
  1.) Cache the effective ComputedStyle on the element (this CL),
  2.) Make customStyleForLayoutObject cache the ComputedStyle in
NodeRareData::computedStyle,
  3.) Keep the on-demand calculation in Element::ensureComputedStyle [1], but
make it send the style-ignoring-pending-stylesheets through
customStyleForLayoutObject.

In contrast to (1), with alternatives (2 + 3), calls to Node::computedStyle()
will still return nullptr, as it currently does not consult
NodeRareData::computedStyle.

[1]:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme...

[esprehn, 2017-01-12 20:28:06]

Yeah interesting, that is a bug in the design of this system that modifications
are lost. In general customStyleForLayoutObject is a bug. Perhaps we should stop
using customStyleForLayoutObject here and use adjustStyleForHTMLElement with an
isHTMLImageElement check and then force things to display: none there?

[engedy, 2017-01-12 20:42:58]

On 2017/01/12 20:28:06, esprehn wrote:
> Yeah interesting, that is a bug in the design of this system that
modifications
> are lost. In general customStyleForLayoutObject is a bug. Perhaps we should
stop
> using customStyleForLayoutObject here and use adjustStyleForHTMLElement with
an
> isHTMLImageElement check and then force things to display: none there?

Yeah, that could work if we can live with the following caveats:
 1) The ComputedStyle will still not be retained after layout, so the first call
to getComputedStyle will recalculate it (and then cache it on NodeRareData).
 2) We still need to keep HTMLImageElement::customStyleForLayoutObject to handle
the fallback content case, so the style customization will be split into two
places, unless we are okay with essentially replacing the style in
adjustStyleForHTMLElement (by calling
HTMLImageFallbackHelper::customStyleForAltText from there).

[engedy, 2017-01-18 21:58:05]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 21:58:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-01-18 21:59:59]

Patchset #2 (id:20001) has been deleted

[engedy, 2017-01-18 22:01:02]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 22:01:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-01-18 22:02:47]

Done, please take another look. One more caveat:

3) Now we have an element-specific check in SharedStyleFinder. :-(

[engedy, 2017-01-18 22:13:13]

Description was changed from

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. To make things worse, this code path was also
oblivious of style customizations that would otherwise be applied during style
recalculation by Element::customStyleForLayoutObject for some element types.

This CL instead implements the `collapsed` state by means of explicitly setting
the `display` property in HTMLImageElement::customStyleForLayoutObject, and
making the style non-shareable.

Because ultimately we still do not create a LayoutObject that would otherwise
retain the effective ComputedStyle, the style is cached on the element and
returned through Node::nonLayoutObjectComputedStyle, which is consulted by
Node::computedStyle. So from now on, both internal callers, as well as DOM APIs
can be provided with correct style.

BUG=609747
==========

to

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. This code path is also oblivious of style
customizations that would otherwise be applied during style recalculation by
Element::customStyleForLayoutObject for some element types.

Therefore this CL instead implements the `collapsed` state by explicitly
setting the `display` property from StyleAdjuster::adjustComputedStyle, which
is consulted directly by StyleResolver::styleForElement, and is therefore
applied in both Element::ensureComputedStyle and Element::styleForLayoutObject.

As a side effect, SharedStyleFinder now has to perform HTLMImageElement-specific
checks to prevent sharing styles between collapsed and non-collapsed images.

BUG=609747,682243
==========

[commit-bot: I haz the power, 2017-01-18 23:27:11]

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

[commit-bot: I haz the power, 2017-01-18 23:27:12]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-01-20 16:52:52]

@Elliott, what do you think of caveats 1..3?

[engedy, 2017-01-24 17:43:57]

Friendly ping.

[engedy, 2017-02-08 10:22:11]

@Elliott, friendly ping.

[esprehn, 2017-02-15 02:08:46]

lgtm

https://codereview.chromium.org/2627953003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp (right):

https://codereview.chromium.org/2627953003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp:292:
toHTMLImageElement(element()).isCollapsed()) {
good catch!

[esprehn, 2017-02-15 02:09:26]

On 2017/01/18 at 22:02:47, engedy wrote:
> Done, please take another look. One more caveat:
> 
> 3) Now we have an element-specific check in SharedStyleFinder. :-(

Yeah so as an alternative you could do setUnique() inside StyleAdjuster instead.
That prevents sharing in this case, but it's probably rare so it's okay?

[engedy, 2017-02-15 13:18:24]

On 2017/02/15 02:09:26, esprehn wrote:
> On 2017/01/18 at 22:02:47, engedy wrote:
> > Done, please take another look. One more caveat:
> > 
> > 3) Now we have an element-specific check in SharedStyleFinder. :-(
> 
> Yeah so as an alternative you could do setUnique() inside StyleAdjuster
instead.
> That prevents sharing in this case, but it's probably rare so it's okay?

That's what I initially wanted to do, but it's not sufficient: it prevents
reusing the ComputedStyle from a collapsed image for a normal image, but not
vice versa. Obviously we don't want to disable sharing style for normal images,
so this seems to be the only solution.

[engedy, 2017-02-15 13:21:50]

Description was changed from

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. This code path is also oblivious of style
customizations that would otherwise be applied during style recalculation by
Element::customStyleForLayoutObject for some element types.

Therefore this CL instead implements the `collapsed` state by explicitly
setting the `display` property from StyleAdjuster::adjustComputedStyle, which
is consulted directly by StyleResolver::styleForElement, and is therefore
applied in both Element::ensureComputedStyle and Element::styleForLayoutObject.

As a side effect, SharedStyleFinder now has to perform HTLMImageElement-specific
checks to prevent sharing styles between collapsed and non-collapsed images.

BUG=609747,682243
==========

to

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. This code path is also oblivious of style
customizations that would otherwise be applied during style recalculation by
Element::customStyleForLayoutObject for some element types.

Therefore this CL instead implements the `collapsed` state by explicitly
setting the `display` property from StyleAdjuster::adjustComputedStyle, which
is consulted directly by StyleResolver::styleForElement, and is therefore
applied in both Element::ensureComputedStyle and Element::styleForLayoutObject.

As a side effect, SharedStyleFinder now has to perform HTLMImageElement-specific
checks to prevent sharing styles between `collapsed` and `non-collapsed` images.
Marking the ComputedStyle for `collapsed` images as unique is not sufficient,
because it does not prevent reusing the ComputedStyle from a `non-collapsed`
image element on a `collapsed` one.

BUG=609747,682243
==========

[engedy, 2017-02-15 13:23:18]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-02-15 13:23:19]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2627953003/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-15 13:23:36]

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-02-15 14:51:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487164998785410,
"parent_rev": "eb1284eaf4c4a1edffe7800e211a9b940410d2a3", "commit_rev":
"d77ba4c9ecb5315db4dc26ac3f42ff767e94ae63"}

[commit-bot: I haz the power, 2017-02-15 14:52:08]

Description was changed from

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. This code path is also oblivious of style
customizations that would otherwise be applied during style recalculation by
Element::customStyleForLayoutObject for some element types.

Therefore this CL instead implements the `collapsed` state by explicitly
setting the `display` property from StyleAdjuster::adjustComputedStyle, which
is consulted directly by StyleResolver::styleForElement, and is therefore
applied in both Element::ensureComputedStyle and Element::styleForLayoutObject.

As a side effect, SharedStyleFinder now has to perform HTLMImageElement-specific
checks to prevent sharing styles between `collapsed` and `non-collapsed` images.
Marking the ComputedStyle for `collapsed` images as unique is not sufficient,
because it does not prevent reusing the ComputedStyle from a `non-collapsed`
image element on a `collapsed` one.

BUG=609747,682243
==========

to

==========
Make `getComputedStyle` return the correct style for collapsed <img> elements.

HTMLImageElements can be in the `collapsed` state, meaning they are not
included in the layout. Previously, this was implemented by overriding
layoutObjectIsNeeded() to return false so that no LayoutObject would be
created.

A disadvantage of this solution was that the style returned through the
`window.getComputedStyle` method was not consistent with the element's
effective style, for example, the `display` property was still set to `inline`.

As a matter of fact, the ComputedStyle calculated during layout was not even
persisted, and the style returned through the DOM API was calculated on-demand
in Element::ensureComputedStyle. This code path is also oblivious of style
customizations that would otherwise be applied during style recalculation by
Element::customStyleForLayoutObject for some element types.

Therefore this CL instead implements the `collapsed` state by explicitly
setting the `display` property from StyleAdjuster::adjustComputedStyle, which
is consulted directly by StyleResolver::styleForElement, and is therefore
applied in both Element::ensureComputedStyle and Element::styleForLayoutObject.

As a side effect, SharedStyleFinder now has to perform HTLMImageElement-specific
checks to prevent sharing styles between `collapsed` and `non-collapsed` images.
Marking the ComputedStyle for `collapsed` images as unique is not sufficient,
because it does not prevent reusing the ComputedStyle from a `non-collapsed`
image element on a `collapsed` one.

BUG=609747,682243

Review-Url: https://codereview.chromium.org/2627953003
Cr-Commit-Position: refs/heads/master@{#450694}
Committed:
https://chromium.googlesource.com/chromium/src/+/d77ba4c9ecb5315db4dc26ac3f42...
==========

[commit-bot: I haz the power, 2017-02-15 14:52:09]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d77ba4c9ecb5315db4dc26ac3f42...